linux-kernel - Re: [PATCH iwl-next 1/2] PCI: Add PCI_VDEVICE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241017142152.GA685610@bhelgaas>
Date: Thu, 17 Oct 2024 09:21:52 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	bhelgaas@...gle.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Przemek Kitszel <przemyslaw.kitszel@...el.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@...el.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.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.