[RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[YuanShang]

An upstream bridge is shared by every function below it, so taking it
with pci_dev_trylock() makes the release-time reset fail whenever two
functions under the same bridge are released at the same time.  One
wins the trylock and resets; the others take the goto and are handed
back to the next user without ever being reset.  Nothing is logged.

This is easy to hit with SR-IOV, where all VFs of a device sit behind
the same bridge.  Releasing just two VFs concurrently is already
enough; which one wins the trylock is random between runs.

Take the bridge lock blocking instead.  The lock order (bridge, then
device) matches pci_bus_lock(), which takes both blocking.  The
trylock on the device itself is left alone.

Concurrent releases now serialise their resets, so a teardown of many
functions under one bridge pays one FLR settling time per function.

Fixes: 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()")
Cc: stable@vger.kernel.org
Signed-off-by: YuanShang <YuanShang.Mao@amd.com>
---
Hi Alex, Anthony,

Sending this as an RFC because I would like to know whether the approach
is acceptable before going further.

I hit this with SR-IOV: all VFs of a device share one upstream bridge, so
when several VFs are released at the same time only one of them wins the
trylock and gets reset.  The rest silently skip the reset.  Two concurrent
releases are enough to reproduce it here.

My question is whether taking the bridge lock blocking is safe.  I have
run this without problems, but I do not fully understand what the trylock
on the bridge was protecting against -- 962ae6892d8b added it to silence
the unlocked-SBR warning, and it is not obvious to me whether blocking
there can deadlock.  If there is a path I am missing, I would rather hear
it now.

An alternative would be to keep the trylock but at least log when the
reset is skipped, since today it is completely silent.

Not yet tested under lockdep; I am setting that up.

 drivers/vfio/pci/vfio_pci_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 362c375a0579..8966a5bea406 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -790,8 +790,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 	 */
 	if (vdev->reset_works) {
 		bridge = pci_upstream_bridge(pdev);
-		if (bridge && !pci_dev_trylock(bridge))
-			goto out_restore_state;
+		if (bridge)
+			pci_dev_lock(bridge);
 		if (pci_dev_trylock(pdev)) {
 			if (!__pci_reset_function_locked(pdev))
 				vdev->needs_reset = false;
@@ -801,7 +801,6 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 			pci_dev_unlock(bridge);
 	}
 
-out_restore_state:
 	pci_restore_state(pdev);
 out:
 	pci_disable_device(pdev);
-- 
2.25.1

RE: [RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[Zhang, Tiantian (Celine)]

AMD General

Hi @alex@shazbot.org, @anthony.pighin@nokia.com,

Just following up on the RFC below. Have you had a chance to take a look?
I'd appreciate any feedback on the proposed blocking bridge lock approach.
Thanks!

Best Regards,
Celine Zhang
-----Original Message-----
From: YuanShang Mao (River) <YuanShang.Mao@amd.com>
Sent: Tuesday, September 8, 2026 4:10 PM
To: alex@shazbot.org; anthony.pighin@nokia.com
Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Zhang, Tiantian (Celine) <Tiantian.Zhang@amd.com>
Subject: [RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

An upstream bridge is shared by every function below it, so taking it with pci_dev_trylock() makes the release-time reset fail whenever two functions under the same bridge are released at the same time.  One wins the trylock and resets; the others take the goto and are handed back to the next user without ever being reset.  Nothing is logged.

This is easy to hit with SR-IOV, where all VFs of a device sit behind the same bridge.  Releasing just two VFs concurrently is already enough; which one wins the trylock is random between runs.

Take the bridge lock blocking instead.  The lock order (bridge, then
device) matches pci_bus_lock(), which takes both blocking.  The trylock on the device itself is left alone.

Concurrent releases now serialise their resets, so a teardown of many functions under one bridge pays one FLR settling time per function.

Fixes: 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()")
Cc: stable@vger.kernel.org
Signed-off-by: YuanShang <YuanShang.Mao@amd.com>
---
Hi Alex, Anthony,

Sending this as an RFC because I would like to know whether the approach is acceptable before going further.

I hit this with SR-IOV: all VFs of a device share one upstream bridge, so when several VFs are released at the same time only one of them wins the trylock and gets reset.  The rest silently skip the reset.  Two concurrent releases are enough to reproduce it here.

My question is whether taking the bridge lock blocking is safe.  I have run this without problems, but I do not fully understand what the trylock on the bridge was protecting against -- 962ae6892d8b added it to silence the unlocked-SBR warning, and it is not obvious to me whether blocking there can deadlock.  If there is a path I am missing, I would rather hear it now.

An alternative would be to keep the trylock but at least log when the reset is skipped, since today it is completely silent.

Not yet tested under lockdep; I am setting that up.

 drivers/vfio/pci/vfio_pci_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 362c375a0579..8966a5bea406 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -790,8 +790,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
         */
        if (vdev->reset_works) {
                bridge = pci_upstream_bridge(pdev);
-               if (bridge && !pci_dev_trylock(bridge))
-                       goto out_restore_state;
+               if (bridge)
+                       pci_dev_lock(bridge);
                if (pci_dev_trylock(pdev)) {
                        if (!__pci_reset_function_locked(pdev))
                                vdev->needs_reset = false;
@@ -801,7 +801,6 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
                        pci_dev_unlock(bridge);
        }

-out_restore_state:
        pci_restore_state(pdev);
 out:
        pci_disable_device(pdev);
--
2.25.1

Re: [RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[Alex Williamson]

On Tue, 15 Sep 2026 08:08:56 +0000
"Zhang, Tiantian (Celine)" <Tiantian.Zhang@amd.com> wrote:

> AMD General
> 
> Hi @alex@shazbot.org, @anthony.pighin@nokia.com,
> 
> Just following up on the RFC below. Have you had a chance to take a look?
> I'd appreciate any feedback on the proposed blocking bridge lock approach.

Sashiko already outlined the locking issue:

https://sashiko.dev/#/patchset/20260908080946.2235849-1-YuanShang.Mao@amd.com

So no, it's not safe to promote this to a blocking lock.  Subsequent
opens will reset the device, the only remaining gap that I'm aware of
is an unbind with the device unreset, where there's some current
discussion[1] that devices should be reset on unbind if dirty.  Thanks,

Alex

[1]https://lore.kernel.org/all/20260818143906.1f58aecb@nvidia.com/

> -----Original Message-----
> From: YuanShang Mao (River) <YuanShang.Mao@amd.com>
> Sent: Tuesday, September 8, 2026 4:10 PM
> To: alex@shazbot.org; anthony.pighin@nokia.com
> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Zhang, Tiantian (Celine) <Tiantian.Zhang@amd.com>
> Subject: [RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()
> 
> An upstream bridge is shared by every function below it, so taking it with pci_dev_trylock() makes the release-time reset fail whenever two functions under the same bridge are released at the same time.  One wins the trylock and resets; the others take the goto and are handed back to the next user without ever being reset.  Nothing is logged.
> 
> This is easy to hit with SR-IOV, where all VFs of a device sit behind the same bridge.  Releasing just two VFs concurrently is already enough; which one wins the trylock is random between runs.
> 
> Take the bridge lock blocking instead.  The lock order (bridge, then
> device) matches pci_bus_lock(), which takes both blocking.  The trylock on the device itself is left alone.
> 
> Concurrent releases now serialise their resets, so a teardown of many functions under one bridge pays one FLR settling time per function.
> 
> Fixes: 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()")
> Cc: stable@vger.kernel.org
> Signed-off-by: YuanShang <YuanShang.Mao@amd.com>
> ---
> Hi Alex, Anthony,
> 
> Sending this as an RFC because I would like to know whether the approach is acceptable before going further.
> 
> I hit this with SR-IOV: all VFs of a device share one upstream bridge, so when several VFs are released at the same time only one of them wins the trylock and gets reset.  The rest silently skip the reset.  Two concurrent releases are enough to reproduce it here.
> 
> My question is whether taking the bridge lock blocking is safe.  I have run this without problems, but I do not fully understand what the trylock on the bridge was protecting against -- 962ae6892d8b added it to silence the unlocked-SBR warning, and it is not obvious to me whether blocking there can deadlock.  If there is a path I am missing, I would rather hear it now.
> 
> An alternative would be to keep the trylock but at least log when the reset is skipped, since today it is completely silent.
> 
> Not yet tested under lockdep; I am setting that up.
> 
>  drivers/vfio/pci/vfio_pci_core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 362c375a0579..8966a5bea406 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -790,8 +790,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>          */
>         if (vdev->reset_works) {
>                 bridge = pci_upstream_bridge(pdev);
> -               if (bridge && !pci_dev_trylock(bridge))
> -                       goto out_restore_state;
> +               if (bridge)
> +                       pci_dev_lock(bridge);
>                 if (pci_dev_trylock(pdev)) {
>                         if (!__pci_reset_function_locked(pdev))
>                                 vdev->needs_reset = false;
> @@ -801,7 +801,6 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>                         pci_dev_unlock(bridge);
>         }
> 
> -out_restore_state:
>         pci_restore_state(pdev);
>  out:
>         pci_disable_device(pdev);
> --
> 2.25.1
>