Re: [PATCH v1 1/1] PCI: probe: Use pci_find_vsec_capability() when looking for TBT devices

["Krzysztof Wilczyński" <kw@linux.com>]

Hi Andy,

Nice find!  There might one more driver that leverages the vendor-specific
capabilities that seems to be also open coding pci_find_vsec_capability(),
as per:

  139-static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
  140-{
  141-	u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
  142-	int dfl_res_off, i, bars, voff = 0;
  143-	resource_size_t start, len;
  144-
  145-	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
  146-		vndr_hdr = 0;
  147:		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
  148-
  149:		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
  150-		    pcidev->vendor == PCI_VENDOR_ID_INTEL)
  151-			break;
  152-	}
  153-
  154-	if (!voff) {
  155-		dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
  156-		return -ENODEV;
  157-	}

Do you think that it would be worthwhile to also update this other driver
to use pci_find_vsec_capability() at the same time?  I might be nice to rid
of the other open coded implementation too.

> Currently the set_pcie_thunderbolt() opens code pci_find_vsec_capability().

I would write it as "open codes" in the above.

>  static void set_pcie_thunderbolt(struct pci_dev *dev)
>  {
> -	int vsec = 0;
> -	u32 header;
> +	u16 vsec;
>  
> -	while ((vsec = pci_find_next_ext_capability(dev, vsec,
> -						    PCI_EXT_CAP_ID_VNDR))) {
> -		pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header);
> -
> -		/* Is the device part of a Thunderbolt controller? */
> -		if (dev->vendor == PCI_VENDOR_ID_INTEL &&
> -		    PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) {
> -			dev->is_thunderbolt = 1;
> -			return;
> -		}
> -	}
> +	vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_TBT);
> +	if (vsec)
> +		dev->is_thunderbolt = 1;
>  }

Thank you!

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

	Krzysztof