linux-kernel - Re: [Intel-gfx] [PATCH] PCI/ASPM: pci_enable_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ab9bf3032ed46fc0586e089edc5aac6e71b331d8.camel@linux.intel.com>
Date:   Wed, 22 Mar 2023 15:46:55 -0700
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     ville.syrjala@...ux.intel.com, nirmal.patel@...ux.intel.com,
        jonathan.derrick@...ux.dev, lorenzo.pieralisi@....com,
        hch@...radead.org, kw@...ux.com, robh@...nel.org,
        bhelgaas@...gle.com, michael.a.bottini@...el.com,
        rafael@...nel.org, me@...ityamohan.in, linux-pci@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [Intel-gfx] [PATCH] PCI/ASPM: pci_enable_link_state: Add
 argument to acquire bus lock

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@...ux.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@...ux.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
> > >