Re: [PATCH v3 09/16] platform/x86: x86-android-tablets: Add include defining struct dmi_system_id

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 11854 bytes --]

On Wed, 1 Jul 2026, Uwe Kleine-König (The Capable Hub) wrote:
> On Tue, Jun 30, 2026 at 09:07:08PM +0300, Ilpo Järvinen wrote:
> > On Tue, 30 Jun 2026, Uwe Kleine-König (The Capable Hub) wrote:
> > > On Tue, Jun 30, 2026 at 05:58:54PM +0300, Ilpo Järvinen wrote:
> > > > On Mon, 29 Jun 2026, Uwe Kleine-König (The Capable Hub) wrote:
> > > > > On Mon, Jun 29, 2026 at 02:38:35PM +0300, Ilpo Järvinen wrote:
> > > > > > On Sun, 28 Jun 2026, Uwe Kleine-König (The Capable Hub) wrote:
> > > > > > 
> > > > > > > Currently <linux/i2c.h> includes <linux/mod_devicetable.h> transitively
> > > > > > > which ensures that struct dmi_system_id is defined in
> > > > > > > drivers/platform/x86/x86-android-tablets/x86-android-tablets.h. However
> > > > > > > this include in <linux/i2c.h> will be replaced by one for i2c_device_id
> > > > > > > only. To ensure that dmi_system_id is available add the include for that
> > > > > > > explicitly.
> > > > > > > 
> > > > > > > Acked-by: Danilo Krummrich <dakr@kernel.org>
> > > > > > > Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
> > > > > > > ---
> > > > > > >  drivers/platform/x86/x86-android-tablets/x86-android-tablets.h | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
> > > > > > > index 2498390958ad..c756961ae5fd 100644
> > > > > > > --- a/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
> > > > > > > +++ b/drivers/platform/x86/x86-android-tablets/x86-android-tablets.h
> > > > > > > @@ -13,6 +13,7 @@
> > > > > > >  #include <linux/gpio/consumer.h>
> > > > > > >  #include <linux/i2c.h>
> > > > > > >  #include <linux/irqdomain_defs.h>
> > > > > > > +#include <linux/device-id/dmi.h>
> > > > > > >  #include <linux/spi/spi.h>
> > > > > > >  
> > > > > > >  struct gpio_desc;
> > > > > > 
> > > > > > Indirect include patchs are never a good idea as it always makes header 
> > > > > > reorganizations minefield. So for this change,
> > > > > > 
> > > > > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > > 
> > > > > > 
> > > > > > When it comes to the series, I certainly like the direction it goes very 
> > > > > > much (never have been fan of mod_devicetable.h) but I'd have preferred 
> > > > > > stepwise approach over trying to introduce it to some mid-rc.
> > > > > 
> > > > > The hurdle here is that at least the header part isn't easily
> > > > > automateable.
> > > > 
> > > > Patch 1?
> > > 
> > > No, patch 15.
> > >
> > > > > (Well it is, but the script that I have now to check all
> > > > > .c files already takes longer than an hour to run. I guess it's not as
> > > > > optimal as it could, but still.) And so getting this ready for inclusion
> > > > > in next and keeping it up to date until the merge window opens is a
> > > > > tough job.
> > 
> > It looks to me more a problem with the chosen approach where you wanted 
> > to get it all quickly done in a single all-reaching series (which IMO 
> > would have been fine right after -rc1 but mid-rc cycle, I'm not so 
> > positive about it).
> 
> In my book we're still in the "right after -rc1" window. The patch set
> was mostly ready at -rc1 (it just grew a patch to fix a build failure on
> parisc and I added review tags) and assuming Linus doesn't oppose to my
> timing it should go in before -rc2.
>
> And yes, I admit to want it done, but that's not part of my
> justification to get it in after -rc1.
> 
> > > > > The further downside is that each modification to one of the highly used
> > > > > headers is expensive (in rebuild time) when these are merged one after
> > > > > another (or when bisecting). So getting these changes in in one batch is
> > > > > beneficial.
> > > > > 
> > > > > So while I agree with your preference, I don't think it's feasible.
> > > > 
> > > > While I understand keeping it up-to-date must be a pain, I'm not entirely 
> > > > convinced by the rebuild time argument as it has been the status quo so 
> > > > far too for anything touching mod_devicetable.h.
> > > 
> > > Yes. I don't know about others, but big rebuilds annoy me regularily.
> > > And with another quest that involves touching most struct *_device_id
> > > this really gets a pain. And just because "we" endured something in the
> > > past, doesn't mean we shouldn't improve.
> > > 
> > > > If one commit would move all linux/device-id/* AND ADD them all into 
> > > > mod_devicetable.h as includes, that's just one rebuild more (and one 
> > > > rebuild will occur anyway whichever way you architect this).
> > > 
> > > Yes, one rebuild isn't possible to prevent. You're talking about patch
> > > #1, right?
> > > 
> > > > Only if includes would be then removed one-by-one from mod_devicetable.h 
> > > > (e.g. per subsystem basis), I can see it causing n rebuilds (+conflicts), 
> > > > but they could be removed in bulk as well.
> > > 
> > > Removing those from mod_devicetable.h isn't part of the plan.
> > 
> > It isn't because you're not doing this in steps so you didn't put them in 
> > there at all (which would have been fine at any time and only enforces 
> > one rebuild comparable to any other linux/mod_devicetable.h edit).
> 
> <linux/mod_devicetable.h> only consists of #includes of the new files
> (well, plus struct cpu_feature). That's patch #1 and it enforces a
> rebuild. So either "you didn't put them in there at all" is wrong or I
> misunderstand you.

Yes, I was wrong, sorry about that.

I looked at patch 1's diffstat and thought it cannot be adding those 
headers with only two +, totally disregarding the scale. And they were 
so much down in the in diff I didn't see them there either because I 
didn't look that far.

Most of my worry was related to that misunderstanding because it would 
meant trouble for subsystem maintainer if adding linux/mod_devicetable.h 
would have been problems during merges.

I suppose it doesn't matter much then to me if it's done mid-cycle.

-- 
 i.

> > > But
> > > replacing `#include <linux/mod_devicetable.h>` by `#include
> > > <linux/device-id/of.h>` in <linux/of.h> triggers again many rebuilds
> > > when done separately.
> > 
> > But this is not because you touch linux/mod_devicetable.h but linux/of.h.
> > It certainly should cause rebuild of all the code depending on linux/of.h 
> > but I assume that is much less than what linux/mod_devicetable.h causes.
> 
> It's a subset, yes. In an ARCH=arm64 allmodconfig build of -rc1 + this
> patch series I have:
> 
> 	# number of object files:
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | wc -l
> 	28590
> 
> 	# these are the old users of <linux/mod_devicetable.h>:
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | xargs grep -l /device-id/ | wc -l
> 	23583
> 
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | xargs grep -l linux/of.h | wc -l
> 	12302
> 
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | xargs grep -l linux/platform_device.h | wc -l
> 	9255
> 
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | xargs grep -l linux/pci.h | wc -l
> 	7747
> 
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | xargs grep -l linux/acpi.h | wc -l
> 	7233
> 
> 	$ find -name \*.o.cmd -not -name \*mod.o.cmd | xargs grep -l linux/i2c.h | wc -l
> 	5373
> 
> so it's a considerable subset. For x86_64 the numbers are similar:
> 
> 	29655
> 	/device-id/: 24800
> 	linux/of.h: 12602
> 	linux/platform_device.h: 9344
> 	linux/pci.h: 8605
> 	linux/acpi.h: 7875
> 	linux/i2c.h: 5758
> 
> .
> 
> > > And then for <linux/pci.h> and again for <linux/platform_driver.h> and
> > > .... That's why those are included here, too, to at least use the "one
> > > rebuild needed" window for the high-impact follow up changes, and
> > > trigger a rebuild only once (unless you happen to hit an inbetween
> > > commit while bisecting).
> > 
> > I disagree, it's a different set of files for each. But there could, of 
> > course, be some overlap. Maybe the overlaps is so significant it's 
> > nearly as same as linux/mod_devicetable.h, I don't know but is that what 
> > you're trying to imply?
> 
> Yes, these are all headers with many users. So modifying them in bulk
> triggers less rebuilds than doing the modifications in separate series
> that go in via separate trees.
> 
> > > > What you can achieve is preventing those "normal" mod_devicetable.h 
> > > > changes enforcing rebuilds right after this has been applied, but that is 
> > > > just the rebuilds as status quo would have without this series and no 
> > > > more.
> > > 
> > > This is exactly the goal. I don't get the "just" part of your argument.
> > > The point is that a modification to struct i2c_device_id should only
> > > trigger a rebuild of i2c drivers, but not of pci, platform, spi and all
> > > the others, too.
> > 
> > With "just" I meant it's no worse than status quo. If that prolongs one or 
> > two kernel versions more to avoid mid-rc cycle merge it doesn't really 
> > seem so high cost.
> > 
> > And I understand where you want to end up, and you could end up there 
> > using steps as well:
> > 
> > 1. add linux/device-id/* + include them from mod_devicetable.h
> 
> Done, that's patch #1.
>
> > 2. convert stuff to use linux/device-id/* without removing includes from 
> > mod_devicetable.h
> 
> patch #15 and #16 are in that category, it's not complete yet, but I
> catched the high-impact usages. Again on -rc1 + this patch set there are
> 1921 .o files left that make use of linux/mod_devicetable.h. Roughly
> half of them via <linux/mdio.h>:
> 
>     ...
>      49   drivers/net/wireless/intel/iwlwifi/iwl-config.h
>     179   include/linux/mmc/card.h
>     188   include/sound/soc-acpi.h
>     231   include/linux/hid.h
>     925   include/linux/mdio.h
> 
> In retrospect it would have been great to tackle <linux/mdio.h> as part
> of this series, too, but I won't modify it for that any more now. I'll
> address that for the next merge window.
> 
> > 3. bulk remove linux/device-id/* includes from mod_devicetable.h
> 
> This isn't done yet, in the end I want to remove that header, but that
> is somehow "bulk remove linux/device-id/*". And that indeed is planned
> to be done later. Each patch of type 2 reduces the impact of that
> change, so postponing doesn't hurt a lot.
> 
> > 2 & 3 are chunkable/iteratable so they don't need to occur for all at once 
> > as long as 3 is not split per one linux/device-id/* its rebuild cost isn't 
> > that huge compared with the status quo.
> 
> Each patch of type 2 is potentially expensive. Less than modifying
> <linux/mod_devicetable.h> directly, and depending on which stuff it
> touched the impact is high or not. For a .c file it's low, for the above
> mentioned headers it's high.
> 
> So the justification to get the whole series in in one go is for the
> high impact headers that they should be near (in git history) to patch
> #1 to reduce rebuilds and for .c files that the automation that created
> the changes is reliable and only breaks in corner cases (like a driver
> using `of ## _device_id` which my script won't identify as a usage of
> `of_device_id`). And if you say the justification for the .c files (i.e.
> patch #16) is weak, I agree and we can postpone that. But please let's
> keep #1 and #15 (and thus also #2 - #14) together.
> 
> Maybe the relevant difference between us, that makes us judge the
> circumstances differently, is that I often do build tests on my laptop
> and at Intel you might have a beefy server to do that?!
> 
> Best regards
> Uwe
>