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

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

[Anthony Pighin (Nokia)]

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

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.

Address the pci_try_reset_function() path.

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

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

[Alex Williamson]

On Tue, 13 Jan 2026 21:21:25 +0000
"Anthony Pighin (Nokia)" <anthony.pighin@nokia.com> wrote:

> Address this issue:
> [  125.942583] pcieport 0000:00:00.0: unlocked secondary bus reset via:
>                pci_reset_bus_function+0x188/0x1b8
> 
> 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.
> 
> Address the pci_try_reset_function() path.
> 

Fixes: 7e89efc6e9e4 ("PCI: Lock upstream bridge for pci_reset_function()")

> Signed-off-by: Anthony Pighin <anthony.pighin@nokia.com>
> ---
>  drivers/pci/pci.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)


Reviewed-by: Alex Williamson <alex.williamson@nvidia.com>


> 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);

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

[Ilpo Järvinen]

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.