On Mon, Mar 3, 2025 at 7:02 PM Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > > 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. 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@xxxxxxxxx/ > > 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@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 -- Best Regards Masahiro Yamada
