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

Mon Mar 3 12:59:33 UTC 2025

On Mon, Mar 3, 2025 at 7:02 PM Jani Nikula <jani.nikula at intel.com> wrote:
>
> On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy at kernel.org> wrote:
> > +CC: Linus
> >
> > On Wed, Jan 22, 2025 at 11:41 PM 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.
> >
> > NACK.
> >
> > I replied here:
> > https://lore.kernel.org/all/CAK7LNARJgqADxnOXAX49XzDFD4zT=7i8yTB0o=EmNtxmScq8jA@mail.gmail.com/T/#u
>
> I really don't find it fair to completely ignore several pings over an
> extended period of time, and then show up to NAK after the patches have
> been merged.

Sorry, I didn't mean to ignore it - I simply didn't notice it.

I regularly check linux-kbuild and linux-kernel MLs (though I still miss
responding to many emails).
However, I don't check the drm ML at all.
I need to reconsider my email filtering rules, but in reality,
I can't respond to all emails in time.

I believe you are re-adding something Linus was negative about:
https://lore.kernel.org/all/87a7982hwc.fsf@intel.com/

> > I CCed Linus to avoid him accidentally pulling this.
> > He disliked this misfeature.
>
> I believe being able to statically check the headers at build time, both
> by the developers and CI, depending on a config option, makes for a more
> pleasant development experience.
>
> We've had this in i915 and xe for a long time, and we avoid a lot of
> build breakage due to missing includes e.g. while refactoring, and we
> don't get reports about kernel-doc issues either. Because they all fail
> at build, and we catch the issues pre-merge. We skip a whole class of
> merge->dammit->fix cycles with this.
>
> All of the drm headers are clean and pass. We don't add any exception
> lists. It's not enabled by default.

I'm not a big fan of the header tests in i915 and xe.
However, you've built a fence and you are dong what you want
in driver-local Makefiles, so I can't avoid them.

>
> I can appreciate this might not be the best approach for all of
> include/linux, but for include/drm, I think it's definitely a win.
>
> And one of the underlying goals is to make for minimal headers with
> minimal includes and minimal dependencies, preferring forward
> declarations over includes, splitting functionality by header, etc. It's
> just that doing that often leads to broken headers, unless you actually
> build test them... and here we are.

What I learned from my last attempt is that we cannot avoid
false positives without adding a lot of exceptions.

We can never be certain whether you are making DRM headers
self-contained for valid reasons or for hypothetical, invalid ones.

>
> BR,
> Jani.
>
>
> >
> >
> >
> >
> >>
> >> v4: check for CONFIG_WERROR in addition to CONFIG_DRM_WERROR
> >>
> >> v3: adapt to upstream build changes
> >>
> >> v2: make DRM_HEADER_TEST depend on DRM
> >>
> >> 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>
> >> ---
> >>  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 fbef3f471bd0..f9b3ebf63fa9 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -494,6 +494,17 @@ config DRM_WERROR
> >>
> >>           If in doubt, say N.
> >>
> >> +config DRM_HEADER_TEST
> >> +       bool "Ensure DRM headers are self-contained and pass kernel-doc"
> >> +       depends on DRM && 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.
> >> +
> >>  endif
> >>
> >>  # Separate option because drm_panel_orientation_quirks.c is shared with fbdev
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 85af94bb907d..42901f877bf2 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -222,3 +222,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 $(src) && find . -maxdepth 1 -name 'drm_*.h') \
> >> +       $(shell cd $(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_WERROR)$(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..a7bd15d2803e
> >> --- /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 $(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_WERROR)$(CONFIG_DRM_WERROR),-Werror) $<; \
> >> +               touch $@
> >> +
> >> +$(obj)/%.hdrtest: $(src)/%.h FORCE
> >> +       $(call if_changed_dep,hdrtest)
> >> --
> >> 2.39.5
> >>
>
> --
> Jani Nikula, Intel

-- 
Best Regards
Masahiro Yamada