Re: [PATCH net-next 3/3] mlxsw: pci: Lock configuration space of upstream bridge during reset

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 7/1/24 18:41, Petr Machata wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> The driver triggers a "Secondary Bus Reset" (SBR) by calling
> __pci_reset_function_locked() which asserts the SBR bit in the "Bridge
> Control Register" in the configuration space of the upstream bridge for
> 2ms. This is done without locking the configuration space of the
> upstream bridge port, allowing user space to access it concurrently.

This means your patch is a bugfix.

> 
> Linux 6.11 will start warning about such unlocked resets [1][2]:
> 
> pcieport 0000:00:01.0: unlocked secondary bus reset via: pci_reset_bus_function+0x51c/0x6a0
> 
> Avoid the warning by locking the configuration space of the upstream
> bridge prior to the reset and unlocking it afterwards.

You are not avoiding the warning but protecting concurrent access,
please add a Fixes tag.

> 
> [1] https://lore.kernel.org/all/171711746953.1628941.4692125082286867825.stgit@dwillia2-xfh.jf.intel.com/
> [2] https://lore.kernel.org/all/20240531213150.GA610983@bhelgaas/
> 
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlxsw/pci.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> index 0320dabd1380..060e5b939211 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
> @@ -1784,6 +1784,7 @@ static int mlxsw_pci_reset_at_pci_disable(struct mlxsw_pci *mlxsw_pci,
>   {
>   	struct pci_dev *pdev = mlxsw_pci->pdev;
>   	char mrsr_pl[MLXSW_REG_MRSR_LEN];
> +	struct pci_dev *bridge;
>   	int err;
>   
>   	if (!pci_reset_sbr_supported) {
> @@ -1800,6 +1801,9 @@ static int mlxsw_pci_reset_at_pci_disable(struct mlxsw_pci *mlxsw_pci,
>   sbr:
>   	device_lock_assert(&pdev->dev);
>   
> +	bridge = pci_upstream_bridge(pdev);
> +	if (bridge)
> +		pci_cfg_access_lock(bridge);
>   	pci_cfg_access_lock(pdev);
>   	pci_save_state(pdev);
>   
> @@ -1809,6 +1813,8 @@ static int mlxsw_pci_reset_at_pci_disable(struct mlxsw_pci *mlxsw_pci,
>   
>   	pci_restore_state(pdev);
>   	pci_cfg_access_unlock(pdev);
> +	if (bridge)
> +		pci_cfg_access_unlock(bridge);
>   
>   	return err;
>   }