Commit 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()") makes vfio_pci_core_disable() trylock the upstream bridge before resetting the device, and skips the reset if the trylock fails. All VFs of a PF have the same upstream bridge. Take a device with two VFs, both used by one process. When the process is killed, both VFs can be released at the same time. The first one takes the bridge lock and does its FLR, the second one fails the trylock and is never reset. The bridge lock is only needed for a bus reset, which is done through the bridge. A VF is reset with FLR, and pci_parent_bus_reset() never resets the bus for it because the PF and the other VFs are on the same bus. Add pci_reset_needs_bridge_lock(), which returns true if any reset method enabled for the device is a bus reset, and only lock the bridge in vfio_pci_core_disable() when it does. Fixes: 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()") Cc: stable@vger.kernel.org Signed-off-by: Samiullah Khawaja --- Tested in QEMU with an igb PF behind a PCIe root port, bound to vfio-pci with two VFs. Both VFs are opened through VFIO and then closed from two threads at the same time, counting FLRs with a kprobe on pcie_flr(). Without this patch one VF missed its FLR in 50 of 50 rounds, with it both VFs were reset in every round. I will send a VFIO selftest for this separately. --- drivers/pci/pci.c | 28 ++++++++++++++++++++++++++++ drivers/vfio/pci/vfio_pci_core.c | 3 ++- include/linux/pci.h | 1 + 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 7c6f2aba8144..c44e4d4d8774 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5256,6 +5256,34 @@ int __pci_reset_function_locked(struct pci_dev *dev) } EXPORT_SYMBOL_GPL(__pci_reset_function_locked); +/** + * pci_reset_needs_bridge_lock - check whether a reset of @dev may need the + * upstream bridge to be locked + * @dev: PCI device to check + * + * A bus reset of @dev is done through its upstream bridge, so the caller must + * hold the bridge lock. The other reset methods only access @dev itself. + * + * Return: true if any of the reset methods enabled for @dev is a bus reset. + */ +bool pci_reset_needs_bridge_lock(struct pci_dev *dev) +{ + int i, m; + + for (i = 0; i < PCI_NUM_RESET_METHODS; i++) { + m = dev->reset_methods[i]; + if (!m) + break; + + if (pci_reset_fn_methods[m].reset_fn == pci_reset_bus_function || + pci_reset_fn_methods[m].reset_fn == cxl_reset_bus_function) + return true; + } + + return false; +} +EXPORT_SYMBOL_GPL(pci_reset_needs_bridge_lock); + /** * pci_init_reset_methods - check whether device can be safely reset * and store supported reset mechanisms. diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index fcbb2da860e2..ba3dfdadae39 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -777,7 +777,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev) * overwrite the previously restored configuration information. */ if (vdev->reset_works) { - bridge = pci_upstream_bridge(pdev); + bridge = pci_reset_needs_bridge_lock(pdev) ? + pci_upstream_bridge(pdev) : NULL; if (bridge && !pci_dev_trylock(bridge)) goto out_restore_state; if (pci_dev_trylock(pdev)) { diff --git a/include/linux/pci.h b/include/linux/pci.h index aa3765a20a96..cb2856f444ca 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1486,6 +1486,7 @@ void pcie_print_link_status(struct pci_dev *dev); int pcie_reset_flr(struct pci_dev *dev, bool probe); int pcie_flr(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev); +bool pci_reset_needs_bridge_lock(struct pci_dev *dev); int pci_reset_function(struct pci_dev *dev); int pci_reset_function_locked(struct pci_dev *dev); int pci_try_reset_function(struct pci_dev *dev); base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb -- 2.56.0.611.g1030f0b940-goog