Re: [PATCH] x86/platform/intel-mid: Use named initializer for pci_device_id array

["Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>]

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

Hello Andy,

On Sat, May 09, 2026 at 09:35:57AM +0300, Andy Shevchenko wrote:
> On Fri, May 08, 2026 at 12:49:41PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> > On Fri, May 08, 2026 at 10:15:51AM +0300, Andy Shevchenko wrote:
> > > On Fri, May 08, 2026 at 08:25:45AM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> > > > On Thu, May 07, 2026 at 08:24:18PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, May 7, 2026 at 6:40 PM Uwe Kleine-König (The Capable Hub)
> > > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > >  static const struct pci_device_id mid_pwr_pci_ids[] = {
> > > > > > -       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
> > > > > > -       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
> > > > > > +       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data = (kernel_ulong_t)&pnw_info },
> > > > > > +       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data = (kernel_ulong_t)&tng_info },
> > > > > >         {}
> > > > > >  };
> > > > > 
> > > > > Just use PCI_DEVICE_DATA() here and do whatever you want in the future
> > > > > there, once for all.
> > > > 
> > > > I see quite some benefit in having some things explicit in a driver and
> > > > not hide them behind a macro using _Generic magic. Yes being explicit is
> > > > more verbose and here requires to touch the driver twice. But in my
> > > > subjective view it's better to be able to look at the array in the
> > > > driver and be able to understand instantly what happens without having
> > > > to recurse into macro definitions to understand that. IMHO PCI_VDEVICE
> > > > is already at least borderline in that regard because it composes
> > > > identifier names (with the result that `INTEL` is used to build
> > > > `PCI_VENDOR_ID_INTEL` which takes away the possibility to jump to its
> > > > definition using the usual code editor functions) and it contains two
> > > > zeros initializing .class and .class_mask which also isn't obvious for
> > > > the casual reader.
> > > 
> > > > Yes PCI_DEVICE_DATA is more compact and for someone who touches PCI code
> > > > daily it's also readable and if your working for Intel you maybe even
> > > > know the value of PCI_VENDOR_ID_INTEL by heart and so never are in the
> > > > situation to want to easily jump to its definition.
> > > 
> > > The same argument may be applied to many macros that use concatenation
> > > (##) in them,
> > 
> > I fully agree!
> > 
> > > this is not an argument at all.
> > 
> > I don't understand how you deduce this then however.
> 
> You fully agreed on the ## for any macro, hence every macro using it is not okay.

I argued that ## results in the disadvantage of being harder to work
with and harder to understand. But as this is not about being absolutely
black or white, this is only one aspect and there has to be a weighing
of the benefits that a macro yields against disadvantages. The absolute
interpretation is yours only. What I agreed to is that is a weighing
has to be applied for every macro using ##.

> > Not every macro using ## is bad, but every such macro comes at a cost
> > that you have to consider when justifying the usefulness of the macro.
> 
> However, you also telling that not every macro is bad.

So this is not a contradiction.

> And that's my point, PCI_DEVICE_DATA() is not bad and grepping for the
> IDs is quite a niche task.

Note that my argument is about using ## *plus* the hidden assignment in
PCI_VDEVICE() (that PCI_DEVICE_DATA() also does BTW). *Plus* for
PCI_DEVICE_DATA() that it hides ".driver_data" (and in your suggestion
later ".driver_data_ptr" using _Generic).

> One can do partial grep and we all know that the PCI device IDs either in
> pci_ids.h or in the same file or same driver folder, so there is really easy to
> find the real definition (unlike, for example, le32_encode_bits() and other
> using ## to construct their names).

Every additional indirection added to some code makes understanding "only"
one indirection harder. But these sum up, and you either have to grep
for INTEL and sort out all the false positives (

$ git grep INTEL | wc -l
20756

hmm, that's much, then maybe:

$ git grep -E '#define.*INTEL' | wc -l
3451

. But maybe it's not a cpp define but an enum? That is much harder to
grep for. And even if you are able to identify the definition of
PCI_VENDOR_ID_INTEL in that haystack, you cannot be sure that's the used
one without understanding PCI_VDEVICE), or you have to look at the
definition of PCI_VDEVICE (or PCI_DEVICE_DATA) and understand that. With
an explicit PCI_VENDOR_ID_INTEL you can just invoke your editor function
to jump to its definition, or a mouse-over shows its value directly in
your editor. And it goes further with PCI_VENDOR_ID_PHILIPS being
available for code completion while PHILIPS most probably isn't.

(OK, you could argue that then not using any macro at all is the way to
go with my argumentation. That would also be fine for me. The advantage
of using a macro (and preferably exactly one and not the plethora of
macros we have today) is that this makes these easier greppable without
getting sdio_device_ids or virtio_device_ids into the mix as these also
have a .vendor and a .device.

> > In my subjective
> > weighing using both ## and assignments to .class and .class_mask makes
> > PCI_VDEVICE *at least* questionable. (When all initializers are using
> > names, the 2nd can be fixed.)
> > 
> > PCI_DEVICE_DATA using ## and the outlook of it using a _Generic
> > construct is IMHO even worse and so I hesitate going that route.
> 
> We are talking about the user visibility, which is in the users and not
> in the definition of the macro.

Using PCI_DEVICE_DATA is more compact, agreed. But someone who is
proficient in C directly understands what is happening when an array of
structs is initialized using names without having to lookup a macro
definition to understand where the driver data parameter ends up.
This is what I consider easier. Less beauty to the eye maybe, but
instantly detectable what actually happens.

> My point is to put all burden in one place
> id est PCI_DEVICE_DATA() macro, your point is to spread a churn all over
> the kernel which I disagree with.

In my approach the first step (U1) is:

	 static const struct pci_device_id mid_pwr_pci_ids[] = {
	-        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
	-        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
	+        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data = (kernel_ulong_t)&pnw_info },
	+        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data = (kernel_ulong_t)&tng_info },
		 {}
	 };

(i.e. the patch under discussion) and the second (U2) is

	 static int mid_pwr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
	 {
	-	struct mid_pwr_device_info *info = (void *)id->driver_data;
	+	const struct mid_pwr_device_info *info = id->driver_data_ptr;
		struct device *dev = &pdev->dev;
		struct mid_pwr *pwr;
		int ret;
	...
	 static const struct pci_device_id mid_pwr_pci_ids[] = {
	-        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data = (kernel_ulong_t)&pnw_info },
	-        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data = (kernel_ulong_t)&tng_info },
	+        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data_ptr = &pnw_info },
	+        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data_ptr = &tng_info },
		 {}
	 };

. With your suggested approach we have instead of U1 the following (A1):

	 static const struct pci_device_id mid_pwr_pci_ids[] = {
	-        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
	-        { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
	+        { PCI_DEVICE_DATA(INTEL, PENWELL), &pnw_info) },
	+        { PCI_DEVICE_DATA(INTEL, TANGIER), &tng_info) },
		 {}
	 };

and to benefit later from the union there is still the first hunk of U2
needed (A2):

	 static int mid_pwr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
	 {
	-	struct mid_pwr_device_info *info = (void *)id->driver_data;
	+	const struct mid_pwr_device_info *info = id->driver_data_ptr;
		struct device *dev = &pdev->dev;
		struct mid_pwr *pwr;
		int ret;

. I was under the impression you assumed there is no need for A2, but
that's wrong. Additionally U2 is easier to review and judge that is
correct than A2 because it uses an indirection less, while A2 depends on
.driver_data and .driver_data_ptr holding the same data. With my
approach there is always a directly visible 1:1 correspondance between
the array and the probe function.

Also if A2 at a later point is backported to stable it applies just fine
without A1 and the _Generic extention of PCI_DEVICE_DATA() and the added
union. With my approach it's more obvious that you also need U1 before
you can apply U2. (Yes, it still doesn't fail to apply without the added
union to pci_device_id, but that's three potential papercuts with your
approach vs. one with mine.)

So I don't buy your argument that A1 + A2 is less churn than U1 + U2. In
my book U1 + U2 is even less.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]