Re: [PATCH net 4/4] net/mlx5e: Keep netdev when leave switchdev for devlink set legacy only

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

On 12/20/24 09:15, Tariq Toukan wrote:
> From: Jianbo Liu <jianbol@nvidia.com>
> 
> In the cited commit, when changing from switchdev to legacy mode,
> uplink representor's netdev is kept, and its profile is replaced with
> nic profile, so netdev is detached from old profile, then attach to
> new profile.
> 
> During profile change, the hardware resources allocated by the old
> profile will be cleaned up. However, the cleanup is relying on the
> related kernel modules. And they may need to flush themselves first,
> which is triggered by netdev events, for example, NETDEV_UNREGISTER.
> However, netdev is kept, or netdev_register is called after the
> cleanup, which may cause troubles because the resources are still
> referred by kernel modules.
> 
> The same process applies to all the caes when uplink is leaving

case

> switchdev mode, including devlink eswitch mode set legacy, driver
> unload and devlink reload. For the first one, it can be blocked and
> returns failure to users, whenever possible. But it's hard for the
> others. Besides, the attachment to nic profile is unnecessary as the
> netdev will be unregistered anyway for such cases.
> 
> So in this patch, the original behavior is kept only for devlink
> eswitch set mode legacy. For the others, moves netdev unregistration
> before the profile change.
> 
> Fixes: 7a9fb35e8c3a ("net/mlx5e: Do not reload ethernet ports when changing eswitch mode")
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>   .../net/ethernet/mellanox/mlx5/core/en_main.c | 19 +++++++++++++++++--
>   .../net/ethernet/mellanox/mlx5/core/en_rep.c  | 15 +++++++++++++++
>   .../mellanox/mlx5/core/eswitch_offloads.c     |  2 ++
>   include/linux/mlx5/driver.h                   |  1 +
>   4 files changed, 35 insertions(+), 2 deletions(-)
> 

sorry for nitpick-only review
I didn't spotted anything bad in the logic through the series OTOH

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index dd16d73000c3..0ec17c276bdd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6542,8 +6542,23 @@ static void _mlx5e_remove(struct auxiliary_device *adev)
>   
>   	mlx5_core_uplink_netdev_set(mdev, NULL);
>   	mlx5e_dcbnl_delete_app(priv);
> -	unregister_netdev(priv->netdev);
> -	_mlx5e_suspend(adev, false);
> +	/* When unload driver, the netdev is in registered state

/*
  * Netdev dropped the special comment allowance rule,
  * now you have to put one line almost blank at the front.
  */

> +	 * if it's from legacy mode. If from switchdev mode, it
> +	 * is already unregistered before changing to NIC profile.
> +	 */
> +	if (priv->netdev->reg_state == NETREG_REGISTERED) {
> +		unregister_netdev(priv->netdev);
> +		_mlx5e_suspend(adev, false);
> +	} else {
> +		struct mlx5_core_dev *pos;
> +		int i;
> +
> +		if (test_bit(MLX5E_STATE_DESTROYING, &priv->state))

you have more than one statement/expression inside the if,
so you must wrap with braces

> +			mlx5_sd_for_each_dev(i, mdev, pos)
> +				mlx5e_destroy_mdev_resources(pos);
> +		else
> +			_mlx5e_suspend(adev, true);
> +	}
>   	/* Avoid cleanup if profile rollback failed. */
>   	if (priv->profile)
>   		priv->profile->cleanup(priv);