[PATCH iwl-next 1/2] PCI: Add PCI_VDEVICE_SUB helper macro

[PATCH iwl-next 1/2] PCI: Add PCI_VDEVICE_SUB helper macro

[Piotr Kwapulinski]

PCI_VDEVICE_SUB generates the pci_device_id struct layout for
the specific PCI device/subdevice. The subvendor field is set
to PCI_ANY_ID. Private data may follow the output.

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
---
 include/linux/pci.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

This patch is a part of the series from netdev.

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 573b4c4..2b6b2c8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1050,6 +1050,20 @@ struct pci_driver {
 	.vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
 	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0
 
+/**
+ * PCI_VDEVICE_SUB - describe a specific PCI device/subdevice in a short form
+ * @vend: the vendor name
+ * @dev: the 16 bit PCI Device ID
+ * @subdev: the 16 bit PCI Subdevice ID
+ *
+ * Generate the pci_device_id struct layout for the specific PCI
+ * device/subdevice. The subvendor field is set to PCI_ANY_ID. Private data
+ * may follow the output.
+ */
+#define PCI_VDEVICE_SUB(vend, dev, subdev) \
+	.vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
+	.subvendor = PCI_ANY_ID, .subdevice = subdev, 0, 0
+
 /**
  * PCI_DEVICE_DATA - macro used to describe a specific PCI device in very short form
  * @vend: the vendor name (without PCI_VENDOR_ID_ prefix)
-- 
2.43.0

Re: [PATCH iwl-next 1/2] PCI: Add PCI_VDEVICE_SUB helper macro

[Bjorn Helgaas]

On Thu, Oct 17, 2024 at 03:16:47PM +0200, Piotr Kwapulinski wrote:
> PCI_VDEVICE_SUB generates the pci_device_id struct layout for
> the specific PCI device/subdevice. The subvendor field is set
> to PCI_ANY_ID. Private data may follow the output.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> ---
>  include/linux/pci.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> This patch is a part of the series from netdev.
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 573b4c4..2b6b2c8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1050,6 +1050,20 @@ struct pci_driver {
>  	.vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
>  	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0
>  
> +/**
> + * PCI_VDEVICE_SUB - describe a specific PCI device/subdevice in a short form
> + * @vend: the vendor name
> + * @dev: the 16 bit PCI Device ID
> + * @subdev: the 16 bit PCI Subdevice ID
> + *
> + * Generate the pci_device_id struct layout for the specific PCI
> + * device/subdevice. The subvendor field is set to PCI_ANY_ID. Private data
> + * may follow the output.
> + */
> +#define PCI_VDEVICE_SUB(vend, dev, subdev) \
> +	.vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
> +	.subvendor = PCI_ANY_ID, .subdevice = subdev, 0, 0

I don't think it's right to specify the subdevice (actually "Subsystem
ID" per spec) without specifying the subvendor ("Subsystem Vendor ID"
in the spec).

Subsystem IDs are assigned by the vendor, so they have to be used in
conjunction with the Subsystem Vendor ID.  See PCIe r6.0, sec
7.5.1.2.3:

  Values for the Subsystem ID are vendor assigned. Subsystem ID
  values, in conjunction with the Subsystem Vendor ID, form a unique
  identifier for the PCI product. Subsystem ID and Device ID values
  are distinct and unrelated to each other, and software should not
  assume any relationship between them.

RE: [PATCH iwl-next 1/2] PCI: Add PCI_VDEVICE_SUB helper macro

[Kwapulinski, Piotr]

>-----Original Message-----
>From: Bjorn Helgaas <helgaas@kernel.org> 
>Sent: Thursday, October 17, 2024 4:22 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; bhelgaas@google.com; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
>Subject: Re: [PATCH iwl-next 1/2] PCI: Add PCI_VDEVICE_SUB helper macro
>
>On Thu, Oct 17, 2024 at 03:16:47PM +0200, Piotr Kwapulinski wrote:
>> PCI_VDEVICE_SUB generates the pci_device_id struct layout for the 
>> specific PCI device/subdevice. The subvendor field is set to 
>> PCI_ANY_ID. Private data may follow the output.
>> 
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
>> ---
>>  include/linux/pci.h | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> This patch is a part of the series from netdev.
>> 
>> diff --git a/include/linux/pci.h b/include/linux/pci.h index 
>> 573b4c4..2b6b2c8 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1050,6 +1050,20 @@ struct pci_driver {
>>  	.vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
>>  	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, 0, 0
>>  
>> +/**
>> + * PCI_VDEVICE_SUB - describe a specific PCI device/subdevice in a 
>> +short form
>> + * @vend: the vendor name
>> + * @dev: the 16 bit PCI Device ID
>> + * @subdev: the 16 bit PCI Subdevice ID
>> + *
>> + * Generate the pci_device_id struct layout for the specific PCI
>> + * device/subdevice. The subvendor field is set to PCI_ANY_ID. 
>> +Private data
>> + * may follow the output.
>> + */
>> +#define PCI_VDEVICE_SUB(vend, dev, subdev) \
>> +	.vendor = PCI_VENDOR_ID_##vend, .device = (dev), \
>> +	.subvendor = PCI_ANY_ID, .subdevice = subdev, 0, 0
>
>I don't think it's right to specify the subdevice (actually "Subsystem ID" per spec) without specifying the subvendor ("Subsystem Vendor ID"
>in the spec).
>
>Subsystem IDs are assigned by the vendor, so they have to be used in conjunction with the Subsystem Vendor ID.  See PCIe r6.0, sec
>7.5.1.2.3:
I'll add the subvendor to the interface.
Thank you for comments.
Piotr

>
>  Values for the Subsystem ID are vendor assigned. Subsystem ID
>  values, in conjunction with the Subsystem Vendor ID, form a unique
>  identifier for the PCI product. Subsystem ID and Device ID values
>  are distinct and unrelated to each other, and software should not
>  assume any relationship between them.