diff mbox series

PCI/ASPM: pci_enable_link_state: Add argument to acquire bus lock

Message ID 20230321233849.3408339-1-david.e.box@linux.intel.com
State New
Headers
Series PCI/ASPM: pci_enable_link_state: Add argument to acquire bus lock |

Commit Message

David E. Box March 21, 2023, 11:38 p.m. UTC 
  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@linux.intel.com>
Fixes: de82f60f9c86 ("PCI/ASPM: Add pci_enable_link_state()")
Link: https://lore.kernel.org/linux-pci/ZBjko%2FifunIwsK2v@intel.com/
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 2 +-
 drivers/pci/pcie/aspm.c      | 9 ++++++---
 include/linux/pci.h          | 5 +++--
 3 files changed, 10 insertions(+), 6 deletions(-)

Comments

Bjorn Helgaas March 22, 2023, 8:44 p.m. UTC | #1 
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@linux.intel.com>
> 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.

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.

Nit: I prefer to add "()" after function names in the commit log to
make it more obvious that they're functions and help distinguish them
from variables.

> Link: https://lore.kernel.org/linux-pci/ZBjko%2FifunIwsK2v@intel.com/
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  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
>
Bjorn Helgaas March 22, 2023, 8:57 p.m. UTC | #2 
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@linux.intel.com>
> > 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@intel.com/
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  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
> >
David E. Box March 22, 2023, 10:46 p.m. UTC | #3 
Hi Bjorn,

On Wed, 2023-03-22 at 15:57 -0500, Bjorn Helgaas wrote:
> 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@linux.intel.com>
> > > 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.

Yeah this should have been two patches.

> 
> 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().

It should work. I'll test the change.

David

> 
> > > Link: https://lore.kernel.org/linux-pci/ZBjko%2FifunIwsK2v@intel.com/
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > ---
> > >  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
> > >
diff mbox series

Patch

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