[+cc Hans, Kalle, Dave]
On Thu, Jul 11, 2024 at 12:36:50PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@xxxxxxxxxx>
>
> If one of the bus' devices has subordinates, the recursive call locks
> itself, so no need to lock the device before the recursion or it will
> surely deadlock.
>
> Fixes: dbc5b5c0d268f87 ("PCI: Add missing bridge lock to pci_bus_lock()"
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
dbc5b5c0d268f87 was queued for the v6.11 merge window on pci/reset, so
I squashed this fix into that commit. Thanks for the fix!
Hans, Kalle, Dave, I retained your Tested-by and Reviewed-by from
dbc5b5c0d268f87. Please let me know if you want to update or remove
those.
> ---
> Changes from v1:
>
> Fixed the same recursive locking for pci_bus_trylock(),
> pci_slot_lock(), and pci_slot_trylock()
>
> drivers/pci/pci.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index df550953fa260..82b74d3f6bb1e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5488,9 +5488,10 @@ 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)
> pci_bus_lock(dev->subordinate);
> + else
> + pci_dev_lock(dev);
> }
> }
>
> @@ -5502,7 +5503,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);
> }
> @@ -5512,16 +5514,15 @@ static int pci_bus_trylock(struct pci_bus *bus)
> {
> struct pci_dev *dev;
>
> - pci_dev_lock(bus->self);
> + if (!pci_dev_trylock(bus->self))
> + return 0;
> +
> list_for_each_entry(dev, &bus->devices, bus_list) {
> - if (!pci_dev_trylock(dev))
> - goto unlock;
> if (dev->subordinate) {
> - if (!pci_bus_trylock(dev->subordinate)) {
> - pci_dev_unlock(dev);
> + if (!pci_bus_trylock(dev->subordinate))
> goto unlock;
> - }
> - }
> + } else if (!pci_dev_trylock(dev))
> + goto unlock;
> }
> return 1;
>
> @@ -5529,7 +5530,8 @@ static int pci_bus_trylock(struct pci_bus *bus)
> list_for_each_entry_continue_reverse(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);
> return 0;
> @@ -5563,9 +5565,10 @@ static void pci_slot_lock(struct pci_slot *slot)
> list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> if (!dev->slot || dev->slot != slot)
> continue;
> - pci_dev_lock(dev);
> if (dev->subordinate)
> pci_bus_lock(dev->subordinate);
> + else
> + pci_dev_lock(dev);
> }
> }
>
> @@ -5591,14 +5594,13 @@ static int pci_slot_trylock(struct pci_slot *slot)
> list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> if (!dev->slot || dev->slot != slot)
> continue;
> - if (!pci_dev_trylock(dev))
> - goto unlock;
> if (dev->subordinate) {
> if (!pci_bus_trylock(dev->subordinate)) {
> pci_dev_unlock(dev);
> goto unlock;
> }
> - }
> + } else if (!pci_dev_trylock(dev))
> + goto unlock;
> }
> return 1;
>
> @@ -5609,7 +5611,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
> continue;
> if (dev->subordinate)
> pci_bus_unlock(dev->subordinate);
> - pci_dev_unlock(dev);
> + else
> + pci_dev_unlock(dev);
> }
> return 0;
> }
> --
> 2.43.0
>
