Re: [PATCH] PCI: Lock upstream bridge for pci_try_reset_function()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 13 Jan 2026, Anthony Pighin (Nokia) wrote:

> Address this issue:
> [  125.942583] pcieport 0000:00:00.0: unlocked secondary bus reset via:
>                pci_reset_bus_function+0x188/0x1b8

Timestamp is unnecessary for understanding this issue and can be removed.

> which flows from a VFIO_GROUP_GET_DEVICE_FD ioctl when a PCI device is
> being added to a VFIO group.
> 
> 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. That was in response to
> commit 7e89efc6e9e4 ("Lock upstream bridge for pci_reset_function()")
> such that remaining paths would be made more visible.

It would be more logical to start the entire changelog with something 
like:

The commit 7e89efc6e9e4 ("Lock upstream bridge for pci_reset_function()") 
added locking of the upstream bridge the 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.

And only then explain how it triggers in this particular case + quote the 
warning.

> Address the pci_try_reset_function() path.

"Address" is very vague. Please state explicitly that you add missing 
bridge locking and add a Fixes tag.

> Signed-off-by: Anthony Pighin <anthony.pighin@nokia.com>
> ---
>  drivers/pci/pci.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 13dbb405dc31..ff3f2df7e9c8 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5196,19 +5196,34 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
>   */
>  int pci_try_reset_function(struct pci_dev *dev)
>  {
> +	struct pci_dev *bridge;
>  	int rc;
>  
>  	if (!pci_reset_supported(dev))
>  		return -ENOTTY;
>  
> -	if (!pci_dev_trylock(dev))
> +	/*
> +	 * If there's no upstream bridge, no locking is needed since there is
> +	 * no upstream bridge configuration to hold consistent.
> +	 */
> +	bridge = pci_upstream_bridge(dev);
> +	if (bridge && !pci_dev_trylock(bridge))
>  		return -EAGAIN;
>  
> +	if (!pci_dev_trylock(dev)) {
> +		rc = -EAGAIN;
> +		goto out_unlock_bridge;
> +	}
> +
>  	pci_dev_save_and_disable(dev);
>  	rc = __pci_reset_function_locked(dev);
>  	pci_dev_restore(dev);
>  	pci_dev_unlock(dev);
>  
> +out_unlock_bridge:
> +	if (bridge)
> +		pci_dev_unlock(bridge);
> +
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(pci_try_reset_function);
> -- 
> 2.43.0
> 

-- 
 i.