* Re: [PATCH net] net/mlx5: Lag, only cache max_tx_speed that FW has not accepted
2026-10-04 7:02 [PATCH net] net/mlx5: Lag, only cache max_tx_speed that FW has not accepted Tariq Toukan
@ 2026-10-04 7:08 ` netdev-bot+sinfo
2026-10-05 7:42 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sinfo @ 2026-10-04 7:08 UTC (permalink / raw)
To: Tariq Toukan
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
netdev, Paolo Abeni, Gal Pressman, Leon Romanovsky, open list,
linux-rdma, Mark Bloch, Or Har-Toov, Saeed Mahameed, Shay Drory
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] net/mlx5: Lag, only cache max_tx_speed that FW has not accepted
2026-10-04 7:02 [PATCH net] net/mlx5: Lag, only cache max_tx_speed that FW has not accepted Tariq Toukan
2026-10-04 7:08 ` netdev-bot+sinfo
@ 2026-10-05 7:42 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-05 7:42 UTC (permalink / raw)
To: tariqt
Cc: andrew+netdev, davem, edumazet, kuba, netdev, pabeni, gal, leon,
linux-kernel, linux-rdma, mbloch, ohartoov, saeedm, shayd
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
^ permalink raw reply [flat|nested] 3+ messages in thread