Re: [PATCH v3 07/12] PCI: Set ports for discrete USB4 controllers appropriately

[Bjorn Helgaas <helgaas@kernel.org>]

Make the subject specific, not just "appropriately."  I think you're
marking something *removable*, so include that.

On Fri, Feb 11, 2022 at 01:32:45PM -0600, Mario Limonciello wrote:
> Discrete USB4 controllers won't have ACPI nodes specifying which
> PCIe or XHCI port they are linked with.
> 
> In order to set the removable attribute appropriately, use the
> USB4 DVSEC extended capabability set on these root ports to determine
> if they are located on a discrete USB4 controller.

s/capabability/capability/

> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Link: https://usb.org/sites/default/files/USB4%20Specification%2020211116.zip
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/probe.c     | 33 +++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 67ca33188cba..1ed3e24db11e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -25,6 +25,8 @@
>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
>  #define CARDBUS_RESERVE_BUSNR	3
>  
> +#define PCI_DVSEC_ID_USB4	0x23
> +
>  static struct resource busn_resource = {
>  	.name	= "PCI busn",
>  	.start	= 0,
> @@ -1590,6 +1592,36 @@ static void set_pcie_untrusted(struct pci_dev *dev)
>  		dev->untrusted = true;
>  }
>  
> +static bool pci_is_discrete_usb4(struct pci_dev *dev)
> +{
> +	int dvsec_val = 0, pos;
> +	u32 hdr;
> +
> +	/* USB4 spec says vendors can use either */
> +	pos = pci_find_dvsec_capability(dev,
> +					PCI_VENDOR_ID_INTEL,
> +					PCI_DVSEC_ID_USB4);
> +	if (pos) {
> +		dvsec_val = 0x06;
> +	} else {
> +		pos = pci_find_dvsec_capability(dev,
> +						PCI_VENDOR_ID_USB_IF,
> +						PCI_DVSEC_ID_USB4);
> +		if (pos)
> +			dvsec_val = 0x01;
> +	}
> +	if (!dvsec_val)
> +		return false;
> +
> +	pci_read_config_dword(dev, pos + PCI_DVSEC_HEADER2, &hdr);
> +	if ((hdr & GENMASK(15, 0)) != dvsec_val)
> +		return false;
> +	/* this port is used for either NHI/PCIe tunnel/USB tunnel */

Capitalize comment like others in this file.

Spec reference would be helpful, too.  I don't know how to verify
any of these values.  The Link: above is great, but name, revision,
section number would be even better.

> +	if (hdr & GENMASK(18, 16))
> +		return true;
> +	return false;
> +}
> +
>  static void pci_set_removable(struct pci_dev *dev)
>  {
>  	struct pci_dev *parent = pci_upstream_bridge(dev);
> @@ -1612,6 +1644,7 @@ static void pci_set_removable(struct pci_dev *dev)
>  	if (vsec ||
>  	    dev->class == PCI_CLASS_SERIAL_USB_USB4 ||
>  	    pci_acpi_is_usb4(dev) ||
> +	    pci_is_discrete_usb4(dev) ||
>  	    (parent &&
>  	    (parent->external_facing || dev_is_removable(&parent->dev))))
>  		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 61b161d914f0..271326e058b9 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -3097,4 +3097,6 @@
>  
>  #define PCI_VENDOR_ID_NCUBE		0x10ff
>  
> +#define PCI_VENDOR_ID_USB_IF		0x1EC0

This file is supposed to be sorted by Vendor ID.  PCI_VENDOR_ID_XEN,
PCI_VENDOR_ID_OCZ, and PCI_VENDOR_ID_NCUBE screwed up, but you can put
USB_IF in the correct spot.

It's not 100%, but most entries use lower-case hex.