Re: [PATCH net 2/5] net/mlx5: Lag, reset vport speed on teardown

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: Lag, reset vport speed on teardown

The cached agg_max_tx_speed was only reset on the regular LAG teardown
path in mlx5_do_bond(), so MPESW teardown left stale values behind. The
reset call is moved into mlx5_deactivate_lag() so all teardown paths run
it.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 5aa9d5c98fe16..c2f21eb4ea7cf 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1075,6 +1075,8 @@ int mlx5_deactivate_lag(struct mlx5_lag *ldev)
>  	if (master_idx < 0)
>  		return -EINVAL;
>  
> +	mlx5_lag_reset_vports_speed(ldev);
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but now that mlx5_deactivate_lag()
is the single place claimed to reset the vport speeds on "every teardown
path", is the helper it calls complete enough for that claim?

mlx5_lag_reset_vports_speed() skips a PF entirely when the oper speed query
fails:

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_reset_vports_speed() {
	ret = mlx5_port_oper_linkspeed(mdev, &speed);
	if (ret) {
		mlx5_core_dbg(mdev,
			      "Failed to reset vports speed for device %s. Oper speed is not available (err=%d)\n",
			      dev_name(mdev->device), ret);
		continue;
	}
}

For that PF, vport->agg_max_tx_speed keeps the LAG aggregate value written
earlier by mlx5_lag_modify_device_vports_speed(), and enabled vports keep
the aggregated max_tx_speed in FW even though the FW LAG object is
destroyed.

The stale value then gets replayed later, in eswitch.c:

mlx5_esw_vport_enable() {
	if (vport->agg_max_tx_speed)
		mlx5_esw_vport_set_max_tx_speed(esw, vport);
}

and is also used as the fallback in vport.c:

mlx5_modify_vport_admin_state() {
	speed = IS_ERR(esw_vport) ? 0 : esw_vport->agg_max_tx_speed;
}

so an inflated reported TX speed can survive with no LAG present. This is
the same best-effort handling that came in with 50f1d188c580, and moving the
call site strictly increases the number of paths that attempt the reset, so
nothing is made worse here.

Both mlx5_lag_reset_vports_speed() and
mlx5_lag_modify_device_vports_speed() return void, so mlx5_deactivate_lag()
cannot observe the skip and still returns 0. Would it make sense to have the
reset path clear the cached agg_max_tx_speed (or fall back to zero) when
mlx5_port_oper_linkspeed() fails, rather than leaving the LAG aggregate
cached?

Also worth noting on the caching order in
mlx5_lag_modify_device_vports_speed():

	vport->agg_max_tx_speed = speed;

	if (!vport->enabled)
		continue;

	ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod, vport->vport, true, speed);
	if (ret)
		mlx5_core_dbg(...);

the cache is updated before the FW command and a FW failure is only logged,
so cache and FW can diverge during teardown too.

> @@ -1676,7 +1678,6 @@ static void mlx5_do_bond(struct mlx5_lag *ldev)
>  		mlx5_modify_lag(ldev, &tracker);
>  		mlx5_lag_set_vports_agg_speed(ldev);
>  	} else if (mlx5_lag_should_disable_lag(ldev, do_bond)) {
> -		mlx5_lag_reset_vports_speed(ldev);
>  		mlx5_disable_lag(ldev);
>  	}
>  }