Re: [PATCH net V2] net/mlx5: Lag, reset vport speed on MPESW teardown

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The fix says it removes stale cached and FW-configured vport
  speeds after MPESW teardown, and it relies on…

--

net/mlx5: Lag, reset vport speed on MPESW teardown

This adds a call to mlx5_lag_reset_vports_speed() in
mlx5_lag_disable_mpesw(). Regular LAG teardown in mlx5_do_bond() already
makes this call, and the MPESW path now matches it. The goal is to stop
cached and FW-configured vport speeds from going stale after MPESW
teardown.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> index abf72026c7513..e0a57b7dd82ed 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> @@ -157,6 +157,7 @@ void mlx5_lag_disable_mpesw(struct mlx5_lag *ldev)
>  	if (ldev->mode != MLX5_LAG_MODE_MPESW)
>  		return;
>  
> +	mlx5_lag_reset_vports_speed(ldev);

[Severity: Medium]
The commit message says this addresses "stale cached and FW-configured
speeds after MPESW is torn down". Does mlx5_lag_reset_vports_speed()
actually put the vports back into their pre-LAG unset state?

The helper reads the current uplink oper speed and writes it as a non-zero
value:

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_reset_vports_speed() {
    ...
		ret = mlx5_port_oper_linkspeed(mdev, &speed);
    ...
		speed = speed / MLX5_MAX_TX_SPEED_UNIT;
		mlx5_lag_modify_device_vports_speed(mdev, speed);
}

mlx5_lag_modify_device_vports_speed() then caches that value. It also
programs the value into FW for every enabled non-uplink vport:

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

Elsewhere, 0 looks like the "no override" value. mlx5_esw_vport_enable()
only programs a speed when the cached value is non-zero:

	if (vport->agg_max_tx_speed)
		mlx5_esw_vport_set_max_tx_speed(esw, vport);

mlx5_ib_query_port_speed_from_vport() falls back to the live port speed
only when max_tx_speed is 0:

	if (vport_state == VPORT_STATE_DOWN || max_tx_speed == 0)
		/* Value 0 indicates field not supported, fallback */

After teardown, nothing refreshes this snapshot.
mlx5_mpesw_speed_update_work() only acts when ldev->mode is still
MLX5_LAG_MODE_MPESW.

Say the uplink later renegotiates to a different speed. Would the vports
keep reporting the teardown-time speed, with FW still holding it as
max_tx_speed? VFs created after the LAG is gone would also get the
snapshot through mlx5_esw_vport_enable().

There is also the failure path. If mlx5_port_oper_linkspeed() fails during
the reset, the helper skips that device:

		if (ret) {
			mlx5_core_dbg(mdev, ...);
			continue;
		}

That leaves the full MPESW aggregate in both agg_max_tx_speed and FW for
that device. Isn't that the same stale state this commit is meant to
remove?

Should teardown clear the override back to 0 instead of writing a
snapshot of the current oper speed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006112415.259157-1-tariqt%40nvidia.com