On Wed, Oct 23, 2024 at 03:58:21PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 29, 2024 at 11:32:47AM +0200, Lukas Wunner wrote:
> > 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@xxxxxxxxx> 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.
>
> Was there ever any followup on this? Do we need any?
>
> This uses fwnode_find_reference("usb4-host-interface"), and while
> "usb4-host-interface" is only defined for ACPI systems (as far as I
> know), the fwnode_find_reference() interface itself is not
> ACPI-specific.
>
> So maybe this is OK as-is, and it will just never find that property
> on non-ACPI systems?
Sigh, sorry for the noise. Right after sending this, I noticed that
Esther had posted a v5 with this rework:
https://lore.kernel.org/r/20240910-trust-tbt-fix-v5-1-7a7a42a5f496@xxxxxxxxxxxx
Although I'm still unclear on whether we actually need to make this
ACPI-specific as v5 does.
I'm also not sure about making it x86-specific, since the
"usb4-host-interface" property doesn't seem x86-specific, other than
maybe as an accidental consequence of some hardware implementations.
Bjorn
