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(-)
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
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
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
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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.