Re: [PATCH net-next V2 02/13] net/mlx5: Detach auxiliary devices on attach failure during load

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/mlx5: pass suspend=false to mlx5_detach_device() on
  err_attach path
--

commit c641732ab192276884adbc4d8e49ff0e43bff599
Author: Dragos Tatulea <dtatulea@nvidia.com>

net/mlx5: Detach auxiliary devices on attach failure during load

This commit calls mlx5_detach_device() on the error path in
mlx5_load_one_devl_locked() when mlx5_attach_device() fails.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 1a45474ce03db..7e762f72b19ef 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -1547,6 +1547,7 @@ int mlx5_load_one_devl_locked(struct mlx5_core_dev *dev, bool recovery)
>  	return 0;
>  
>  err_attach:
> +	mlx5_detach_device(dev, true);
>  	clear_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state);
>  	mlx5_unload(dev);
>  err_load:

[Severity: High]
Should this pass suspend = false instead of true to mlx5_detach_device()?

When suspend is true, mlx5_detach_device() invokes adrv->suspend() and
skips del_adev() for auxiliary drivers that implement .suspend (eth and
dpll), leaving those auxiliary devices registered while mlx5_unload() and
mlx5_function_teardown() tear down the core device:

drivers/net/ethernet/mellanox/mlx5/core/dev.c:mlx5_detach_device() {
    ...
		if (adrv->suspend && suspend) {
			adrv->suspend(adev, pm);
			continue;
		}

skip_suspend:
		del_adev(&priv->adev[i]->adev);
		priv->adev[i] = NULL;
    ...
}

For dpll, mlx5_dpll_suspend() is a no-op, so its periodic delayed work
mdpll->work remains active and continues issuing firmware commands against
the torn-down core device.  For eth, suspending instead of deleting leaves
the netdev registered with MLX5E_STATE_DESTROYING set after destroying
mdev_resources, which causes a later _mlx5e_remove() -> _mlx5e_suspend()
to destroy mdev_resources a second time, and on a Socket-Direct secondary
mlx5e_suspend() clears mdev->sd so a subsequent mlx5e_remove()
dereferences NULL drvdata on the secondary adev.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110183826.2927274-1-tariqt@nvidia.com?part=2