Re: [PATCH net V2 4/4] net/mlx5: Lag, save and restore vport max_tx_speed across LAG

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

On 26/10/2026 22:32, netdev-bot+sashiko@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> Thank you for your contribution! Sashiko AI review found 7 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 5 · Low: 1
> 
> 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).
> 
> - [High] mlx5_esw_vport_save_max_tx_speed() (eswitch.c:948) treats a non-
>    zero agg_max_tx_speed during an active LAG as meaning the aggregate is…
> - [Medium] The new restore logic only runs on one LAG teardown path.
> - [Medium] vport->max_tx_speed is read from FW only once per mlx5_vport
>    object, because of the `|| vport->max_tx_speed) return;` early return…
> - [Medium] In mlx5_lag_reset_vports_speed(), when
>    mlx5_modify_vport_max_tx_speed() fails for an enabled vport, the loop…
> - [Medium] mlx5_lag_reset_vports_speed() records agg_max_tx_speed = 0 in
>    two cases where FW still holds the LAG aggregate.
> - [Medium] The save gate in mlx5_esw_vport_save_max_tx_speed() uses
>    mlx5_lag_is_active(), which reads ldev->mode under lag_lock.
> - [Low] The fallback in mlx5_lag_get_restore_speed() clamps with the raw
>    per-device pcie_bandwidth_mbps(mdev->pdev).
> 
> --
> 
> net/mlx5: Lag, save and restore vport max_tx_speed across LAG
> 
> This commit reads each vport's max_tx_speed from FW when the vport is
> enabled. At LAG teardown it writes that saved value back per vport,
> instead of writing the oper link speed. If nothing was saved, it falls
> back to a link-derived speed clamped by PCIe bandwidth, and disabled
> vports get their saved value as agg_max_tx_speed for the next enable.
> 
> Link: https://lore.kernel.org/all/20260915015118.875210-1-kuba@kernel.org/
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> index fc197d1dc9dfe..7cf9c15ec65d2 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> @@ -935,6 +935,27 @@ static void esw_vport_cleanup(struct mlx5_eswitch *esw, struct mlx5_vport *vport
>>        esw_vport_cleanup_acl(esw, vport);
>>   }
>>
>> +static void mlx5_esw_vport_save_max_tx_speed(struct mlx5_eswitch *esw,
>> +                                          struct mlx5_vport *vport)
>> +{
>> +     u16 op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
>> +     int ret;
>> +
>> +     if (!MLX5_CAP_ESW(esw->dev, esw_vport_state_max_tx_speed) ||
>> +         vport->vport == MLX5_VPORT_UPLINK || vport->max_tx_speed)
>> +             return;
> 
> [Severity: Medium]
> Once vport->max_tx_speed is non-zero, this returns early. Nothing in the
> patch ever sets it back to 0. Can the saved value go stale?
> 
> The mlx5_vport objects are allocated once, in mlx5_esw_vports_init()
> from mlx5_init_once(). They survive devlink reload, fw_activate, sync
> reset and health recovery, because mlx5_load_one_devl_locked() only
> runs mlx5_function_setup() and mlx5_load().
> 
> After such a reset, FW may recompute its per-vport default (the lower
> of port speed and PCIe bandwidth). If the PCIe link or port speed has
> changed, mlx5_lag_get_restore_speed() still uses the old saved value at
> the next teardown.
> 
> For disabled vports, the old value is copied into agg_max_tx_speed.
> Since that field is never cleared, mlx5_esw_vport_enable() writes it on
> every later enable, even when no LAG is configured.
> 
> Should max_tx_speed be reset on reload/reset, or read again from FW
> when no LAG is active?
> 
>> +
>> +     if (mlx5_lag_is_active(esw->dev) && !vport->agg_max_tx_speed)
>> +             return;
> 
> [Severity: High]
> This check reads a non-zero agg_max_tx_speed during an active LAG as
> "the aggregate is pending and FW still holds the original". Is that
> always the case?
> 
> mlx5_lag_modify_device_vports_speed() sets agg_max_tx_speed for every
> vport. For enabled vports it also writes the aggregate to FW right away:
> 
>      vport->agg_max_tx_speed = speed;
> 
>      if (!vport->enabled)
>          continue;
> 
>      ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod, vport->vport, true, speed);
> 
> Nothing clears agg_max_tx_speed once it has been applied.
> mlx5_esw_vport_disable() doesn't touch agg_max_tx_speed or
> max_tx_speed either.
> 
> So take a vport with max_tx_speed == 0 while FW already holds the
> aggregate. That state is reachable in two ways:
> 
> - An earlier save query failed (only logged at debug level).
> - The vport was enabled while mlx5_lag_is_active() was true and
>    agg_max_tx_speed was 0, so the save was skipped. One example is an
>    SD-only shared FDB: __mlx5_lag_is_sd_active() makes
>    mlx5_lag_is_active() return true, but no aggregate is applied. A
>    later aggregate, for example from MPESW enable, then lands on the
>    enabled vport.
> 
> If that vport is disabled and re-enabled while the LAG is still active,
> this check passes. The query then stores the FW aggregate in
> vport->max_tx_speed.
> 
> At teardown, mlx5_lag_get_restore_speed() returns that value and the
> aggregate is written back. Because max_tx_speed is never reset, every
> later LAG cycle restores the same wrong baseline.
> 
> That seems to contradict the commit message:
> 
>      If the max_tx_speed of a vport could not be saved, it stays zero and
>      mlx5_lag_get_restore_speed() falls back to a link-derived speed at
>      teardown.
> 
> Is a separate marker needed here, one that can tell "aggregate pending"
> apart from "aggregate applied"?
> 
> [Severity: Medium]
> Does mlx5_lag_is_active() stay consistent with agg_max_tx_speed here?
> 
> mlx5_lag_is_active() reads ldev->mode under lag_lock. ldev->mode is
> written under ldev->lock only, in mlx5_activate_lag() and
> mlx5_deactivate_lag(). agg_max_tx_speed and the FW aggregate are
> written in a separate esw->state_lock section.
> 
> On activation, mlx5_do_bond()->mlx5_lag_shared_fdb_create() sets the
> mode and drops ldev->lock for rep rescans and reloads. Only after that
> does it call mlx5_lag_set_vports_agg_speed().
> 
> A vport can be enabled in that window. One way is mlx5_eswitch_enable()
> when the FDB already exists: toggle_lag is false, so
> mlx5_lag_disable_change() doesn't serialize it. SF/ECPF function
> changes are another way. Such a vport sees an active LAG with
> agg_max_tx_speed == 0. It skips the save even though FW still holds the
> original, and then receives the aggregate.
> 
> On teardown, mlx5_do_bond() does:
> 
>      mlx5_lag_reset_vports_speed(ldev);
>      mlx5_disable_lag(ldev);
> 
> The mode is still set after mlx5_lag_reset_vports_speed() drops
> state_lock. A vport enabled in that window also skips the save.
> 
> Either way, the vport ends up with max_tx_speed == 0 while FW holds the
> aggregate. Isn't that the precondition for the saved-baseline problem
> described above?
> 
>> +
>> +     ret = mlx5_query_vport_max_tx_speed(esw->dev, op_mod, vport->vport,
>> +                                         true, &vport->max_tx_speed, NULL);
>> +     if (ret)
>> +             mlx5_core_dbg(esw->dev,
>> +                           "Failed to save vport %d max tx speed, err=%d\n",
>> +                           vport->vport, ret);
>> +}
> 
> [ ... ]
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>> index afe0b9909f2dc..3e623c96b2a1d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>> @@ -1712,11 +1712,37 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)
>>        }
>>   }
>>
>> +static int mlx5_lag_get_restore_speed(struct mlx5_core_dev *mdev,
>> +                                   struct mlx5_vport *vport, u32 *speed)
>> +{
>> +     u32 pci_bw;
>> +
>> +     if (vport->max_tx_speed) {
>> +             *speed = vport->max_tx_speed;
>> +             return 0;
>> +     }
>> +
>> +     *speed = 0;
>> +     mlx5_port_oper_linkspeed(mdev, speed);
>> +     if (!*speed && mlx5_port_max_linkspeed(mdev, speed))
>> +             return -ENODATA;
>> +
>> +     pci_bw = pcie_bandwidth_mbps(mdev->pdev);
>> +     if (pci_bw)
>> +             *speed = min(*speed, pci_bw);
> 
> [Severity: Low]
> This might not be an issue, but the fallback clamps with the per-device
> pcie_bandwidth_mbps(mdev->pdev). The aggregate path uses
> mlx5_lag_port_pcie_bw() instead, which sums bandwidth across
> Socket-Direct group members and treats RC_END as unlimited.
> 
> The restore value is per vport, and the FW default is described as
> coming from the endpoint's own link status. So the per-device clamp may
> be intended. For RC_END, pcie_bandwidth_mbps() returns 0 and the
> if (pci_bw) check skips the clamp.
> 
> Could a short comment explain why the per-device value is used here
> rather than the SD-aware helper?
> 
>> +     *speed /= MLX5_MAX_TX_SPEED_UNIT;
>> +     return 0;
>> +}
> 
> [ ... ]
> 
>> @@ -1728,17 +1754,39 @@ void mlx5_lag_reset_vports_speed(struct mlx5_lag *ldev)
>>                mdev = pf->dev;
>>                if (!mdev)
>>                        continue;
>> -
>> -             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);
>> +             esw = mdev->priv.eswitch;
>> +             if (!esw || !MLX5_CAP_ESW(mdev, esw_vport_state_max_tx_speed))
>>                        continue;
>> -             }
>>
>> -             speed = speed / MLX5_MAX_TX_SPEED_UNIT;
>> -             mlx5_lag_modify_device_vports_speed(mdev, speed);
>> +             mutex_lock(&esw->state_lock);
>> +             mlx5_esw_for_each_vport(esw, i, vport) {
> 
> [Severity: Medium]
> mlx5_lag_reset_vports_speed() is only called from the
> mlx5_lag_should_disable_lag() branch of mlx5_do_bond(). That predicate
> excludes MPESW:
> 
>      return !do_bond && __mlx5_lag_is_active(ldev) &&
>             ldev->mode != MLX5_LAG_MODE_MPESW;
> 
> Should the other teardown paths restore vport speeds as well?
> 
> mlx5_lag_disable_change() goes straight to mlx5_lag_disable_mpesw() or
> mlx5_disable_lag(). It is called from mlx5_eswitch_enable(),
> mlx5_eswitch_disable() and the devlink eswitch mode change.
> 
> Explicit MPESW disable goes through
> mlx5_mpesw_work()->mlx5_lag_disable_mpesw()->mlx5_lag_shared_fdb_destroy(),
> and doesn't restore either.
> 
> The aggregate is still applied when these LAGs come up, through
> mlx5_lag_shared_fdb_create()->mlx5_lag_set_vports_agg_speed() and
> mlx5_mpesw_speed_update_work().
> 
> After those teardowns, FW and agg_max_tx_speed both still hold the
> aggregate. mlx5_esw_vport_enable() writes it again on every later
> enable.
> 
> With the new save step, a vport whose max_tx_speed is still 0 then
> reads the leftover aggregate from FW and keeps it as its saved
> original. Every later mlx5_do_bond() teardown then restores the
> aggregate.
> 
> The missing restore calls on these paths were already there before
> this patch. But the commit message says the saved value is restored
> "at teardown", and the interaction with the new save step is new.

Will be fixed in a different patch that will be added to the V3 of this 
series (calling mlx5_lag_reset_vports_speed in MPESW teardown.

> 
>> +                     if (!vport || vport->vport == MLX5_VPORT_UPLINK)
>> +                             continue;
>> +
>> +                     if (!vport->enabled) {
>> +                             vport->agg_max_tx_speed = vport->max_tx_speed;
>> +                             continue;
>> +                     }
>> +
>> +                     if (mlx5_lag_get_restore_speed(mdev, vport,
>> +                                                    &restore_speed)) {
>> +                             vport->agg_max_tx_speed = 0;
>> +                             continue;
>> +                     }
> 
> [Severity: Medium]
> Both of these branches set agg_max_tx_speed to 0 without writing FW.
> Could FW still hold the aggregate in either case?
> 
> For a disabled vport with max_tx_speed == 0, the pending restore is
> dropped. That vport may have been enabled partway through the LAG
> without a saved value (the mlx5_lag_is_active() check skipped the save,
> or the query failed). It then received the aggregate and was disabled
> before teardown.
> 
> The patch already assumes FW keeps max_tx_speed across a vport disable.
> That is why disabled vports get a replay value.
> 
> For an enabled vport, mlx5_lag_get_restore_speed() returns -ENODATA
> when nothing was saved and the PTYS query fails.
> 
> In both cases, the next mlx5_esw_vport_enable() after the LAG is gone
> sees max_tx_speed == 0 and an inactive LAG. It queries FW and saves the
> leftover aggregate as the original.
> 
> Before this patch, disabled vports got agg_max_tx_speed set to the oper
> speed, so re-enabling them corrected FW.
> 
> The commit message covers "a vport disabled for the entire LAG period".
> Does the same reasoning hold for a vport that was disabled partway
> through the LAG?
> 
>> +
>> +                     ret = mlx5_modify_vport_max_tx_speed(mdev, op_mod,
>> +                                                          vport->vport,
>> +                                                          true,
>> +                                                          restore_speed);
>> +                     if (ret) {
>> +                             mlx5_core_dbg(mdev,
>> +                                           "Failed to restore vport %d max tx speed, err=%d\n",
>> +                                           vport->vport, ret);
>> +                             continue;
>> +                     }
> 
> [Severity: Medium]
> If mlx5_modify_vport_max_tx_speed() fails here, agg_max_tx_speed keeps
> the LAG aggregate. Is that intended?
> 
> Before, mlx5_lag_modify_device_vports_speed() set agg_max_tx_speed to
> the teardown speed before trying the FW write. A later enable would
> then retry with a non-LAG value.
> 
> Now, on a later enable with no LAG active, mlx5_esw_vport_enable()
> runs:
> 
>      if (vport->agg_max_tx_speed)
>          mlx5_esw_vport_set_max_tx_speed(esw, vport);
> 
> which writes the stale aggregate to FW again. If max_tx_speed is still
> 0, the save step runs first and records the aggregate still in FW as
> the baseline.
> 
> That looks like the state the commit message says is fixed:
> 
>      leaving the aggregate in FW and in agg_max_tx_speed
> 
> Would setting vport->agg_max_tx_speed = restore_speed in this failure
> branch let the next enable retry the restore?
> 
>> +                     vport->agg_max_tx_speed = 0;
>> +             }
>> +             mutex_unlock(&esw->state_lock);
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022193053.3815866-1-tariqt%40nvidia.com