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

[Or Har-Toov <ohartoov@nvidia.com>]

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