Re: [PATCH v2] vfio/pci: Lock upstream bridge for vfio_pci_core_disable()

[Alex Williamson <alex@shazbot.org>]

On Wed, 21 Jan 2026 12:20:22 -0800
<dan.j.williams@intel.com> wrote:

> Anthony Pighin (Nokia) wrote:
> > The commit 7e89efc6e9e4 ("Lock upstream bridge for pci_reset_function()")
> > added locking of the upstream bridge to the reset function. To catch
> > paths that are not properly locked, the commit 920f6468924f ("Warn on
> > missing cfg_access_lock during secondary bus reset") added a warning
> > if the PCI configuration space was not locked during a secondary bus reset
> > request.
> > 
> > When a VFIO PCI device is released from userspace ownership, an attempt
> > to reset the PCI device function may be made. If so, and the upstream bridge
> > is not locked, the release request results in a warning:
> > 
> >    pcieport 0000:00:00.0: unlocked secondary bus reset via:
> >    pci_reset_bus_function+0x188/0x1b8
> > 
> > Add missing upstream bridge locking to vfio_pci_core_disable().
> > 
> > Fixes: 7e89efc6e9e4 ("PCI: Lock upstream bridge for pci_reset_function()")
> > Signed-off-by: Anthony Pighin <anthony.pighin@nokia.com>
> > ---
> > V1 -> V2:
> >   - Reworked commit log for clarity
> >   - Corrected indentation
> >   - Added a Fixes: tag
> > 
> > 
> >  drivers/vfio/pci/vfio_pci_core.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index 3a11e6f450f7..72c33b399800 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -588,6 +588,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_enable);
> >  
> >  void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
> >  {
> > +	struct pci_dev *bridge;
> >  	struct pci_dev *pdev = vdev->pdev;
> >  	struct vfio_pci_dummy_resource *dummy_res, *tmp;
> >  	struct vfio_pci_ioeventfd *ioeventfd, *ioeventfd_tmp;
> > @@ -694,12 +695,20 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
> >  	 * We can not use the "try" reset interface here, which will
> >  	 * overwrite the previously restored configuration information.
> >  	 */
> > -	if (vdev->reset_works && pci_dev_trylock(pdev)) {
> > -		if (!__pci_reset_function_locked(pdev))
> > -			vdev->needs_reset = false;
> > -		pci_dev_unlock(pdev);
> > +	if (vdev->reset_works) {
> > +		bridge = pci_upstream_bridge(pdev);
> > +		if (bridge && !pci_dev_trylock(bridge))
> > +			goto out_restore_state;
> > +		if (pci_dev_trylock(pdev)) {
> > +			if (!__pci_reset_function_locked(pdev))
> > +				vdev->needs_reset = false;
> > +			pci_dev_unlock(pdev);
> > +		}
> > +		if (bridge)
> > +			pci_dev_unlock(bridge);  
> 
> This looks ok, but a bit unfortunate that it duplicates what
> mlxsw_pci_reset_at_pci_disable() is also open coding. It leaves Octeon
> (orphaned) and Xen to rediscover the same bug. At a minimum I copied the Xen
> folks for their awareness, but it feels like __pci_reset_function_locked()
> really is no longer suitable to be exported to drivers with this new locking
> requirement. It wants a wrapper that contains this detail.

Even pci_try_reset_function() was missed when when this new locking
requirement was added, so adding yet another core wrapper function
doesn't guarantee we won't hit such problems.

The device_lock requirement for PCI reset has been particularly
problematic over the years, leading to the trylock nonsense we use to
avoid deadlocks.  Even the previously converted mlxsw call path cannot
acquire the bridge device lock since they already expect to hold the
reset target device lock, deferring to only acquiring the
pci_cfg_access_lock().

The above vfio-pci code may ultimately choose something similar because
even when userspace has correctly shutdown the VM before unbinding the
device from vfio-pci, the fput is run through a workqueue and may not
be able to acquire device lock here.

I don't know what a wrapper to handle all these variant would look
like.  Maybe we need to bubble up the warning to the existing wrapper
so we can see it without having specific hardware that triggers the bus
reset path.  Thanks,

Alex