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

On 15/09/2026 4:51, Jakub Kicinski wrote:

External email: Use caution opening links or attachments


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.

quoted

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.

Not related to this patch, will send in separated patches

quoted

@@ -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);
       }
  }