Re: [Intel-gfx] [PATCH] PCI/ASPM: pci_enable_link_state: Add argument to acquire bus lock

[Bjorn Helgaas]

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