[PATCH v2 0/4] kbuild: resurrect generic header check facility

[PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jani Nikula]

Another go at hiding the turds.

In v1 [1] I hid the build artifacts under .hdrtest subdirectories, one in each
$(obj) directory, but the feedback from Linus [2] was to have one top level
directory for this.

This is not possible without turning the whole thing back into a generic header
check facility. Personally, I think this is a good thing. Just look at patches
2-4, it's great.

The main reason we've been doing this in the subsystem/driver level at all is
the opposition from the kbuild maintainer. We'd very much like for Masahiro to
support us in our efforts, but without that support, we're limited to hacking in
the subsystem/driver Makefiles.

BR,
Jani.


[1] https://lore.kernel.org/r/20250401121830.21696-1-jani.nikula@intel.com

[2] https://lore.kernel.org/r/CAHk-=wiP0ea7xq2P3ryYs6xGWoqTw1E4jha67ZbJkaFrjqUdkQ@mail.gmail.com


Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: linux-kbuild@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org


Jani Nikula (4):
  kbuild: add generic header check facility
  drm: switch to generic header check facility
  drm/i915: switch to generic header check facility
  drm/xe: switch to generic header check facility

 drivers/gpu/drm/Kconfig           |  2 +-
 drivers/gpu/drm/Makefile          | 15 +--------------
 drivers/gpu/drm/i915/Makefile     | 14 ++------------
 drivers/gpu/drm/xe/Makefile       | 10 ++--------
 drivers/gpu/drm/xe/xe_pcode_api.h |  4 ++++
 include/drm/Makefile              | 15 +--------------
 init/Kconfig                      | 25 +++++++++++++++++++++++++
 scripts/Makefile.build            | 13 +++++++++++++
 scripts/Makefile.lib              |  7 +++++++
 9 files changed, 56 insertions(+), 49 deletions(-)

-- 
2.39.5

[PATCH v2 1/4] kbuild: add generic header check facility

[Jani Nikula]

Resurrect a generic header check facility. Check that the headers are
self-contained, have header guards, and (if enabled separately) pass
kernel-doc. Run header checks on .h files listed in header-check-y or
header-check-m, relative to $(src).

Hide header check artifacts under a .header-check subdirectory at the
top level of the build output directory.

Add the facility behind CONFIG_HEADER_CHECK_DISABLE, reversed to keep
the feature disabled for allmodconfig and allyesconfig builds. Also add
a proxy CONFIG_HEADER_CHECK option to simplify dependencies on the
facility. The kernel-doc check requires CONFIG_HEADER_CHECK_KERNEL_DOC.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: linux-kbuild@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
---
 init/Kconfig           | 25 +++++++++++++++++++++++++
 scripts/Makefile.build | 13 +++++++++++++
 scripts/Makefile.lib   |  7 +++++++
 3 files changed, 45 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 681f38ee68db..2678a5ba7b93 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -217,6 +217,31 @@ config UAPI_HEADER_TEST
 	  If you are a developer or tester and want to ensure the exported
 	  headers are self-contained, say Y here. Otherwise, choose N.
 
+# Reversed option to disable on allyesconfig/allmodconfig builds
+config HEADER_CHECK_DISABLE
+	bool "Disable extra build-time header checks"
+	default y
+	help
+	  Disable extra build-time header checks. The checks may be
+	  overzealous. They may slow down or fail the build altogether. They may
+	  create excessive dependency files in the tree. They should not be
+	  enabled for regular builds, and thus they are disabled by default.
+
+# Proxy config to allow simple "depends on HEADER_CHECK"
+config HEADER_CHECK
+	bool
+	depends on EXPERT && HEADER_CHECK_DISABLE=n
+	default !HEADER_CHECK_DISABLE
+
+config HEADER_CHECK_KERNEL_DOC
+	bool "Run kernel-doc as part of header checks"
+	depends on HEADER_CHECK
+	default n
+	help
+	  Run kernel-doc as part of header checks. In addition to compiling,
+	  also check kernel-doc comments. With CONFIG_WERROR=y, kernel-doc
+	  warnings are treated as errors.
+
 config LOCALVERSION
 	string "Local version - append to kernel release"
 	help
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 56be83024851..f963b2356b0e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -223,6 +223,19 @@ quiet_cmd_cc_lst_c = MKLST   $@
 $(obj)/%.lst: $(obj)/%.c FORCE
 	$(call if_changed_dep,cc_lst_c)
 
+# Compile C headers (.h) for header check
+# ---------------------------------------------------------------------------
+
+# Include the header twice to detect missing include guard.
+quiet_cmd_header_check = HDRCHK  $(patsubst $(srctree)/%,%,$<)
+      cmd_header_check = \
+		$(CC) $(c_flags) -fsyntax-only -x c /dev/null -include $< -include $<; \
+		$(if $(CONFIG_HEADER_CHECK_KERNEL_DOC),$(srctree)/scripts/kernel-doc -none $(if $(CONFIG_WERROR),-Werror) $<,true); \
+		touch $@
+
+.header-check/$(obj)/%.header-check: $(src)/%.h FORCE
+	$(call if_changed_dep,header_check)
+
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 57620b439a1f..272a1b42292e 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -89,6 +89,13 @@ multi-obj-m	:= $(addprefix $(obj)/, $(multi-obj-m))
 subdir-ym	:= $(addprefix $(obj)/,$(subdir-ym))
 endif
 
+# Header checks
+# header-check-y/m contain .h files to be checked, relative to $(src)
+
+header-check-y	:= $(addprefix .header-check/$(obj)/,$(patsubst %.h,%.header-check,$(header-check-y) $(header-check-m)))
+
+always-$(CONFIG_HEADER_CHECK) += $(header-check-y)
+
 # Finds the multi-part object the current object will be linked into.
 # If the object belongs to two or more multi-part objects, list them all.
 modname-multi = $(sort $(foreach m,$(multi-obj-ym),\
-- 
2.39.5

[PATCH v2 2/4] drm: switch to generic header check facility

[Jani Nikula]

Switch to the generic header check facility, and sunset the copy-pasted
local version.

Keep CONFIG_DRM_HEADER_TEST around for fine-grained control of what gets
checked. To be unified later.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Closes: https://lore.kernel.org/r/CAHk-=wjMrqzuUmH-mFbR_46EWEFS=bB=J7h9ABMVy56Vi81PKQ@mail.gmail.com
Fixes: 62ae45687e43 ("drm: ensure drm headers are self-contained and pass kernel-doc")
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: linux-kbuild@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/Kconfig  |  2 +-
 drivers/gpu/drm/Makefile | 15 +--------------
 include/drm/Makefile     | 15 +--------------
 3 files changed, 3 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 2cba2b6ebe1c..10189d0ec30d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -505,7 +505,7 @@ config DRM_WERROR
 
 config DRM_HEADER_TEST
 	bool "Ensure DRM headers are self-contained and pass kernel-doc"
-	depends on DRM && EXPERT && BROKEN
+	depends on DRM && HEADER_CHECK
 	default n
 	help
 	  Ensure the DRM subsystem headers both under drivers/gpu/drm and
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index ed54a546bbe2..fb2642d46f29 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -225,19 +225,6 @@ obj-$(CONFIG_DRM_LOONGSON) += loongson/
 obj-$(CONFIG_DRM_POWERVR) += imagination/
 
 # Ensure drm headers are self-contained and pass kernel-doc
-hdrtest-files := \
+header-check-$(CONFIG_DRM_HEADER_TEST) += \
 	$(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/drm/Makefile b/include/drm/Makefile
index a7bd15d2803e..ed0567a7956a 100644
--- a/include/drm/Makefile
+++ b/include/drm/Makefile
@@ -1,18 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 
 # Ensure drm headers are self-contained and pass kernel-doc
-hdrtest-files := \
+header-check-$(CONFIG_DRM_HEADER_TEST) += \
 	$(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

[PATCH v2 3/4] drm/i915: switch to generic header check facility

[Jani Nikula]

Switch to the generic header check facility, and sunset the copy-pasted
local version.

Keep the header checks gated on CONFIG_DRM_I915_WERROR as before. To be
unified later.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Closes: https://lore.kernel.org/r/CAHk-=wjMrqzuUmH-mFbR_46EWEFS=bB=J7h9ABMVy56Vi81PKQ@mail.gmail.com
Fixes: c6d4a099a240 ("drm/i915: reimplement header test feature")
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: linux-kbuild@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/i915/Makefile | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ed05b131ed3a..dca187e58bda 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -411,19 +411,9 @@ ifdef CONFIG_DRM_I915_WERROR
     cmd_checkdoc = $(srctree)/scripts/kernel-doc -none -Werror $<
 endif
 
-# header test
-
 # exclude some broken headers from the test coverage
 no-header-test := \
 	display/intel_vbt_defs.h
 
-always-$(CONFIG_DRM_I915_WERROR) += \
-	$(patsubst %.h,%.hdrtest, $(filter-out $(no-header-test), \
-		$(shell cd $(src) && find * -name '*.h')))
-
-quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
-      cmd_hdrtest = $(CC) $(filter-out $(CFLAGS_GCOV), $(c_flags)) -S -o /dev/null -x c /dev/null -include $<; \
-		$(srctree)/scripts/kernel-doc -none -Werror $<; touch $@
-
-$(obj)/%.hdrtest: $(src)/%.h FORCE
-	$(call if_changed_dep,hdrtest)
+header-check-$(CONFIG_DRM_I915_WERROR) += \
+	$(filter-out $(no-header-test),$(shell cd $(src) && find * -name '*.h'))
-- 
2.39.5

[PATCH v2 4/4] drm/xe: switch to generic header check facility

[Jani Nikula]

Switch to the generic header check facility, and sunset the copy-pasted
local version.

Keep the header checks gated on CONFIG_DRM_XE_WERROR as before. To be
unified later.

While at it, fix a header missing header guards that was not caught by
the local version.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Closes: https://lore.kernel.org/r/CAHk-=wjMrqzuUmH-mFbR_46EWEFS=bB=J7h9ABMVy56Vi81PKQ@mail.gmail.com
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Cc: linux-kbuild@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/xe/Makefile       | 10 ++--------
 drivers/gpu/drm/xe/xe_pcode_api.h |  4 ++++
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 9699b08585f7..2a1854024c84 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -326,14 +326,8 @@ ifneq ($(CONFIG_DRM_XE_DISPLAY),y)
 	hdrtest_find_args += -not -path display/\* -not -path compat-i915-headers/\* -not -path xe_display.h
 endif
 
-always-$(CONFIG_DRM_XE_WERROR) += \
-	$(patsubst %.h,%.hdrtest, $(shell cd $(src) && find * -name '*.h' $(hdrtest_find_args)))
-
-quiet_cmd_hdrtest = HDRTEST $(patsubst %.hdrtest,%.h,$@)
-      cmd_hdrtest = $(CC) -DHDRTEST $(filter-out $(CFLAGS_GCOV), $(c_flags)) -S -o /dev/null -x c /dev/null -include $<; touch $@
-
-$(obj)/%.hdrtest: $(src)/%.h FORCE
-	$(call if_changed_dep,hdrtest)
+header-check-$(CONFIG_DRM_XE_WERROR) += \
+	$(shell cd $(src) && find * -name '*.h' $(hdrtest_find_args))
 
 uses_generated_oob := $(addprefix $(obj)/, $(xe-y))
 $(uses_generated_oob): $(obj)/generated/xe_wa_oob.h
diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
index 2bae9afdbd35..4fd58b3c0d9a 100644
--- a/drivers/gpu/drm/xe/xe_pcode_api.h
+++ b/drivers/gpu/drm/xe/xe_pcode_api.h
@@ -3,6 +3,9 @@
  * Copyright © 2022 Intel Corporation
  */
 
+#ifndef _XE_PCODE_API_H_
+#define _XE_PCODE_API_H_
+
 /* Internal to xe_pcode */
 
 #include "regs/xe_reg_defs.h"
@@ -68,3 +71,4 @@ struct pcode_err_decode {
 	const char *str;
 };
 
+#endif
-- 
2.39.5

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Linus Torvalds]

On Wed, 2 Apr 2025 at 05:47, Jani Nikula <jani.nikula@intel.com> wrote:
>
> Another go at hiding the turds.

The patches look sane to me.

I didn't _test_ them - because that's not how I roll - but they look
like they would do the right thing and not interfere with my odd TAB
lifestyle (I say "odd", because apparently I'm the only one who lives
and dies by auto-complete, but obviously my way is never really odd.
By definition).

         Linus

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Masahiro Yamada]

On Wed, Apr 2, 2025 at 9:47 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> Another go at hiding the turds.
>
> In v1 [1] I hid the build artifacts under .hdrtest subdirectories, one in each
> $(obj) directory, but the feedback from Linus [2] was to have one top level
> directory for this.
>
> This is not possible without turning the whole thing back into a generic header
> check facility. Personally, I think this is a good thing. Just look at patches
> 2-4, it's great.
>
> The main reason we've been doing this in the subsystem/driver level at all is
> the opposition from the kbuild maintainer. We'd very much like for Masahiro to
> support us in our efforts, but without that support, we're limited to hacking in
> the subsystem/driver Makefiles.
>
> BR,
> Jani.
>
>
> [1] https://lore.kernel.org/r/20250401121830.21696-1-jani.nikula@intel.com
>
> [2] https://lore.kernel.org/r/CAHk-=wiP0ea7xq2P3ryYs6xGWoqTw1E4jha67ZbJkaFrjqUdkQ@mail.gmail.com
>
>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Masahiro Yamada <masahiroy@kernel.org>


NACK.

This does not solve any real issue, except making Linus happy
- Sure, he is happy as long as he no longer has to see the turds.

This patch merely hides the turds by moving all the
header-test build artifacts under the .header-check/
and introducing CONFIG_HEADER_CHECK_DISABLE.
Yes, Linus advised us to hide all the turds because he cares
about the TAB-completion.

But to me, from the Kbuild perspective, this is not a solution at all.
What is worse, Jani is pushing his workaround into the common
Kbuild Makefiles, which I maintain, and he is even make this
broken feature widely accessible.

I agree with Jason.
His idea sounds better, although I do not have enough time
for investigating it further or implementing it now.


At least, this patchset is not something we should rush into.



> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: linux-kbuild@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
>
>
> Jani Nikula (4):
>   kbuild: add generic header check facility
>   drm: switch to generic header check facility
>   drm/i915: switch to generic header check facility
>   drm/xe: switch to generic header check facility
>
>  drivers/gpu/drm/Kconfig           |  2 +-
>  drivers/gpu/drm/Makefile          | 15 +--------------
>  drivers/gpu/drm/i915/Makefile     | 14 ++------------
>  drivers/gpu/drm/xe/Makefile       | 10 ++--------
>  drivers/gpu/drm/xe/xe_pcode_api.h |  4 ++++
>  include/drm/Makefile              | 15 +--------------
>  init/Kconfig                      | 25 +++++++++++++++++++++++++
>  scripts/Makefile.build            | 13 +++++++++++++
>  scripts/Makefile.lib              |  7 +++++++
>  9 files changed, 56 insertions(+), 49 deletions(-)
>
> --
> 2.39.5
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jani Nikula]

On Fri, 04 Apr 2025, Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Wed, Apr 2, 2025 at 9:47 PM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> Another go at hiding the turds.
>>
>> In v1 [1] I hid the build artifacts under .hdrtest subdirectories, one in each
>> $(obj) directory, but the feedback from Linus [2] was to have one top level
>> directory for this.
>>
>> This is not possible without turning the whole thing back into a generic header
>> check facility. Personally, I think this is a good thing. Just look at patches
>> 2-4, it's great.
>>
>> The main reason we've been doing this in the subsystem/driver level at all is
>> the opposition from the kbuild maintainer. We'd very much like for Masahiro to
>> support us in our efforts, but without that support, we're limited to hacking in
>> the subsystem/driver Makefiles.
>>
>> BR,
>> Jani.
>>
>>
>> [1] https://lore.kernel.org/r/20250401121830.21696-1-jani.nikula@intel.com
>>
>> [2] https://lore.kernel.org/r/CAHk-=wiP0ea7xq2P3ryYs6xGWoqTw1E4jha67ZbJkaFrjqUdkQ@mail.gmail.com
>>
>>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>
>
> NACK.
>
> This does not solve any real issue, except making Linus happy
> - Sure, he is happy as long as he no longer has to see the turds.
>
> This patch merely hides the turds by moving all the
> header-test build artifacts under the .header-check/
> and introducing CONFIG_HEADER_CHECK_DISABLE.
> Yes, Linus advised us to hide all the turds because he cares
> about the TAB-completion.
>
> But to me, from the Kbuild perspective, this is not a solution at all.
> What is worse, Jani is pushing his workaround into the common
> Kbuild Makefiles, which I maintain, and he is even make this
> broken feature widely accessible.
>
> I agree with Jason.
> His idea sounds better, although I do not have enough time
> for investigating it further or implementing it now.
>
>
> At least, this patchset is not something we should rush into.

I expect much better rationale for NAKs than this.

The shortcoming in this series is that it offloads the decision *which*
header files to check to the subsystems and drivers that actually opt-in
to having the header files to be checked. Because you have to opt-in
anyway, because not everyone wants this.

This makes concrete forward progress, and enables subsystems (like drm)
and drivers (like i915 and xe) have their headers checked the way they
want. It converts the local hacks into a generic solution. And does not
block future improvements.

Even with Jason's idea [1], you *still* have to start small and opt-in
(i.e. the patch series at hand). You can't just start off by testing
every header in one go, because it's a flag day switch. There'll be so
many warnings that it's useless. This series only spotted one omission
of header guards, because we've gradually cleaned stuff up. Oh, and
there's the small detail that the idea is not backed up by any code or
testing.

I fully expect sharp and concrete technical review, but handwavy "does
not solve any real issue", "workaround", and "broken feature" comments
don't help anyone.

With this type of antagonistic rather than encouraging attitude towards
contributions, there's just no way I can justify to myself (or my
employer) spending more time on what looks like a wild goose chase. I
have zero confidence that no matter what I do I'd get approval from you.

And this is the primary reason subsystems and drivers hack up stuff in
their little corners of the kernel instead of sticking their necks out
and trying to generalize anything.

BR,
Jani.


[1] https://lore.kernel.org/r/20250401191455.GC325917@nvidia.com


>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Simona Vetter <simona.vetter@ffwll.ch>
>> Cc: linux-kbuild@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: intel-xe@lists.freedesktop.org
>> Cc: intel-gfx@lists.freedesktop.org
>>
>>
>> Jani Nikula (4):
>>   kbuild: add generic header check facility
>>   drm: switch to generic header check facility
>>   drm/i915: switch to generic header check facility
>>   drm/xe: switch to generic header check facility
>>
>>  drivers/gpu/drm/Kconfig           |  2 +-
>>  drivers/gpu/drm/Makefile          | 15 +--------------
>>  drivers/gpu/drm/i915/Makefile     | 14 ++------------
>>  drivers/gpu/drm/xe/Makefile       | 10 ++--------
>>  drivers/gpu/drm/xe/xe_pcode_api.h |  4 ++++
>>  include/drm/Makefile              | 15 +--------------
>>  init/Kconfig                      | 25 +++++++++++++++++++++++++
>>  scripts/Makefile.build            | 13 +++++++++++++
>>  scripts/Makefile.lib              |  7 +++++++
>>  9 files changed, 56 insertions(+), 49 deletions(-)
>>
>> --
>> 2.39.5
>>

-- 
Jani Nikula, Intel

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jason Gunthorpe]

On Mon, Apr 07, 2025 at 10:17:40AM +0300, Jani Nikula wrote:

> Even with Jason's idea [1], you *still* have to start small and opt-in
> (i.e. the patch series at hand). You can't just start off by testing
> every header in one go, because it's a flag day switch. 

You'd add something like 'make header_check' that does not run
automatically. Making it run automatically after everything is fixed
to keep it fixed would be the flag day change. It is how we have
managed to introduce other warning levels in the past.

If you added the infrastructure there is a whole list of people on
kernel-janitors that would probably help with the trivial cleanups to
make it run clean.

> With this type of antagonistic rather than encouraging attitude towards
> contributions, there's just no way I can justify to myself (or my
> employer) spending more time on what looks like a wild goose chase. I
> have zero confidence that no matter what I do I'd get approval from you.

I think you've been given a clear direction on what would be accepted
and have the option to persue it. Claiming that is "antagonistic"
seems unnecessary.

> And this is the primary reason subsystems and drivers hack up stuff in
> their little corners of the kernel instead of sticking their necks out
> and trying to generalize anything.

Seems to me like this is the usual case of generalizing being actually
hard, you almost always have to actually do more work to succeed.

Jason

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jani Nikula]

On Mon, 07 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
> On Mon, Apr 07, 2025 at 10:17:40AM +0300, Jani Nikula wrote:
>
>> Even with Jason's idea [1], you *still* have to start small and opt-in
>> (i.e. the patch series at hand). You can't just start off by testing
>> every header in one go, because it's a flag day switch. 
>
> You'd add something like 'make header_check' that does not run
> automatically. Making it run automatically after everything is fixed
> to keep it fixed would be the flag day change. It is how we have
> managed to introduce other warning levels in the past.

That approach does not help *me* or drm, i915 and xe in the least. They
are already fixed, and we want a way to keep them fixed. This is how all
of this got started.

Your goal may be to make everything self-contained, but AFAICS there is
no agreement on that goal. As long as there's no buy-in to this, it's
not possible fix everything, it's an unreachable goal.

Arguably the situation is similar to W=1 builds. We can't run W=1 in our
CI, because of failures outside of the drivers we maintain. Thus we add
most W=1 checks manually in our Makefiles, and keep our drivers warning
free. This is what we want for headers too. But the key difference is
that there *is* general consensus that W=1 fixes should be merged.

Perhaps inadvertently, you're making it harder for the people who are on
board with your goal to do their part of the job in their corner of the
kernel.

> If you added the infrastructure there is a whole list of people on
> kernel-janitors that would probably help with the trivial cleanups to
> make it run clean.
>
>> With this type of antagonistic rather than encouraging attitude towards
>> contributions, there's just no way I can justify to myself (or my
>> employer) spending more time on what looks like a wild goose chase. I
>> have zero confidence that no matter what I do I'd get approval from you.
>
> I think you've been given a clear direction on what would be accepted
> and have the option to persue it. Claiming that is "antagonistic"
> seems unnecessary.

I have to disagree on both of those points.

>> And this is the primary reason subsystems and drivers hack up stuff in
>> their little corners of the kernel instead of sticking their necks out
>> and trying to generalize anything.
>
> Seems to me like this is the usual case of generalizing being actually
> hard, you almost always have to actually do more work to succeed.

I think you and me both share the idea that most headers should be
self-contained. Neither Linus nor Masahiro seem to think that is
generally true. I've provided a way to opt-in for the checks. Linus
seems to find the approach acceptable. You and Masahiro don't.

I think I'm at an impasse.

Even if I put in the effort to generalize this the way you prefer, I
guess a few kernel releases from now, it still would not do what we have
already in place in i915 and xe. And, no offense, but I think your
proposal is technically vague to start with. I really don't know where
the goal posts are.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jason Gunthorpe]

On Tue, Apr 08, 2025 at 11:27:58AM +0300, Jani Nikula wrote:
> On Mon, 07 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > On Mon, Apr 07, 2025 at 10:17:40AM +0300, Jani Nikula wrote:
> >
> >> Even with Jason's idea [1], you *still* have to start small and opt-in
> >> (i.e. the patch series at hand). You can't just start off by testing
> >> every header in one go, because it's a flag day switch. 
> >
> > You'd add something like 'make header_check' that does not run
> > automatically. Making it run automatically after everything is fixed
> > to keep it fixed would be the flag day change. It is how we have
> > managed to introduce other warning levels in the past.
> 
> That approach does not help *me* or drm, i915 and xe in the least. They
> are already fixed, and we want a way to keep them fixed. This is how all
> of this got started.

I imagine you'd include a way to have the 'make header_check' run on
some subset of files only, then use that in your CI for the interm.

> Your goal may be to make everything self-contained, but AFAICS there is
> no agreement on that goal. As long as there's no buy-in to this, it's
> not possible fix everything, it's an unreachable goal.

I didn't see that. I saw technical problems with the implementation
that was presented. I'd be shocked if there was broad opposition to
adding missing includes and forward declaration to most headers. It is
a pretty basic C thing. :\ 

Until someone sends a series trying to add missing includes and
forward declarations we can't really know..

> Arguably the situation is similar to W=1 builds. We can't run W=1 in our
> CI, because of failures outside of the drivers we maintain. 

You can run W=1 using a subdirectory build just for your drivers.

> Even if I put in the effort to generalize this the way you prefer, I
> guess a few kernel releases from now, it still would not do what we have
> already in place in i915 and xe. And, no offense, but I think your
> proposal is technically vague to start with. I really don't know where
> the goal posts are.

Well, I spent a little bit and wrote a mock up and did some looking at
how much work is here. Focusing on allnoconfig as a starting point,
293 out of 1858 headers failed to build, and with some fiddling I got
it down to 150, a couple of hours would get patches made for the vast
majority of it.

https://github.com/jgunthorpe/linux/commits/hdrcheck/

I don't see the same dire view as you do, it seems reasonable and doable.

Jason

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jani Nikula]

On Tue, 08 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
> On Tue, Apr 08, 2025 at 11:27:58AM +0300, Jani Nikula wrote:
>> On Mon, 07 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
>> > On Mon, Apr 07, 2025 at 10:17:40AM +0300, Jani Nikula wrote:
>> >
>> >> Even with Jason's idea [1], you *still* have to start small and opt-in
>> >> (i.e. the patch series at hand). You can't just start off by testing
>> >> every header in one go, because it's a flag day switch. 
>> >
>> > You'd add something like 'make header_check' that does not run
>> > automatically. Making it run automatically after everything is fixed
>> > to keep it fixed would be the flag day change. It is how we have
>> > managed to introduce other warning levels in the past.
>> 
>> That approach does not help *me* or drm, i915 and xe in the least. They
>> are already fixed, and we want a way to keep them fixed. This is how all
>> of this got started.
>
> I imagine you'd include a way to have the 'make header_check' run on
> some subset of files only, then use that in your CI for the interm.
>
>> Your goal may be to make everything self-contained, but AFAICS there is
>> no agreement on that goal. As long as there's no buy-in to this, it's
>> not possible fix everything, it's an unreachable goal.
>
> I didn't see that. I saw technical problems with the implementation
> that was presented. I'd be shocked if there was broad opposition to
> adding missing includes and forward declaration to most headers. It is
> a pretty basic C thing. :\

Unless I'm mistaken, both Linus and Masahiro have said they disagree
with headers having to be self-contained as a general rule, regardless
of the issues with kconfig and the build artifacts.

We actually had header checks back in 2019 but it was reverted basically
without discussion with commit fcbb8461fd23 ("kbuild: remove header
compile test"). Sure, there were issues, but still removed without an
attempt to address the issues. Since then it's been skunkworks in
i915. There's a reason this has felt like an uphill battle, and why I'm
reluctant to putting effort into much more than small incremental steps
at a time.

> Until someone sends a series trying to add missing includes and
> forward declarations we can't really know..
>
>> Arguably the situation is similar to W=1 builds. We can't run W=1 in our
>> CI, because of failures outside of the drivers we maintain. 
>
> You can run W=1 using a subdirectory build just for your drivers.

I don't think there's a way to build the entire kernel while limiting
W=1 warnings to a subdirectory, is there? Mixing W=1 and regular builds
causes everything to be rebuilt due to dependencies. It's not only for
CI, it's also for developers.

>> Even if I put in the effort to generalize this the way you prefer, I
>> guess a few kernel releases from now, it still would not do what we have
>> already in place in i915 and xe. And, no offense, but I think your
>> proposal is technically vague to start with. I really don't know where
>> the goal posts are.
>
> Well, I spent a little bit and wrote a mock up and did some looking at
> how much work is here. Focusing on allnoconfig as a starting point,
> 293 out of 1858 headers failed to build, and with some fiddling I got
> it down to 150, a couple of hours would get patches made for the vast
> majority of it.
>
> https://github.com/jgunthorpe/linux/commits/hdrcheck/
>
> I don't see the same dire view as you do, it seems reasonable and doable.

Thanks for the proof-of-concept. It's just that I don't see how that
could be bolted to kbuild, with dependency tracking. I don't want to
have to rebuild the world every time something changes.

Say, I'm refactoring stuff, and I want to ensure headers are okay every
step of the way. git rebase -i origin -x 'make header_check'. How do you
only check the headers whose dependencies were changed since the
previous commit? That requires looking at the .cmd again.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Jason Gunthorpe]

On Tue, Apr 08, 2025 at 09:42:36PM +0300, Jani Nikula wrote:
> On Tue, 08 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > On Tue, Apr 08, 2025 at 11:27:58AM +0300, Jani Nikula wrote:
> >> On Mon, 07 Apr 2025, Jason Gunthorpe <jgg@nvidia.com> wrote:
> >> > On Mon, Apr 07, 2025 at 10:17:40AM +0300, Jani Nikula wrote:
> >> >
> >> >> Even with Jason's idea [1], you *still* have to start small and opt-in
> >> >> (i.e. the patch series at hand). You can't just start off by testing
> >> >> every header in one go, because it's a flag day switch. 
> >> >
> >> > You'd add something like 'make header_check' that does not run
> >> > automatically. Making it run automatically after everything is fixed
> >> > to keep it fixed would be the flag day change. It is how we have
> >> > managed to introduce other warning levels in the past.
> >> 
> >> That approach does not help *me* or drm, i915 and xe in the least. They
> >> are already fixed, and we want a way to keep them fixed. This is how all
> >> of this got started.
> >
> > I imagine you'd include a way to have the 'make header_check' run on
> > some subset of files only, then use that in your CI for the interm.
> >
> >> Your goal may be to make everything self-contained, but AFAICS there is
> >> no agreement on that goal. As long as there's no buy-in to this, it's
> >> not possible fix everything, it's an unreachable goal.
> >
> > I didn't see that. I saw technical problems with the implementation
> > that was presented. I'd be shocked if there was broad opposition to
> > adding missing includes and forward declaration to most headers. It is
> > a pretty basic C thing. :\
> 
> Unless I'm mistaken, both Linus and Masahiro have said they disagree
> with headers having to be self-contained as a general rule, regardless
> of the issues with kconfig and the build artifacts.

Right, no general rule.

But the data I just ran shows the vast majority are already self
contained (~15% are not) and many are trivially fixable to be self
contained. There is a fairly small minority that will not and should
not be self contained.

So I expect there is alot of headers where people would agree to add
the missing #include <linux/types.h> for example, which I found
about 20 of in about 10 mins.

And a smallish exclusion list to ignore the special cases. Ie I
started by just regex ignoring all of asm because there was lots of
interesting stuff in there.

The point is we can probably get to a full kernel check, with a
minority of special headers excluded, that does not have any errors.

As I said in my first email I think this brings real actual value to
people using clangd. AFAICT there is no good reason that every day
normal headers should be missing their #include <linux/types.h> (which
seems to be the most common error)

This is where I think it is constructive to present what the actual
proposed header files changes would be.

> > You can run W=1 using a subdirectory build just for your drivers.
> 
> I don't think there's a way to build the entire kernel while limiting
> W=1 warnings to a subdirectory, is there? Mixing W=1 and regular builds
> causes everything to be rebuilt due to dependencies. It's not only for
> CI, it's also for developers.

You'd have to do the W=0 build then a subdirectory W=1 build.

I agree this is annoying and I do wish kbuild had a better solution
here.

> Thanks for the proof-of-concept. It's just that I don't see how that
> could be bolted to kbuild, with dependency tracking. I don't want to
> have to rebuild the world every time something changes.

I used ninja to run this because it is very easy to get setup and
going and doesn't leave behind the 'turds'. The main point was to show
that the .cmd processing and so on works sensibly and does avoid the
kconfig issues.

If people agree to stick with ninja for this then you'd use the -MD
option to gcc and the depfile=foo.d instruction then you get full
dependency tracking and incremental compilation. Along with a rule to
rebuild the rule file if any .cmd file changes. I did not show this,
but it is very easy.

Jason

Re: [PATCH v2 0/4] kbuild: resurrect generic header check facility

[Linus Torvalds]

On Tue, 8 Apr 2025 at 01:28, Jani Nikula <jani.nikula@intel.com> wrote:
>
> Your goal may be to make everything self-contained, but AFAICS there is
> no agreement on that goal.

Yeah, absolutely not.

I'm not interested in making some general rule that all headers should
be self-contained.

We already have some fairly obvious and clear exceptions to that, in
how some headers are special and get included early on and headers are
*not* supposed to include them themselves (ie things like
compiler-version.h and kconfig.h)

And while those are *really* special and end up being done
automatically by our compiler flags, they are by no means the only
special cases.

Quite a *lot* of our headers have things like

    # error "Please do not include this file directly."

because those headers are literally *designed* to not be
self-sufficient. And that is absolutely not a mistake. These are
headers that are meant to be included in very specific situations by
specific other header files.

So no. The whole "everything is going to be self-contained" is simply
not going to happen. It's not even worth discussing. It's a
no-starter, and limits our header file design much too much.

Honestly, I think the whole "headers have to be self-contained in
general" thing is a mistake. But I think it's fine for people to mark
their "generic" headers for some kind of checking.

I think it's a bad bad idea to make it a rule, though.

              Linus