[PATCH v2 16/16] drm: ensure drm headers are self-contained and pass kernel-doc

[Masahiro Yamada]

On Mon, Mar 11, 2024 at 9:33 PM Jani Nikula <jani.nikula at intel.com> wrote:
>
> On Fri, 08 Mar 2024, Jani Nikula <jani.nikula at intel.com> wrote:
> > Ensure drm headers build, are self-contained, have header guards, and
> > have no kernel-doc warnings, when CONFIG_DRM_HEADER_TEST=y.
> >
> > The mechanism follows similar patters used in i915, xe, and usr/include.
> >
> > To cover include/drm, we need to recurse there using the top level
> > Kbuild and the new include/Kbuild files.
> >
> > Suggested-by: Daniel Vetter <daniel at ffwll.ch>
> > Cc: David Airlie <airlied at gmail.com>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Maxime Ripard <mripard at kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > Cc: Masahiro Yamada <masahiroy at kernel.org>
> > Acked-by: Thomas Zimmermann <tzimmermann at suse.de>
> > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>
> Masahiro, ack for merging this?


Sorry, I did not notice that I was pinged so long ago.

I just realized this because I saw it in linux-next.

No, NACK.

This was already reverted by
  fcbb8461fd2376ba3782b5b8bd440c929b8e4980

Please do not re-add it.

The reason is explained in the discussion
linked in that commit description.

I do not believe all headers in include/drm/
must be self-contained.

If we know that <drm/drm-foo.h> is included only
when CONFIG_DRM_FOO is enabled, it does not
need to be self-contained when CONFIG_DRM_FOO=n.

So, I am skeptical with this approach, where
the 'find' command detects all headers and they must
be self-contained regardless.

The only case I think makes sense is UAPI headers
(implemented in usr/include/Makefile)


Masahiro



> BR,
> Jani.
>
> > ---
> >  Kbuild                   |  1 +
> >  drivers/gpu/drm/Kconfig  | 11 +++++++++++
> >  drivers/gpu/drm/Makefile | 18 ++++++++++++++++++
> >  include/Kbuild           |  1 +
> >  include/drm/Makefile     | 18 ++++++++++++++++++
> >  5 files changed, 49 insertions(+)
> >  create mode 100644 include/Kbuild
> >  create mode 100644 include/drm/Makefile
> >
> > diff --git a/Kbuild b/Kbuild
> > index 464b34a08f51..f327ca86990c 100644
> > --- a/Kbuild
> > +++ b/Kbuild
> > @@ -97,3 +97,4 @@ obj-$(CONFIG_SAMPLES)       += samples/
> >  obj-$(CONFIG_NET)    += net/
> >  obj-y                        += virt/
> >  obj-y                        += $(ARCH_DRIVERS)
> > +obj-$(CONFIG_DRM_HEADER_TEST)        += include/
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index c08e18108c2a..dd17685ef6e7 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -429,3 +429,14 @@ config DRM_WERROR
> >         this config option is disabled by default.
> >
> >         If in doubt, say N.
> > +
> > +config DRM_HEADER_TEST
> > +     bool "Ensure DRM headers are self-contained and pass kernel-doc"
> > +     depends on EXPERT
> > +     default n
> > +     help
> > +       Ensure the DRM subsystem headers both under drivers/gpu/drm and
> > +       include/drm compile, are self-contained, have header guards, and have
> > +       no kernel-doc warnings.
> > +
> > +       If in doubt, say N.
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index a73c04d2d7a3..6605d5686d01 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -218,3 +218,21 @@ obj-y                    += solomon/
> >  obj-$(CONFIG_DRM_SPRD) += sprd/
> >  obj-$(CONFIG_DRM_LOONGSON) += loongson/
> >  obj-$(CONFIG_DRM_POWERVR) += imagination/
> > +
> > +# Ensure drm headers are self-contained and pass kernel-doc
> > +hdrtest-files := \
> > +     $(shell cd $(srctree)/$(src) && find . -maxdepth 1 -name 'drm_*.h') \
> > +     $(shell cd $(srctree)/$(src) && find display lib -name '*.h')
> > +
> > +always-$(CONFIG_DRM_HEADER_TEST) += \
> > +     $(patsubst %.h,%.hdrtest, $(hdrtest-files))
> > +
> > +# Include the header twice to detect missing include guard.
> > +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
> > +      cmd_hdrtest = \
> > +             $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
> > +             $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_DRM_WERROR),-Werror) $<; \
> > +             touch $@
> > +
> > +$(obj)/%.hdrtest: $(src)/%.h FORCE
> > +     $(call if_changed_dep,hdrtest)
> > diff --git a/include/Kbuild b/include/Kbuild
> > new file mode 100644
> > index 000000000000..5e76a599e2dd
> > --- /dev/null
> > +++ b/include/Kbuild
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_DRM_HEADER_TEST)        += drm/
> > diff --git a/include/drm/Makefile b/include/drm/Makefile
> > new file mode 100644
> > index 000000000000..b9f391d7aadd
> > --- /dev/null
> > +++ b/include/drm/Makefile
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# Ensure drm headers are self-contained and pass kernel-doc
> > +hdrtest-files := \
> > +     $(shell cd $(srctree)/$(src) && find * -name '*.h' 2>/dev/null)
> > +
> > +always-$(CONFIG_DRM_HEADER_TEST) += \
> > +     $(patsubst %.h,%.hdrtest, $(hdrtest-files))
> > +
> > +# Include the header twice to detect missing include guard.
> > +quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
> > +      cmd_hdrtest = \
> > +             $(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
> > +             $(srctree)/scripts/kernel-doc -none $(if $(CONFIG_DRM_WERROR),-Werror) $<; \
> > +             touch $@
> > +
> > +$(obj)/%.hdrtest: $(src)/%.h FORCE
> > +     $(call if_changed_dep,hdrtest)
>
> --
> Jani Nikula, Intel



--
Best Regards
Masahiro Yamada