Skip to content

Commit a4e7728

Browse files
djbwbjorn-helgaas
authored andcommitted
PCI: Add missing bridge lock to pci_bus_lock()
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(). Link: https://lore.kernel.org/r/171711747501.1628941.15217746952476635316.stgit@dwillia2-xfh.jf.intel.com Reported-by: Imre Deak <[email protected]> Closes: http://lore.kernel.org/r/[email protected] Signed-off-by: Dan Williams <[email protected]> Signed-off-by: Keith Busch <[email protected]> [bhelgaas: squash in recursive locking deadlock fix from Keith Busch: https://lore.kernel.org/r/[email protected]] Signed-off-by: Bjorn Helgaas <[email protected]> Tested-by: Hans de Goede <[email protected]> Tested-by: Kalle Valo <[email protected]> Reviewed-by: Dave Jiang <[email protected]>
1 parent 920f646 commit a4e7728

File tree

1 file changed

+21
-14
lines changed

1 file changed

+21
-14
lines changed

drivers/pci/pci.c

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5445,10 +5445,12 @@ static void pci_bus_lock(struct pci_bus *bus)
54455445
{
54465446
struct pci_dev *dev;
54475447

5448+
pci_dev_lock(bus->self);
54485449
list_for_each_entry(dev, &bus->devices, bus_list) {
5449-
pci_dev_lock(dev);
54505450
if (dev->subordinate)
54515451
pci_bus_lock(dev->subordinate);
5452+
else
5453+
pci_dev_lock(dev);
54525454
}
54535455
}
54545456

@@ -5460,33 +5462,37 @@ static void pci_bus_unlock(struct pci_bus *bus)
54605462
list_for_each_entry(dev, &bus->devices, bus_list) {
54615463
if (dev->subordinate)
54625464
pci_bus_unlock(dev->subordinate);
5463-
pci_dev_unlock(dev);
5465+
else
5466+
pci_dev_unlock(dev);
54645467
}
5468+
pci_dev_unlock(bus->self);
54655469
}
54665470

54675471
/* Return 1 on successful lock, 0 on contention */
54685472
static int pci_bus_trylock(struct pci_bus *bus)
54695473
{
54705474
struct pci_dev *dev;
54715475

5476+
if (!pci_dev_trylock(bus->self))
5477+
return 0;
5478+
54725479
list_for_each_entry(dev, &bus->devices, bus_list) {
5473-
if (!pci_dev_trylock(dev))
5474-
goto unlock;
54755480
if (dev->subordinate) {
5476-
if (!pci_bus_trylock(dev->subordinate)) {
5477-
pci_dev_unlock(dev);
5481+
if (!pci_bus_trylock(dev->subordinate))
54785482
goto unlock;
5479-
}
5480-
}
5483+
} else if (!pci_dev_trylock(dev))
5484+
goto unlock;
54815485
}
54825486
return 1;
54835487

54845488
unlock:
54855489
list_for_each_entry_continue_reverse(dev, &bus->devices, bus_list) {
54865490
if (dev->subordinate)
54875491
pci_bus_unlock(dev->subordinate);
5488-
pci_dev_unlock(dev);
5492+
else
5493+
pci_dev_unlock(dev);
54895494
}
5495+
pci_dev_unlock(bus->self);
54905496
return 0;
54915497
}
54925498

@@ -5518,9 +5524,10 @@ static void pci_slot_lock(struct pci_slot *slot)
55185524
list_for_each_entry(dev, &slot->bus->devices, bus_list) {
55195525
if (!dev->slot || dev->slot != slot)
55205526
continue;
5521-
pci_dev_lock(dev);
55225527
if (dev->subordinate)
55235528
pci_bus_lock(dev->subordinate);
5529+
else
5530+
pci_dev_lock(dev);
55245531
}
55255532
}
55265533

@@ -5546,14 +5553,13 @@ static int pci_slot_trylock(struct pci_slot *slot)
55465553
list_for_each_entry(dev, &slot->bus->devices, bus_list) {
55475554
if (!dev->slot || dev->slot != slot)
55485555
continue;
5549-
if (!pci_dev_trylock(dev))
5550-
goto unlock;
55515556
if (dev->subordinate) {
55525557
if (!pci_bus_trylock(dev->subordinate)) {
55535558
pci_dev_unlock(dev);
55545559
goto unlock;
55555560
}
5556-
}
5561+
} else if (!pci_dev_trylock(dev))
5562+
goto unlock;
55575563
}
55585564
return 1;
55595565

@@ -5564,7 +5570,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
55645570
continue;
55655571
if (dev->subordinate)
55665572
pci_bus_unlock(dev->subordinate);
5567-
pci_dev_unlock(dev);
5573+
else
5574+
pci_dev_unlock(dev);
55685575
}
55695576
return 0;
55705577
}

0 commit comments

Comments
 (0)