Re: [PATCH v4] PCI: Detect and trust built-in Thunderbolt chips

[Lukas Wunner <lukas@wunner.de>]

On Wed, Aug 28, 2024 at 05:15:24PM -0400, Esther Shimanovich wrote:
> On Sun, Aug 25, 2024 at 10:49???AM Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, Aug 23, 2024 at 04:53:16PM +0000, Esther Shimanovich wrote:
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > +static bool pcie_has_usb4_host_interface(struct pci_dev *pdev)
> > > +{
> > > +     struct fwnode_handle *fwnode;
> > > +
> > > +     /*
> > > +      * For USB4, the tunneled PCIe root or downstream ports are marked
> > > +      * with the "usb4-host-interface" ACPI property, so we look for
> > > +      * that first. This should cover most cases.
> > > +      */
> > > +     fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
> > > +                                    "usb4-host-interface", 0);
> >
> > This is all ACPI only, so it should either be #ifdef'ed to CONFIG_ACPI
> > or moved to drivers/pci/pci-acpi.c.
> >
> > Alternatively, it could be moved to arch/x86/pci/ because ACPI can also
> > be enabled on arm64 or riscv but the issue seems to only affect x86.
> 
> Thanks for the feedback! Adding an #ifdef to CONFIG_ACPI seems more
> straightforward, but I do like the idea of not having unnecessary code
> run on non-x86 systems.
> 
> I'd appreciate some guidance here. How would I move a portion of a
> function into a completely different location in the kernel src?
> Could you show me an example?

One way to do this would be to move pcie_is_tunneled(),
pcie_has_usb4_host_interface() and pcie_switch_directly_under()
to arch/x86/pci/acpi.c.

Rename pcie_is_tunneled() to arch_pci_dev_is_removable() and remove
the "static" declaration specifier from that function.

Add a function declaration for arch_pci_dev_is_removable() to
include/linux/pci.h.

Add a __weak arch_pci_dev_is_removable() function which just returns
false in drivers/pci/probe.c right above pci_set_removable().

And that's it.

See pcibios_device_add() for an example.

That's one way to do it.  It ensures that the code is only compiled
on x86 and only if CONFIG_ACPI=y.  Basically the linker picks the
arch_pci_dev_is_removable() in arch/x86/pci/acpi.c, or the empty
__weak function of the same name on !x86 or if CONFIG_ACPI=n.

An alternative approach would involve using an empty static inline.
I think the difference is that an empty static inline is optimized
away by the compiler, whereas the empty __weak function is not
optimized away by the compiler, but may be optimized away by the
linker if CONFIG_LTO=y.

For the static inline it's basically the same but you omit the
__weak arch_pci_dev_is_removable() in drivers/pci/probe.c and
instead constrain the function declaration in include/linux/pci.h to:
#if defined(CONFIG_X86) && defined(CONFIG_ACPI)
...and the #else branch would contain the empty static inline
which just returns false.

See pci_mmcfg_early_init() for an example.

Maybe the empty static inline is better because then the entire
"if (arch_pci_dev_is_removable(...))" clause can be optimized away
without reliance on CONFIG_LTO=y.

I hope I haven't confused you completely.

Thanks,

Lukas