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

[Kwapulinski, Piotr]

>-----Original Message-----
>From: Bjorn Helgaas <helgaas@xxxxxxxxxx> 
>Sent: Thursday, October 17, 2024 4:22 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@xxxxxxxxx>
>Cc: intel-wired-lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>
>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@xxxxxxxxx>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@xxxxxxxxx>
>> ---
>>  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.