Date:   Sun, 14 Nov 2021 04:31:43 +0100
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/1] PCI: probe: Use pci_find_vsec_capability() when
 looking for TBT devices
Message-ID: <YZCDHxOwogxPpuWy@rocinante>
References: <20211109151604.17086-1-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211109151604.17086-1-andriy.shevchenko@linux.intel.com>
Precedence: bulk

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