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

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

[Andy Shevchenko]

Currently the set_pcie_thunderbolt() opens code pci_find_vsec_capability().
Refactor the former to use the latter. No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/probe.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 087d3658f75c..db5a0762da03 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1579,20 +1579,11 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev)
 
 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;
 }
 
 static void set_pcie_untrusted(struct pci_dev *dev)
-- 
2.33.0

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

[Krzysztof Wilczyński]

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

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

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 04:31:43AM +0100, Krzysztof Wilczyński wrote:
> 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:
> ...
> 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.

You mean https://lore.kernel.org/linux-fpga/20211109154127.18455-1-andriy.shevchenko@linux.intel.com/T/#u?

It seems a bit hard to explain HW people how the Linux kernel development
process is working. (Yes, shame on me that I haven't compiled that one)

...

> > Currently the set_pcie_thunderbolt() opens code pci_find_vsec_capability().
> 
> I would write it as "open codes" in the above.

Hmm... Is anybody among us a native speaker (me — no)? :-)
But if you think it's better like this I'll definitely change.
(I admit I'm lost in a morphological analysis of the above two
 words)

...

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

Thank you!

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Wilczyński]

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:
> > ...
> > 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.
> 
> You mean https://lore.kernel.org/linux-fpga/20211109154127.18455-1-andriy.shevchenko@linux.intel.com/T/#u?

Ohh!  Thank you for doing it!  Sorry for mentioning it twice then, I wasn't
aware of the conversation there on the other mailing list.

> It seems a bit hard to explain HW people how the Linux kernel development
> process is working. (Yes, shame on me that I haven't compiled that one)

I see what you mean... (after reading the linked conversation).

> > > Currently the set_pcie_thunderbolt() opens code pci_find_vsec_capability().
> > 
> > I would write it as "open codes" in the above.
> 
> Hmm... Is anybody among us a native speaker (me — no)? :-)

Admittedly, neither am I, so hopefully Bjorn (or other native speaker) can
chime in.  Albeit, it's probably not worth spending a lot of time over.

> But if you think it's better like this I'll definitely change.
> (I admit I'm lost in a morphological analysis of the above two
>  words)

Sorry about that!  It was simple something I noticed while reading the
commit messaged - looked somewhat different than the usual to my unskilled
and untrained eyes.

	Krzysztof

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

[Lukas Wunner]

On Tue, Nov 09, 2021 at 05:16:04PM +0200, Andy Shevchenko wrote:
> -	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? */

Could you preserve that code comment please so that an uninitiated
reader knows what the is_thunderbolt flag is about?

Thanks!

Lukas

> -		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;

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

[Andy Shevchenko]

On Sun, Nov 14, 2021 at 07:22:31AM +0100, Lukas Wunner wrote:
> On Tue, Nov 09, 2021 at 05:16:04PM +0200, Andy Shevchenko wrote:

...

> > -		/* Is the device part of a Thunderbolt controller? */
> 
> Could you preserve that code comment please so that an uninitiated
> reader knows what the is_thunderbolt flag is about?

Sure!

-- 
With Best Regards,
Andy Shevchenko