On Wed, Mar 22, 2023 at 03:45:01PM -0500, Bjorn Helgaas wrote:
> Hi David,
>
> On Tue, Mar 21, 2023 at 04:38:49PM -0700, David E. Box wrote:
> > The VMD driver calls pci_enabled_link_state as a callback from
> > pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> > warning. Add an argument to pci_enable_link_state to set whether the lock
> > should be acquired. In the VMD driver, set the argument to false since the
> > lock will already be obtained by pci_bus_walk.
> >
> > Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Fixes: de82f60f9c86 ("PCI/ASPM: Add pci_enable_link_state()")
>
> This means "if your kernel includes de82f60f9c86, you probably want to
> backport this fix to it." But that's not the case here. This patch
> is not fixing an issue with de82f60f9c86, so I don't think there's a
> reason to include a "Fixes" line.
Oops, sorry, forgot to engage brain before hitting "send".
I think the "Fixes" line should reference f492edb40b54 ("PCI: vmd: Add
quirk to configure PCIe ASPM and LTR") instead, since that's where the
locking problem started.
> This patch is adding functionality that is only needed by some other
> patch, and it should be part of a series that also includes the patch
> that uses it to make sure they go together.
And I see that the use *is* included in this patch. But I don't
really like this pattern:
vmd_probe
vmd_enable_domain
vmd->bus = pci_create_root_bus(...);
pci_scan_child_bus(vmd->bus);
pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
because pci_walk_bus() makes locking complicated (as this issue shows)
and it doesn't work for hot-added devices (I don't know if that's an
issue for VMD, but the pattern gets copied to places where it *is*).
Normally vmd_pm_enable_quirk() would be done by making it an actual
DECLARE_PCI_FIXUP_HEADER() or DECLARE_PCI_FIXUP_FINAL(), so it would
be called automatically by the PCI core when a new device is
enumerated. Would that work here? If it would, I don't think you'd
need to add the extra flag to pci_enable_link_state().
> > Link: https://lore.kernel.org/linux-pci/ZBjko%2FifunIwsK2v@xxxxxxxxx/
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > drivers/pci/controller/vmd.c | 2 +-
> > drivers/pci/pcie/aspm.c | 9 ++++++---
> > include/linux/pci.h | 5 +++--
> > 3 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index 990630ec57c6..45aa35744eae 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -737,7 +737,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > return 0;
> >
> > - pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL, false);
> >
> > pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > if (!pos)
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 66d7514ca111..5b5a600bb864 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1147,8 +1147,9 @@ EXPORT_SYMBOL(pci_disable_link_state);
> > *
> > * @pdev: PCI device
> > * @state: Mask of ASPM link states to enable
> > + * @sem: Boolean to acquire/release pci_bus_sem
> > */
> > -int pci_enable_link_state(struct pci_dev *pdev, int state)
> > +int pci_enable_link_state(struct pci_dev *pdev, int state, bool sem)
> > {
> > struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> >
> > @@ -1165,7 +1166,8 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> > return -EPERM;
> > }
> >
> > - down_read(&pci_bus_sem);
> > + if (sem)
> > + down_read(&pci_bus_sem);
> > mutex_lock(&aspm_lock);
> > link->aspm_default = 0;
> > if (state & PCIE_LINK_STATE_L0S)
> > @@ -1186,7 +1188,8 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
> > link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> > pcie_set_clkpm(link, policy_to_clkpm_state(link));
> > mutex_unlock(&aspm_lock);
> > - up_read(&pci_bus_sem);
> > + if (sem)
> > + up_read(&pci_bus_sem);
> >
> > return 0;
> > }
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index fafd8020c6d7..a6f9f24b39fd 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1707,7 +1707,7 @@ extern bool pcie_ports_native;
> > #ifdef CONFIG_PCIEASPM
> > int pci_disable_link_state(struct pci_dev *pdev, int state);
> > int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> > -int pci_enable_link_state(struct pci_dev *pdev, int state);
> > +int pci_enable_link_state(struct pci_dev *pdev, int state, bool sem);
> > void pcie_no_aspm(void);
> > bool pcie_aspm_support_enabled(void);
> > bool pcie_aspm_enabled(struct pci_dev *pdev);
> > @@ -1716,7 +1716,8 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
> > { return 0; }
> > static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
> > { return 0; }
> > -static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
> > +static inline int
> > +pci_enable_link_state(struct pci_dev *pdev, int state, bool sem)
> > { return 0; }
> > static inline void pcie_no_aspm(void) { }
> > static inline bool pcie_aspm_support_enabled(void) { return false; }
> > --
> > 2.34.1
> >
