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?
Bjorn
