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 <[email protected]>
---
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
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 <[email protected]>
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/[email protected]/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 <[email protected]>
Thank you!
--
With Best Regards,
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
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/[email protected]/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