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

[YuanShang <YuanShang.Mao@amd.com>]

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