| Message ID | 171711747501.1628941.15217746952476635316.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | a4e772898f8bf2e7e1cf661a12c60a5612c4afab |
| Headers | show |
| Series | PCI: Revert / replace the cfg_access_lock lockdep mechanism | expand |
On 5/30/24 6:04 PM, Dan Williams wrote: > One of the true positives that the cfg_access_lock lockdep effort > identified is this sequence: > > WARNING: CPU: 14 PID: 1 at drivers/pci/pci.c:4886 pci_bridge_secondary_bus_reset+0x5d/0x70 > RIP: 0010:pci_bridge_secondary_bus_reset+0x5d/0x70 > Call Trace: > <TASK> > ? __warn+0x8c/0x190 > ? pci_bridge_secondary_bus_reset+0x5d/0x70 > ? report_bug+0x1f8/0x200 > ? handle_bug+0x3c/0x70 > ? exc_invalid_op+0x18/0x70 > ? asm_exc_invalid_op+0x1a/0x20 > ? pci_bridge_secondary_bus_reset+0x5d/0x70 > pci_reset_bus+0x1d8/0x270 > vmd_probe+0x778/0xa10 > pci_device_probe+0x95/0x120 > > Where pci_reset_bus() users are triggering unlocked secondary bus > resets. Ironically pci_bus_reset(), several calls down from > pci_reset_bus(), uses pci_bus_lock() before issuing the reset which > locks everything *but* the bridge itself. > > For the same motivation as adding: > > bridge = pci_upstream_bridge(dev); > if (bridge) > pci_dev_lock(bridge); > > ...to pci_reset_function() for the "bus" and "cxl_bus" reset cases, add > pci_dev_lock() for @bus->self to pci_bus_lock(). > > Reported-by: Imre Deak <imre.deak@intel.com> > Closes: http://lore.kernel.org/r/6657833b3b5ae_14984b29437@dwillia2-xfh.jf.intel.com.notmuch > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/pci/pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 8df32a3a0979..aac5daad3188 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5444,6 +5444,7 @@ static void pci_bus_lock(struct pci_bus *bus) > { > struct pci_dev *dev; > > + pci_dev_lock(bus->self); > list_for_each_entry(dev, &bus->devices, bus_list) { > pci_dev_lock(dev); > if (dev->subordinate) > @@ -5461,6 +5462,7 @@ static void pci_bus_unlock(struct pci_bus *bus) > pci_bus_unlock(dev->subordinate); > pci_dev_unlock(dev); > } > + pci_dev_unlock(bus->self); > } > > /* Return 1 on successful lock, 0 on contention */ > @@ -5468,6 +5470,7 @@ static int pci_bus_trylock(struct pci_bus *bus) > { > struct pci_dev *dev; > > + pci_dev_lock(bus->self); > list_for_each_entry(dev, &bus->devices, bus_list) { > if (!pci_dev_trylock(dev)) > goto unlock; > @@ -5486,6 +5489,7 @@ static int pci_bus_trylock(struct pci_bus *bus) > pci_bus_unlock(dev->subordinate); > pci_dev_unlock(dev); > } > + pci_dev_unlock(bus->self); > return 0; > } > >
On Thu, May 30, 2024 at 06:04:35PM -0700, Dan Williams wrote: > @@ -5444,6 +5444,7 @@ static void pci_bus_lock(struct pci_bus *bus) > { > struct pci_dev *dev; > > + pci_dev_lock(bus->self); > list_for_each_entry(dev, &bus->devices, bus_list) { > pci_dev_lock(dev); > if (dev->subordinate) The very next line is: pci_bus_lock(dev->subordinate); The code had just locked "dev". The recursive call will then lock dev->subordinate->self, which is "dev", the same that was just locked a moment ago: deadlocked. Here's an idea: --- diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 46beaf1815fab..4f7cf693857f4 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5487,8 +5487,9 @@ static void pci_bus_lock(struct pci_bus *bus) pci_dev_lock(bus->self); list_for_each_entry(dev, &bus->devices, bus_list) { - pci_dev_lock(dev); - if (dev->subordinate) + if (!dev->subordinate) + pci_dev_lock(dev); + else pci_bus_lock(dev->subordinate); } } @@ -5501,7 +5502,8 @@ static void pci_bus_unlock(struct pci_bus *bus) list_for_each_entry(dev, &bus->devices, bus_list) { if (dev->subordinate) pci_bus_unlock(dev->subordinate); - pci_dev_unlock(dev); + else + pci_dev_unlock(dev); } pci_dev_unlock(bus->self); } --
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 8df32a3a0979..aac5daad3188 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5444,6 +5444,7 @@ static void pci_bus_lock(struct pci_bus *bus) { struct pci_dev *dev; + pci_dev_lock(bus->self); list_for_each_entry(dev, &bus->devices, bus_list) { pci_dev_lock(dev); if (dev->subordinate) @@ -5461,6 +5462,7 @@ static void pci_bus_unlock(struct pci_bus *bus) pci_bus_unlock(dev->subordinate); pci_dev_unlock(dev); } + pci_dev_unlock(bus->self); } /* Return 1 on successful lock, 0 on contention */ @@ -5468,6 +5470,7 @@ static int pci_bus_trylock(struct pci_bus *bus) { struct pci_dev *dev; + pci_dev_lock(bus->self); list_for_each_entry(dev, &bus->devices, bus_list) { if (!pci_dev_trylock(dev)) goto unlock; @@ -5486,6 +5489,7 @@ static int pci_bus_trylock(struct pci_bus *bus) pci_bus_unlock(dev->subordinate); pci_dev_unlock(dev); } + pci_dev_unlock(bus->self); return 0; }
One of the true positives that the cfg_access_lock lockdep effort identified is this sequence: WARNING: CPU: 14 PID: 1 at drivers/pci/pci.c:4886 pci_bridge_secondary_bus_reset+0x5d/0x70 RIP: 0010:pci_bridge_secondary_bus_reset+0x5d/0x70 Call Trace: <TASK> ? __warn+0x8c/0x190 ? pci_bridge_secondary_bus_reset+0x5d/0x70 ? report_bug+0x1f8/0x200 ? handle_bug+0x3c/0x70 ? exc_invalid_op+0x18/0x70 ? asm_exc_invalid_op+0x1a/0x20 ? pci_bridge_secondary_bus_reset+0x5d/0x70 pci_reset_bus+0x1d8/0x270 vmd_probe+0x778/0xa10 pci_device_probe+0x95/0x120 Where pci_reset_bus() users are triggering unlocked secondary bus resets. Ironically pci_bus_reset(), several calls down from pci_reset_bus(), uses pci_bus_lock() before issuing the reset which locks everything *but* the bridge itself. For the same motivation as adding: bridge = pci_upstream_bridge(dev); if (bridge) pci_dev_lock(bridge); ...to pci_reset_function() for the "bus" and "cxl_bus" reset cases, add pci_dev_lock() for @bus->self to pci_bus_lock(). Reported-by: Imre Deak <imre.deak@intel.com> Closes: http://lore.kernel.org/r/6657833b3b5ae_14984b29437@dwillia2-xfh.jf.intel.com.notmuch Cc: Dave Jiang <dave.jiang@intel.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/pci/pci.c | 4 ++++ 1 file changed, 4 insertions(+)