| Message ID | 20211109151604.17086-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v1,1/1] PCI: probe: Use pci_find_vsec_capability() when looking for TBT devices | expand |
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
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;
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!
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!
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
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)
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(-)