| Message ID | 20230321233849.3408339-1-david.e.box@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/ASPM: pci_enable_link_state: Add argument to acquire bus lock | expand |
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 >
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 > >
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 --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; }
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(-)