On Mon, 03 Mar 2025, Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > +CC: Linus > > On Wed, Jan 22, 2025 at 11:41 PM Jani Nikula <jani.nikula@xxxxxxxxx> 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@xxxxxxxxxxxxxx/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. > 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 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. 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@xxxxxxxx> >> Cc: David Airlie <airlied@xxxxxxxxx> >> Cc: Daniel Vetter <daniel@xxxxxxxx> >> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> >> Cc: Maxime Ripard <mripard@xxxxxxxxxx> >> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Cc: Masahiro Yamada <masahiroy@xxxxxxxxxx> >> Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> 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
