[PATCH v2] PCI: Fix incorrect unlocking in pci_slot_trylock()

[PATCH v2] PCI: Fix incorrect unlocking in pci_slot_trylock()

[Jinhui Guo]

Commit a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
delegates the bridge device's pci_dev_trylock() to pci_bus_trylock() in
pci_slot_trylock(), but it forgets to remove the corresponding
pci_dev_unlock() when pci_bus_trylock() fails.

Before the commit, the code did:

  if (!pci_dev_trylock(dev)) /* <- lock bridge device */
    goto unlock;
  if (dev->subordinate) {
    if (!pci_bus_trylock(dev->subordinate)) {
      pci_dev_unlock(dev);   /* <- unlock bridge device */
      goto unlock;
    }
  }

After the commit the bridge-device lock is no longer taken, but the
pci_dev_unlock(dev) on the failure path was left in place, leading to
the bug.

This yields one of two errors:
1. A warning that the lock is being unlocked when no one holds it.
2. An incorrect unlock of a lock that belongs to another thread.

Fix it by removing the now-redundant pci_dev_unlock(dev) on the failure
path.

Fixes: a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
Cc: stable@vger.kernel.org
Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Hi, all

v1: https://lore.kernel.org/all/20251211123635.2215-1-guojinhui.liam@bytedance.com/

Changelog in v1 -> v2
 - The v1 commit message was too brief, so I’ve sent v2 with more detail.
 - Remove the braces from the if (!pci_bus_trylock(dev->subordinate)) statement.

Sorry for the noise.

Best Regards,
Jinhui

 drivers/pci/pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 13dbb405dc31..59319e08fca6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5346,10 +5346,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		if (!dev->slot || dev->slot != slot)
 			continue;
 		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;
 	}
-- 
2.20.1

Re: [PATCH v2] PCI: Fix incorrect unlocking in pci_slot_trylock()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2411 bytes --]

On Fri, 12 Dec 2025, Jinhui Guo wrote:

> Commit a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
> delegates the bridge device's pci_dev_trylock() to pci_bus_trylock() in
> pci_slot_trylock(), but it forgets to remove the corresponding
> pci_dev_unlock() when pci_bus_trylock() fails.
> 
> Before the commit, the code did:
> 
>   if (!pci_dev_trylock(dev)) /* <- lock bridge device */
>     goto unlock;
>   if (dev->subordinate) {
>     if (!pci_bus_trylock(dev->subordinate)) {
>       pci_dev_unlock(dev);   /* <- unlock bridge device */
>       goto unlock;
>     }
>   }
> 
> After the commit the bridge-device lock is no longer taken, but the
> pci_dev_unlock(dev) on the failure path was left in place, leading to
> the bug.
> 
> This yields one of two errors:
> 1. A warning that the lock is being unlocked when no one holds it.
> 2. An incorrect unlock of a lock that belongs to another thread.
> 
> Fix it by removing the now-redundant pci_dev_unlock(dev) on the failure
> path.
> 
> Fixes: a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Please don't make tags like this unless the other people explicitly give 
them to the patch.

Other than that, this looks okay to me.

-- 
 i.

> ---
> 
> Hi, all
> 
> v1: https://lore.kernel.org/all/20251211123635.2215-1-guojinhui.liam@bytedance.com/
> 
> Changelog in v1 -> v2
>  - The v1 commit message was too brief, so I’ve sent v2 with more detail.
>  - Remove the braces from the if (!pci_bus_trylock(dev->subordinate)) statement.
> 
> Sorry for the noise.
> 
> Best Regards,
> Jinhui
> 
>  drivers/pci/pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 13dbb405dc31..59319e08fca6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5346,10 +5346,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
>  		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;
>  	}
> 

[RESEND PATCH v2] PCI: Fix incorrect unlocking in pci_slot_trylock()

[Jinhui Guo]

Commit a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
delegates the bridge device's pci_dev_trylock() to pci_bus_trylock() in
pci_slot_trylock(), but it forgets to remove the corresponding
pci_dev_unlock() when pci_bus_trylock() fails.

Before the commit, the code did:

  if (!pci_dev_trylock(dev)) /* <- lock bridge device */
    goto unlock;
  if (dev->subordinate) {
    if (!pci_bus_trylock(dev->subordinate)) {
      pci_dev_unlock(dev);   /* <- unlock bridge device */
      goto unlock;
    }
  }

After the commit the bridge-device lock is no longer taken, but the
pci_dev_unlock(dev) on the failure path was left in place, leading to
the bug.

This yields one of two errors:
1. A warning that the lock is being unlocked when no one holds it.
2. An incorrect unlock of a lock that belongs to another thread.

Fix it by removing the now-redundant pci_dev_unlock(dev) on the failure
path.

Fixes: a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
Cc: stable@vger.kernel.org
Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
---

Hi, all

Resent v2 to drop the Acked-by tag; no code changes. Sorry for the noise again.

v1: https://lore.kernel.org/all/20251211123635.2215-1-guojinhui.liam@bytedance.com/

Changelog in v1 -> v2
 - The v1 commit message was too brief, so I’ve sent v2 with more detail.
 - Remove the braces from the if (!pci_bus_trylock(dev->subordinate)) statement.

Best Regards,
Jinhui

 drivers/pci/pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 13dbb405dc31..59319e08fca6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5346,10 +5346,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		if (!dev->slot || dev->slot != slot)
 			continue;
 		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;
 	}
-- 
2.20.1

Re: [RESEND PATCH v2] PCI: Fix incorrect unlocking in pci_slot_trylock()

[dan.j.williams]

Jinhui Guo wrote:
> Commit a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
> delegates the bridge device's pci_dev_trylock() to pci_bus_trylock() in
> pci_slot_trylock(), but it forgets to remove the corresponding
> pci_dev_unlock() when pci_bus_trylock() fails.
> 
> Before the commit, the code did:
> 
>   if (!pci_dev_trylock(dev)) /* <- lock bridge device */
>     goto unlock;
>   if (dev->subordinate) {
>     if (!pci_bus_trylock(dev->subordinate)) {
>       pci_dev_unlock(dev);   /* <- unlock bridge device */
>       goto unlock;
>     }
>   }
> 
> After the commit the bridge-device lock is no longer taken, but the
> pci_dev_unlock(dev) on the failure path was left in place, leading to
> the bug.
> 
> This yields one of two errors:
> 1. A warning that the lock is being unlocked when no one holds it.
> 2. An incorrect unlock of a lock that belongs to another thread.
> 
> Fix it by removing the now-redundant pci_dev_unlock(dev) on the failure
> path.
> 
> Fixes: a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
> ---
> 
> Hi, all
> 
> Resent v2 to drop the Acked-by tag; no code changes. Sorry for the noise again.
> 
> v1: https://lore.kernel.org/all/20251211123635.2215-1-guojinhui.liam@bytedance.com/
> 
> Changelog in v1 -> v2
>  - The v1 commit message was too brief, so I’ve sent v2 with more detail.
>  - Remove the braces from the if (!pci_bus_trylock(dev->subordinate)) statement.
> 
> Best Regards,
> Jinhui

I ended up also reviewing Keith's version of the same [1], but since this
one was posted earlier, go with this one.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

[1]: http://lore.kernel.org/20260116184150.3013258-1-kbusch@meta.com

Re: [RESEND PATCH v2] PCI: Fix incorrect unlocking in pci_slot_trylock()

[Bjorn Helgaas]

[+cc Keith, Alex, Lukas]

On Fri, Dec 12, 2025 at 10:55:28PM +0800, Jinhui Guo wrote:
> Commit a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
> delegates the bridge device's pci_dev_trylock() to pci_bus_trylock() in
> pci_slot_trylock(), but it forgets to remove the corresponding
> pci_dev_unlock() when pci_bus_trylock() fails.
> 
> Before the commit, the code did:
> 
>   if (!pci_dev_trylock(dev)) /* <- lock bridge device */
>     goto unlock;
>   if (dev->subordinate) {
>     if (!pci_bus_trylock(dev->subordinate)) {
>       pci_dev_unlock(dev);   /* <- unlock bridge device */
>       goto unlock;
>     }
>   }
> 
> After the commit the bridge-device lock is no longer taken, but the
> pci_dev_unlock(dev) on the failure path was left in place, leading to
> the bug.
> 
> This yields one of two errors:
> 1. A warning that the lock is being unlocked when no one holds it.
> 2. An incorrect unlock of a lock that belongs to another thread.
> 
> Fix it by removing the now-redundant pci_dev_unlock(dev) on the failure
> path.
> 
> Fixes: a4e772898f8b ("PCI: Add missing bridge lock to pci_bus_lock()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>

Applied to pci/virtualization for v6.20, thanks!

> ---
> 
> Hi, all
> 
> Resent v2 to drop the Acked-by tag; no code changes. Sorry for the noise again.
> 
> v1: https://lore.kernel.org/all/20251211123635.2215-1-guojinhui.liam@bytedance.com/
> 
> Changelog in v1 -> v2
>  - The v1 commit message was too brief, so I’ve sent v2 with more detail.
>  - Remove the braces from the if (!pci_bus_trylock(dev->subordinate)) statement.
> 
> Best Regards,
> Jinhui
> 
>  drivers/pci/pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 13dbb405dc31..59319e08fca6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5346,10 +5346,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
>  		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;
>  	}
> -- 
> 2.20.1