From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1871E33F580 for ; Sat, 9 May 2026 13:42:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778334169; cv=none; b=FxelaL32FhK6CRkbbJsb+j9C6VRoBAa/IoKEueFULKXszF93BXYSP9vc7OqIY/wYC67bPYo6TPmhqJ+H0pbt5P6WYpIH0kDtKgPRPZ8iuy3bRp6hWJTmVF0p7rPQE8UIhMazi/Il/CZdGha3hUoBkS2P4ibkFlnAIrYNI6BA+2c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778334169; c=relaxed/simple; bh=H8f64CqHw207B2YqVnnASbvQJbHGVuRT2TwL57qeIA8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=nsppFipWmYsXLQRvTThXY5GksMY2aW/qTssigbNydl2AuNrnCuBUwxTPk8/VPok39N6oGPebHWAMlhrMvXADgPv9my5aGpcUw8fwtIanbjVpyqOMxr8AgJmMcx6xBF/Zg+G7H0mu+/gOL9LTcNo5nF/+Hf5oFIGkuFDm6xYuEw4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com; spf=pass smtp.mailfrom=baylibre.com; dkim=pass (2048-bit key) header.d=baylibre-com.20251104.gappssmtp.com header.i=@baylibre-com.20251104.gappssmtp.com header.b=HcK8cdZ+; arc=none smtp.client-ip=209.85.221.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=baylibre.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=baylibre-com.20251104.gappssmtp.com header.i=@baylibre-com.20251104.gappssmtp.com header.b="HcK8cdZ+" Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-44da2de25f3so1938085f8f.1 for ; Sat, 09 May 2026 06:42:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20251104.gappssmtp.com; s=20251104; t=1778334164; x=1778938964; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=m2OXWLUGYm/tsBV9clt4c5/WmiJ/xixOtQdufodnPxo=; b=HcK8cdZ+/QwKZ7vuSsp/lugdOO6fRv96xVyo72Jsr7GjW77+QAp4yDCwS+TuSHjFF4 TxZJg0Cj5MDW/pjmYTPtK/0MbvyH9E4G/SNF1hTywKBKZhTTdbEhoOnX7n+rws/7oUUW 6PYJEkngqRDK0bpj75IuMdZ63CfvIUoPcsW1YSXiKxfG7aEb1xH3QaVqIuFuyPY6LpIZ 8dmeXMI/PT+2/lYjRuwKq+UzCor4EXuS+EpnLHSRJGrh+FAJ54trb01sQb2Di9vf0w8x JGlDOaV/VO8xz14TdSE2AzqfW7wiyIpmFSmhidn8g5VuDI4aBkQbjNGXeCtAoxVTjEii 7g9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778334164; x=1778938964; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=m2OXWLUGYm/tsBV9clt4c5/WmiJ/xixOtQdufodnPxo=; b=bJ75ilZ05USo3QYNiB4u0zLDNB4um+hRU9FqPyuziBxvN03PoEk8kK8QkvaaNpgxq6 oKIYnjy3ePgN2V/ak17TV5AMqCjLgOumtv1r4xDiPt6LJQKkaLrC38fmK3eXo1uHTw4u z/AF0pEC1/NeIGFEfyQtMcjqzGOQAPWdao09ci90fOASsTKeISdSX8JI0hQQ0KO4nWlx jOaAt6SEJ+36CykT43f0pPg86ZAmUCrP3VZJpdT56YHDruqI2cOamxKjcEGdFOjEXjCf rRqEUtF3VTJR/wJcqX6D29qNXq/rowpzWw1n1H8SrPBmfcASsuqHjvgZz9egLX9oSbI6 WsOQ== X-Forwarded-Encrypted: i=1; AFNElJ9xdruzKDVJZkEOKBa3quodxdXJIXG3YjOdHQy2///MonpU9LcGWMfyl0aPV6KM9+H576fGSy1AcyPNbFA=@vger.kernel.org X-Gm-Message-State: AOJu0YxOf1f/d32HdEbBaF9uOUeUgbyDUGkhWO1XQtzdVjBEaIC1mlnv 2S4vTHivM/hmxItNCLU1iasoWUwxERi2j2stMbs0Nu0EKaYg0la/j+hrqYDcjIHnhMM= X-Gm-Gg: Acq92OFdRBC12Y1xeaqoJ9DSyS6uykGKzSH5sUE0fgIArn0bANBYn6kj5+Vkr9XbWhe qi47jde9Mq4yIxY9lRgqZKelbZ0sOXUBEwkE1OeOa49C2LnOHhQI8a+sOjNFwsquj4kSRLth90t NP/5p6nMyrPgEh//jhzLbkPQtooGMCPfHyJJg/F1CtUEKjMV7H2Pxo/ob3HZ/g4rYBbEsh5Ud75 kYLjgr/fihmGUkjBoSuFlG47BoNhqqLVog2ux2ADQb99NUsvZ1tsd0Gqor+j19WsABgUdgK3VRa kVUsywrRywGNsdGDI1bp6Nat8rgKtl4m1le1S1xSsuEOjQwp6bnLIubQ2gqSctBPIvpvJj/DVN1 3OpgwsE56yctmTTdgyCOzzNMFX7n5rDJb18gZgR4PNCij2jw5PXAaIeHwhmhnXIsFm1cst8STej 2TjWezKAq0Py0DzEoaf1Ko1eE5E1Nn X-Received: by 2002:a5d:4f8a:0:b0:454:9986:d22b with SMTP id ffacd0b85a97d-4549986d85fmr6794720f8f.1.1778334164167; Sat, 09 May 2026 06:42:44 -0700 (PDT) Received: from localhost ([2a02:8071:56d1:2de0:1d24:d58d:2b65:c291]) by smtp.gmail.com with UTF8SMTPSA id ffacd0b85a97d-454922715b9sm12962578f8f.36.2026.05.09.06.42.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 09 May 2026 06:42:43 -0700 (PDT) Date: Sat, 9 May 2026 15:42:42 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig_=28The_Capable_Hub=29?= To: Andy Shevchenko Cc: Andy Shevchenko , Andy Shevchenko , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , linux-kernel@vger.kernel.org, Markus Schneider-Pargmann Subject: Re: [PATCH] x86/platform/intel-mid: Use named initializer for pci_device_id array Message-ID: References: <20260507154043.3113348-2-u.kleine-koenig@baylibre.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="q3w4u4qcr4st2lk7" Content-Disposition: inline In-Reply-To: --q3w4u4qcr4st2lk7 Content-Type: text/plain; protected-headers=v1; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH] x86/platform/intel-mid: Use named initializer for pci_device_id array MIME-Version: 1.0 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=C3=B6nig (The Capa= ble 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=C3=B6nig (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=E2=80=AFPM Uwe Kleine-K=C3=B6nig (The= Capable Hub) > > > > > wrote: > > > > > > static const struct pci_device_id mid_pwr_pci_ids[] =3D { > > > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ul= ong_t)&pnw_info }, > > > > > > - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ul= ong_t)&tng_info }, > > > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_da= ta =3D (kernel_ulong_t)&pnw_info }, > > > > > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_da= ta =3D (kernel_ulong_t)&tng_info }, > > > > > > {} > > > > > > }; > > > > >=20 > > > > > Just use PCI_DEVICE_DATA() here and do whatever you want in the f= uture > > > > > there, once for all. > > > >=20 > > > > 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 explic= it 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 hav= ing > > > > to recurse into macro definitions to understand that. IMHO PCI_VDEV= ICE > > > > 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 i= ts > > > > definition using the usual code editor functions) and it contains t= wo > > > > zeros initializing .class and .class_mask which also isn't obvious = for > > > > the casual reader. > > >=20 > > > > 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 ev= en > > > > 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. > > >=20 > > > The same argument may be applied to many macros that use concatenation > > > (##) in them, > >=20 > > I fully agree! > >=20 > > > this is not an argument at all. > >=20 > > I don't understand how you deduce this then however. >=20 > You fully agreed on the ## for any macro, hence every macro using it is n= ot 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. >=20 > 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 e= asy to > find the real definition (unlike, for example, le32_encode_bits() and oth= er > 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 =2E 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.) > >=20 > > PCI_DEVICE_DATA using ## and the outlook of it using a _Generic > > construct is IMHO even worse and so I hesitate going that route. >=20 > 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[] =3D { - { 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 =3D (ke= rnel_ulong_t)&pnw_info }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data =3D (ke= rnel_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 =3D (void *)id->driver_data; + const struct mid_pwr_device_info *info =3D id->driver_data_ptr; struct device *dev =3D &pdev->dev; struct mid_pwr *pwr; int ret; ... static const struct pci_device_id mid_pwr_pci_ids[] =3D { - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data =3D (ke= rnel_ulong_t)&pnw_info }, - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data =3D (ke= rnel_ulong_t)&tng_info }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data_ptr =3D= &pnw_info }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data_ptr =3D= &tng_info }, {} }; =2E With your suggested approach we have instead of U1 the following (A1): static const struct pci_device_id mid_pwr_pci_ids[] =3D { - { 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 =3D (void *)id->driver_data; + const struct mid_pwr_device_info *info =3D id->driver_data_ptr; struct device *dev =3D &pdev->dev; struct mid_pwr *pwr; int ret; =2E 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 =2Edriver_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 --q3w4u4qcr4st2lk7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmn/Oc8ACgkQj4D7WH0S /k4KtwgAk73jXqcsRBJbxmDbCvWCgyuZvGZDzJW3l5U9uJvIiaUY79tFuk4z4EsX 9TEE71gqJ+0Imqgx/18cL3skDvbgjkGDNVl+XbgDcGJOaw7bDpvyPCzqnHn3rFeR IhsWHIVcKmSjBHxC1SX9mfw91YMOV5VqPydT38U4WofJhFDnWMSgw25omuaa0NEb uiwSBvSJN8YWvdYV2kH8RtpzxzFYlhmGYV3bC1qp13aLiX058qz/bW+s9r7ekKJc IuBdhG8+wnO9VMwGhNHTakmv3Ht5SxXKi7/41+gcKd0+BdCjrx2Ae9FalBqWD9ak GMsjYdYQGkA+nuD+cb7fiH66XrkMrw== =T+U7 -----END PGP SIGNATURE----- --q3w4u4qcr4st2lk7--