Re: [PATCH net] net/mlx5: Lag, only cache max_tx_speed that FW has not accepted

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] This patch breaks the query-failure fallback added by commit
  ad0ae7aefa7a ("net/mlx5: E-Switch, preserve max tx speed on vport…
- [Medium] The commit message does not match the code change.

--

net/mlx5: Lag, only cache max_tx_speed that FW has not accepted

This patch changes mlx5_lag_modify_device_vports_speed() so that
vport->agg_max_tx_speed is written only when the vport is disabled or the
FW push fails. When the push succeeds, the cache is cleared to 0. The
enable-time replay in mlx5_esw_vport_set_max_tx_speed() also clears the
cache on success.

Link: https://lore.kernel.org/all/20260915015118.875210-1-kuba@kernel.org/

> mlx5_lag_modify_device_vports_speed() also wrote it for enabled vports,
> unconditionally, before even attempting the FW push. If that push
> failed, the cache still claimed speed had been applied, even though FW
> might still hold the old value. That unconfirmed value could then leak
> out: mlx5_esw_vport_enable() replays it if the vport is later disabled
> and re-enabled with no LAG update in between.

[Severity: Medium]
Does the change actually address the scenario described here?

After this patch, the failure branch in
mlx5_lag_modify_device_vports_speed() still caches the speed:

		if (ret) {
			mlx5_core_dbg(mdev, ...);
			vport->agg_max_tx_speed = speed;

and mlx5_esw_vport_enable() still replays any nonzero value:

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

So a failed push followed by a disable and re-enable, with no LAG update
in between, behaves the same before and after the patch. The third
paragraph of the message keeps this on purpose.

> Clear it once FW has accepted the value, so a superseded speed is not
> replayed over a newer one.

Which path could program a newer speed that a stale cache would then
overwrite? Only two places write agg_max_tx_speed, and they are also the
only callers of mlx5_modify_vport_max_tx_speed(). One is the LAG loop,
which always overwrites the cache with the latest LAG speed. The other is
the enable-time replay.

The only real change in behavior seems to be clearing the cache on
success. The message doesn't mention two side effects of that:

- The query-failure fallback added by ad0ae7aefa7a ("net/mlx5: E-Switch,
  preserve max tx speed on vport state modification") stops working (see
  below).
- Vports that were programmed successfully no longer get a replay on
  re-enable.

Because of the Fixes: c6df9a65cbb0 tag, this change would go to stable
trees even though the bug described above can still happen. Could the
commit message and the Fixes tag be revisited?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index fc197d1dc9dfe..f8c220bf7b5ca 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -951,6 +951,8 @@ static void mlx5_esw_vport_set_max_tx_speed(struct mlx5_eswitch *esw,
>  		mlx5_core_dbg(esw->dev,
>  			      "Failed to set vport %d speed %d, err=%d\n",
>  			      vport->vport, vport->agg_max_tx_speed, ret);
> +	else
> +		vport->agg_max_tx_speed = 0;
>  }
>  
>  int mlx5_esw_vport_enable(struct mlx5_eswitch *esw, struct mlx5_vport *vport,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 3b34bec559e08..7cee78cb47df9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1502,17 +1502,21 @@ static void mlx5_lag_modify_device_vports_speed(struct mlx5_core_dev *mdev,
>  		if (vport->vport == MLX5_VPORT_UPLINK)
>  			continue;
>  
> -		vport->agg_max_tx_speed = speed;
> -
> -		if (!vport->enabled)
> +		if (!vport->enabled) {
> +			vport->agg_max_tx_speed = speed;
>  			continue;
> +		}
>  
>  		ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod,
>  						     vport->vport, true, speed);
> -		if (ret)
> +		if (ret) {
>  			mlx5_core_dbg(mdev,
>  				      "Failed to set vport %d speed %d, err=%d\n",
>  				      vport->vport, speed, ret);
> +			vport->agg_max_tx_speed = speed;
> +		} else {
> +			vport->agg_max_tx_speed = 0;
> +		}

[Severity: Medium]
Can clearing the cache here break the query-failure fallback in
mlx5_modify_vport_admin_state()?

That function, and mlx5_esw_adj_vport_modify() in esw/adj_vport.c, use
agg_max_tx_speed as the last known speed when QUERY_VPORT_STATE fails:

drivers/net/ethernet/mellanox/mlx5/core/vport.c:mlx5_modify_vport_admin_state() {
    ...
		err = mlx5_query_vport_max_tx_speed(mdev, opmod, vport,
						    other_vport, &speed, NULL);
		if (err) {
			...
			speed = IS_ERR(esw_vport) ? 0 :
				esw_vport->agg_max_tx_speed;
			...
		}
		MLX5_SET(modify_vport_state_in, in, max_tx_speed, speed);
    ...
}

With this patch, agg_max_tx_speed becomes 0 on every vport whose LAG speed
was pushed successfully, either here or in the replay in
mlx5_esw_vport_set_max_tx_speed().

Suppose a later admin state change on that vport hits a query failure.
Wouldn't the fallback then send max_tx_speed = 0, which ad0ae7aefa7a
describes as an intentional reset in FW? That would wipe the LAG aggregate
speed that was just programmed:

mlx5_lag_modify_device_vports_speed()
  mlx5_modify_vport_max_tx_speed()      /* succeeds */
  vport->agg_max_tx_speed = 0
...
mlx5_modify_vport_admin_state()
  mlx5_query_vport_max_tx_speed()       /* fails */
  speed = esw_vport->agg_max_tx_speed   /* 0 */
  MLX5_SET(modify_vport_state_in, in, max_tx_speed, speed)

These are the admin state changes that can reach this path:

- mlx5e_rep_open(), mlx5e_rep_close() and mlx5e_rep_change_carrier()
- mlx5_eswitch_set_vport_state()
- esw_vport_setup() and esw_vport_cleanup()

Adjacent vport connect and disconnect through mlx5_esw_adj_vport_modify()
are affected the same way. After that, nothing pushes the speed again
until the next LAG speed event.

Before this patch, the cache still held the programmed speed after a
successful push, so the fallback rewrote the correct value.

The patch now treats agg_max_tx_speed as a pending value, but both
fallback readers still treat it as the last programmed speed. Can one
field serve both meanings?

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