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

[Bjorn Helgaas <helgaas@kernel.org>]

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.