Hi,
Commit 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to
vports") added a mechanism that computes the aggregated LAG TX speed and
programs it into each vport's max_tx_speed via MODIFY_VPORT_STATE. This
series by Or fixes bugs found in that flow.
The bugs:
1. Down ports were counted into the effective aggregate speed.
When a port goes down, PTYS eth_proto_oper is not necessary 0.
2. The aggregate was not limited by the NIC's PCIe bandwidth. It was
taken from the bond speed, which is calculated from PTYS, and PTYS
is not bounded by the PCI bandwidth of the NIC. FW initializes
max_tx_speed to the minimum of the port speed and the PCIe
bandwidth, so modifying it with a value computed by different rules
changes the meaning of the field rather than updating it.
3. MPESW teardown never reset the vport speeds, leaving stale values
after teardown.
4. RoCE LAG never told the RDMA layer about speed changes. The NIC is
considered to be in a bare metal configuration, without a valid or
enabled eswitch, so there are no VF, SF or PF rep vports to modify
and no event of speed change was raised.
Patch #1 splits the LAG aggregate speed into an operative and a max
helper, so that each can filter on port state on its own.
Patch #2 resets the vport speed on MPESW teardown.
Patch #3 caches the TX-enabled aggregate speed and programs that into
the vports, instead of reading it back from the bond netdev.
Patch #4 limits the aggregated TX speed by the NIC's PCIe bandwidth.
Patch #5 notifies the RDMA layer of RoCE LAG speed changes via a new
driver event.
Regards,
Tariq
Or Har-Toov (5):
net/mlx5: Lag, split aggregate speed into oper and max helpers
net/mlx5: Lag, reset vport speed on teardown
{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports
net/mlx5: Lag, limit aggregated TX speed by PCIe bandwidth
{RDMA,net}/mlx5: notify RoCE LAG speed change via driver event
drivers/infiniband/hw/mlx5/main.c | 12 +-
drivers/net/ethernet/mellanox/mlx5/core/dev.c | 20 +++
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 159 +++++++++++++++---
.../net/ethernet/mellanox/mlx5/core/lag/lag.h | 16 +-
.../ethernet/mellanox/mlx5/core/mlx5_core.h | 1 +
include/linux/mlx5/device.h | 1 +
include/linux/mlx5/driver.h | 1 +
7 files changed, 181 insertions(+), 29 deletions(-)
base-commit: 00f9fbc12320253bfc576fb7539d860029c82d0f
--
2.44.0
From: Or Har-Toov <redacted>
Split mlx5_lag_sum_devices_speed into mlx5_lag_get_devices_oper_speed
and mlx5_lag_get_devices_max_speed. The oper function reflects only
the speed currently available and the max function is state-independent
and represents the maximum achievable speed, taking the best single
port for active-backup and summing all ports for XOR, LACP and MPESW.
The oper function skips a port for two reasons:
- the bonding driver set tx_enabled=false - LACP converging or
active-backup standby slave, where carrier may still be up but the
port is intentionally not forwarding traffic.
- the link is down.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <redacted>
Reviewed-by: Shay Drori <redacted>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 71 ++++++++++++++-----
1 file changed, 53 insertions(+), 18 deletions(-)
@@ -1430,13 +1434,20 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,pf_mdev=pf->dev;if(!pf_mdev)continue;+if(mpesw){+if(mlx5_query_vport_state(pf_mdev,opmod,0)!=+VPORT_STATE_UP)+continue;+}elseif(!ldev->tracker.netdev_state[pf_idx].tx_enabled||+!ldev->tracker.netdev_state[pf_idx].link_up){+continue;+}-ret=get_speed(pf_mdev,&speed);+ret=mlx5_port_oper_linkspeed(pf_mdev,&speed);if(ret){mlx5_core_dbg(pf_mdev,-"Failed to get device speed using %ps. Device %s speed is not available (err=%d)\n",-get_speed,dev_name(pf_mdev->device),-ret);+"Failed to get device %s oper speed (err=%d)\n",+dev_name(pf_mdev->device),ret);returnret;}
@@ -1446,17 +1457,41 @@ mlx5_lag_sum_devices_speed(struct mlx5_lag *ldev, u32 *sum_speed,return0;}-staticintmlx5_lag_sum_devices_max_speed(structmlx5_lag*ldev,u32*max_speed)+staticintmlx5_lag_get_devices_max_speed(structmlx5_lag*ldev,u32*max_speed){-returnmlx5_lag_sum_devices_speed(ldev,max_speed,-mlx5_port_max_linkspeed);-}+structmlx5_core_dev*pf_mdev;+structlag_func*pf;+booltake_max;+intpf_idx;+u32speed;+intret;-staticintmlx5_lag_sum_devices_oper_speed(structmlx5_lag*ldev,-u32*oper_speed)-{-returnmlx5_lag_sum_devices_speed(ldev,oper_speed,-mlx5_port_oper_linkspeed);+take_max=ldev->tracker.tx_type==NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;+if(ldev->mode==MLX5_LAG_MODE_MPESW)+take_max=false;++*max_speed=0;+mlx5_ldev_for_each(pf_idx,0,ldev){+pf=mlx5_lag_pf(ldev,pf_idx);+if(!pf)+continue;+pf_mdev=pf->dev;+if(!pf_mdev)+continue;++ret=mlx5_port_max_linkspeed(pf_mdev,&speed);+if(ret){+mlx5_core_dbg(pf_mdev,+"Failed to get device %s max speed (err=%d)\n",+dev_name(pf_mdev->device),ret);+returnret;+}++*max_speed=take_max?+max(*max_speed,speed):*max_speed+speed;+}++return0;}staticvoidmlx5_lag_modify_device_vports_speed(structmlx5_core_dev*mdev,
@@ -1514,7 +1549,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)}/* If speed is not set, use the sum of max speeds of all PFs */-if(!speed&&mlx5_lag_sum_devices_max_speed(ldev,&speed))+if(!speed&&mlx5_lag_get_devices_max_speed(ldev,&speed))return;speed=speed/MLX5_MAX_TX_SPEED_UNIT;
From: Or Har-Toov <redacted>
mlx5_lag_disable_mpesw() never resets the cached agg_max_tx_speed on
the vports, unlike regular LAG teardown. This leaves stale speed values
cached on struct mlx5_vport, and pushed to FW, after MPESW is torn
down.
Move the mlx5_lag_reset_vports_speed() call from mlx5_do_bond() into
mlx5_deactivate_lag(), so that every teardown path resets the vport
speeds.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <redacted>
Reviewed-by: Shay Drori <redacted>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Or Har-Toov <redacted>
Cache aggregate TX-enabled speed we calculated, and use it instead of
querying the bond speed so we can, in the following patch, limit it by
the PCI bandwidth.
The bond speed takes the calculation from PTYS, and PTYS is not bounded
by the PCI bandwidth of the NIC.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <redacted>
Reviewed-by: Shay Drori <redacted>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/infiniband/hw/mlx5/main.c | 2 +-
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 61 +++++++++++++++----
.../net/ethernet/mellanox/mlx5/core/lag/lag.h | 15 ++++-
include/linux/mlx5/driver.h | 1 +
4 files changed, 65 insertions(+), 14 deletions(-)
@@ -1496,6 +1495,30 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed)return0;}+voidmlx5_lag_update_agg_speed(structmlx5_lag*ldev)+{+u32speed;++lockdep_assert_held(&ldev->lock);++if(mlx5_lag_get_devices_oper_speed(ldev,&speed))+return;++/* If speed is not set, use the sum of max speeds of all PFs */+if(!speed&&mlx5_lag_get_devices_max_speed(ldev,&speed))+return;++ldev->agg_speed_mbps=speed;+}++voidmlx5_lag_reset_agg_speed(structmlx5_lag*ldev)+{+lockdep_assert_held(&ldev->lock);++ldev->agg_speed_mbps=0;+}++#ifdef CONFIG_MLX5_ESWITCHstaticvoidmlx5_lag_modify_device_vports_speed(structmlx5_core_dev*mdev,u32speed){
@@ -1541,17 +1564,10 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev)u32speed;intpf_idx;-if(ldev->mode==MLX5_LAG_MODE_MPESW){-if(mlx5_lag_get_devices_oper_speed(ldev,&speed))-return;-}else{-speed=ldev->tracker.bond_speed_mbps;-if(speed==SPEED_UNKNOWN)-return;-}+mlx5_lag_update_agg_speed(ldev);+speed=ldev->agg_speed_mbps;-/* If speed is not set, use the sum of max speeds of all PFs */-if(!speed&&mlx5_lag_get_devices_max_speed(ldev,&speed))+if(!speed)return;speed=speed/MLX5_MAX_TX_SPEED_UNIT;
From: Or Har-Toov <redacted>
FW initializes max_tx_speed to the minimum of the port speed and the
PCI bandwidth. Modifying it with a value computed by different rules
changes the meaning of the field rather than updating it.
Limit each uplink's contribution to the aggregated LAG TX speed by the
NIC's PCIe bandwidth and reset max_tx_speed to the same value when lag
is torn down.
Fixes: 50f1d188c580 ("net/mlx5: Propagate LAG effective max_tx_speed to vports")
Signed-off-by: Or Har-Toov <redacted>
Reviewed-by: Shay Drori <redacted>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/dev.c | 20 +++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 12 +++++++++++
.../ethernet/mellanox/mlx5/core/mlx5_core.h | 1 +
3 files changed, 33 insertions(+)
From: Or Har-Toov <redacted>
In RoCE LAG we don't modify any max_tx_speed, so no
IB_EVENT_DEVICE_SPEED_CHANGE is ever fired.
Raise IB_EVENT_DEVICE_SPEED_CHANGE from the LAG layer, in
mlx5_lag_update_agg_speed(), right after the cached aggregate is
updated. That cache is where the IB layer now takes the effective speed
from, so a consumer that queries after receiving the event is
guaranteed to read the new value.
Fixes: c6df9a65cbb0 ("net/mlx5: Skip disabled vports when setting max TX speed")
Signed-off-by: Or Har-Toov <redacted>
Reviewed-by: Shay Drori <redacted>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/infiniband/hw/mlx5/main.c | 10 ++++++++++
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 20 +++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lag/lag.h | 1 +
include/linux/mlx5/device.h | 1 +
4 files changed, 32 insertions(+)
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-15 01:51:22
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, split aggregate speed into oper and max helpers
This splits the single mlx5_lag_sum_devices_speed() helper into
mlx5_lag_get_devices_oper_speed() and mlx5_lag_get_devices_max_speed().
The oper helper skips ports that are not forwarding traffic, while the max
helper takes the best single port for active-backup and sums the ports for
XOR, LACP and MPESW.
A few questions on the new helpers below.
net/mlx5: Lag, split aggregate speed into oper and max helpers
The oper function skips a port for two reasons:
- the bonding driver set tx_enabled=false - LACP converging or
active-backup standby slave, where carrier may still be up but the
port is intentionally not forwarding traffic.
- the link is down.
[Severity: Low]
The changelog lists two skip reasons, but the new oper helper also adds a
third, MPESW-only gate that queries the vNIC vport state per PF:
if (mpesw) {
if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
VPORT_STATE_UP)
continue;
}
Since the value returned by this helper ends up in every vport's
max_tx_speed, would it be worth spelling out the MPESW gate and its effect
on the programmed speed in the changelog too?
[Severity: Medium]
Can a failed firmware query be mistaken for a down vport here?
mlx5_query_vport_state() in core/vport.c returns 0 on command failure:
err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
if (err)
return 0;
return MLX5_GET(query_vport_state_out, out, state);
and include/linux/mlx5/device.h has VPORT_STATE_DOWN = 0x0, so a timeout,
-EIO on a device in error state, or -EBUSY is indistinguishable from a real
"vport down" reply.
The PF is then silently dropped from the sum and the helper still returns 0,
unlike the mlx5_port_oper_linkspeed() failure a few lines below which aborts
the whole computation.
The under-computed value reaches the hardware through
mlx5_lag_set_vports_agg_speed() -> mlx5_lag_modify_device_vports_speed(),
which stores it in vport->agg_max_tx_speed and calls
mlx5_modify_vport_max_tx_speed() for every enabled vport, so with one
failing PF in a two-PF MPESW LAG the rate limit is roughly halved until the
next port change event. If every query fails the sum stays 0 and the
capability-max fallback below is used instead.
This path is reachable from ordinary events via
mlx5_lag_mpesw_port_change_event() -> mlx5_mpesw_speed_update_work() ->
mlx5_lag_set_vports_agg_speed(). Should the query failure be distinguished
from VPORT_STATE_DOWN and propagated like the linkspeed error?
quoted hunk
+ } else if (!ldev->tracker.netdev_state[pf_idx].tx_enabled ||+ !ldev->tracker.netdev_state[pf_idx].link_up) {+ continue;+ }
[Severity: Low]
Is this else-if arm reachable at this point of the series? The only caller
in this commit is mlx5_lag_set_vports_agg_speed(), which calls the helper
exclusively under:
if (ldev->mode == MLX5_LAG_MODE_MPESW) {
if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
return;
so mpesw is always true inside the helper and the tx_enabled/link_up skip
described in the changelog has no effect yet. The later patch
"{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports"
introduces mlx5_lag_update_agg_speed(), which calls the oper helper for all
LAG modes and makes this branch live, so this is only an intra-series
ordering observation.
[Severity: Low]
This is a pre-existing issue, not introduced by this patch, but these reads
of ldev->tracker.netdev_state[pf_idx].tx_enabled/.link_up (and the
tracker.tx_type read in mlx5_lag_get_devices_max_speed() below) take only
ldev->lock, while the writers use different conventions:
mlx5_lag_netdev_event() republishes the whole struct from RTNL context with
neither ldev->lock nor lag_lock held:
ldev->tracker = tracker;
if (changed)
mlx5_queue_bond_work(ldev, 0);
and mlx5_ldev_add_netdev()/mlx5_ldev_remove_netdev() write the same fields
under lag_lock. The same pattern already exists in the baseline, where
mlx5_do_bond() snapshots ldev->tracker under ldev->lock and
mlx5_infer_tx_enabled() consumes these fields to program the port mapping.
Any mixed snapshot here looks self-correcting because the event path requeues
the bond work, which recomputes the aggregate speed. Is it worth settling on
one lock for the tracker fields while this code is being reworked?
quoted hunk
- ret = get_speed(pf_mdev, &speed);+ ret = mlx5_port_oper_linkspeed(pf_mdev, &speed); if (ret) { mlx5_core_dbg(pf_mdev,- "Failed to get device speed using %ps. Device %s speed is not available (err=%d)\n",- get_speed, dev_name(pf_mdev->device),- ret);+ "Failed to get device %s oper speed (err=%d)\n",+ dev_name(pf_mdev->device), ret); return ret; }
+ ret = mlx5_port_max_linkspeed(pf_mdev, &speed);+ if (ret) {+ mlx5_core_dbg(pf_mdev,+ "Failed to get device %s max speed (err=%d)\n",+ dev_name(pf_mdev->device), ret);+ return ret;+ }++ *max_speed = take_max ?+ max(*max_speed, speed) : *max_speed + speed;+ }
[Severity: Low]
Both new helpers sum per-port Ethernet link speeds from
mlx5_port_max_linkspeed()/mlx5_port_oper_linkspeed(), which derive their
value purely from the PTYS protocol capabilities, with no bound from the
device's PCIe link bandwidth. The changelog says the max helper "represents
the maximum achievable speed", yet the summed value can exceed what the host
DMA path can sustain, and that value is programmed as the vport rate limit.
The follow-up patch "net/mlx5: Lag, limit aggregated TX speed by PCIe
bandwidth" adds the missing clamp in both helpers:
pci_bw = mlx5_pcie_bandwidth(pf_mdev);
if (pci_bw)
speed = min(speed, pci_bw);
so this is closed inside the series; was there a reason for keeping the
clamp in a separate patch rather than adding it here?
@@ -1505,7 +1540,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) int pf_idx; if (ldev->mode == MLX5_LAG_MODE_MPESW) {- if (mlx5_lag_sum_devices_oper_speed(ldev, &speed))+ if (mlx5_lag_get_devices_oper_speed(ldev, &speed)) return; } else { speed = ldev->tracker.bond_speed_mbps;
@@ -1514,7 +1549,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) } /* If speed is not set, use the sum of max speeds of all PFs */- if (!speed && mlx5_lag_sum_devices_max_speed(ldev, &speed))+ if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed)) return;
[Severity: Low]
Should this comment be updated? mlx5_lag_get_devices_max_speed() no longer
always sums, since:
take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
...
*max_speed = take_max ?
max(*max_speed, speed) : *max_speed + speed;
For an active-backup bond in a non-MPESW mode the fallback returns the best
single port speed, not "the sum of max speeds of all PFs", and that is the
value programmed into vport max_tx_speed.
The comment also survives unchanged in the follow-up patch that moves this
code into mlx5_lag_update_agg_speed(), so nothing later in the series
corrects it.
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-15 01:51:23
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.
@@ -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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-15 01:51:25
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports
The patch adds a new ldev->agg_speed_mbps cache in struct mlx5_lag,
populated by the new mlx5_lag_update_agg_speed() and cleared by
mlx5_lag_reset_agg_speed(). mlx5_lag_set_vports_agg_speed() now programs
vports from that cache, and mlx5_ib switches from mlx5_lag_query_bond_speed()
to the new exported mlx5_lag_query_aggregated_speed().
A few questions below.
@@ -1654,7 +1654,7 @@ static int mlx5_ib_query_port_speed_from_bond(struct mlx5_ib_dev *dev,u32bond_speed;interr;-err=mlx5_lag_query_bond_speed(mdev,&bond_speed);+err=mlx5_lag_query_aggregated_speed(mdev,&bond_speed);if(err)returnerr;
[Severity: Low]
Should the other in-tree consumer of the LAG aggregate speed be converted
too? After this patch mlx5_ib_query_port_speed_from_bond() uses the new
cached aggregate, but mlx5_esw_qos_max_link_speed_get() in
drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c still uses the bond speed
this commit message describes as unsuitable:
if (!mlx5_lag_is_active(mdev) ||
mlx5_lag_query_bond_speed(mdev, link_speed_max) < 0 ||
*link_speed_max == 0)
goto skip_lag;
That value feeds mlx5_esw_qos_link_speed_verify(), so a devlink rate
request can be accepted against the PTYS-derived bond speed while
mlx5_lag_set_vports_agg_speed() programs the vport with the (possibly
smaller, PCIe-clamped) cached aggregate, and while ib_query_port reports
the cached aggregate for the same LAG.
Two related observations: mlx5_lag_query_bond_speed() stays
EXPORT_SYMBOL_GPL (and declared in include/linux/mlx5/driver.h next to the
new getter) although no caller outside mlx5_core is left after this
conversion, and the two getters use different sentinels for "unknown" -
mlx5_lag_query_bond_speed() returns success with *speed == 0 and errors on
SPEED_UNKNOWN, while mlx5_lag_query_aggregated_speed() errors on 0.
@@ -1496,6 +1495,30 @@ static int mlx5_lag_get_devices_max_speed(struct mlx5_lag *ldev, u32 *max_speed) return 0; }+void mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)+{+ u32 speed;++ lockdep_assert_held(&ldev->lock);++ if (mlx5_lag_get_devices_oper_speed(ldev, &speed))+ return;++ /* If speed is not set, use the sum of max speeds of all PFs */
[Severity: Low]
This isn't a bug, but is the comment still accurate for active-backup?
mlx5_lag_get_devices_max_speed() does not sum in that case:
take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
if (ldev->mode == MLX5_LAG_MODE_MPESW)
take_max = false;
...
*max_speed = take_max ?
max(*max_speed, speed) : *max_speed + speed;
The comment was correct before the series, where
mlx5_lag_sum_devices_max_speed() unconditionally did *sum_speed += speed;
the take_max behaviour came in with "net/mlx5: Lag, split aggregate speed
into oper and max helpers" and this patch copies the older comment into the
new helper.
quoted hunk
+ if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))+ return;++ ldev->agg_speed_mbps = speed;+}
[Severity: Medium]
On the two early returns above, ldev->agg_speed_mbps keeps its previous
value and the void return type gives callers no way to notice. Is that
intended?
mlx5_lag_get_devices_oper_speed() propagates the error from
mlx5_port_oper_linkspeed() -> mlx5_port_query_eth_proto() ->
ACCESS_REG(PTYS), so an FW command error or a query during a health/reset
window lands here.
@@ -1541,17 +1564,10 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) u32 speed; int pf_idx;- if (ldev->mode == MLX5_LAG_MODE_MPESW) {- if (mlx5_lag_get_devices_oper_speed(ldev, &speed))- return;- } else {- speed = ldev->tracker.bond_speed_mbps;- if (speed == SPEED_UNKNOWN)- return;- }+ mlx5_lag_update_agg_speed(ldev);+ speed = ldev->agg_speed_mbps;- /* If speed is not set, use the sum of max speeds of all PFs */- if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))+ if (!speed) return;
[Severity: Medium]
Can this reprogram hardware with a stale speed? The !speed test cannot
distinguish a freshly computed value from a leftover non-zero cache, so
after a failed PTYS query mlx5_lag_set_vports_agg_speed() continues into
mlx5_lag_modify_device_vports_speed() and writes the previous aggregate
into vport->agg_max_tx_speed / MODIFY_VPORT_STATE for every non-uplink
vport.
Before this patch the MPESW path aborted on query failure and the
non-MPESW path did not touch PTYS at all (it used
tracker.bond_speed_mbps), so a failing query left the hardware untouched
instead of reprogramming the last known value. The same stale value is
then handed to mlx5_ib via mlx5_lag_query_aggregated_speed() and reported
as the current port rate.
Would returning an int from mlx5_lag_update_agg_speed(), or invalidating
the cache on failure, be preferable here?
quoted hunk
@@ -2068,6 +2085,28 @@ static int mlx5_handle_changeinfodata_event(struct mlx5_lag *ldev, return 1; }+/* Returns speed in Mbps. */+int mlx5_lag_query_aggregated_speed(struct mlx5_core_dev *mdev, u32 *speed)+{+ struct mlx5_lag *ldev;+ int ret = 0;++ ldev = mlx5_lag_dev(mdev);+ if (!ldev)+ return -ENODEV;++ mutex_lock(&ldev->lock);+ *speed = ldev->agg_speed_mbps;+ if (*speed == 0)+ ret = -EINVAL;+ mutex_unlock(&ldev->lock);
[Severity: Medium]
Does the validity window of this cache match the LAG-active state that
mlx5_ib uses to choose the bond path? The replaced
mlx5_lag_query_bond_speed() read tracker.bond_speed_mbps, which is set by
the netdev event that triggered bonding and is never cleared on
deactivation, so the failure conditions of the two getters differ.
At activation, mlx5_do_bond() sets the ROCE mode (so mlx5_lag_is_roce()
is already true) and then calls mlx5_lag_rescan_dev_locked(), which by its
own comment drops ldev->lock while the mlx5_ib aux device is probed and
ib_register_device() publishes the device. Only afterwards is the cache
populated:
mlx5_lag_rescan_dev_locked(ldev, dev0, true);
...
if (!shared_fdb)
mlx5_lag_set_vports_agg_speed(ldev);
mlx5_lag_shared_fdb_create() has the same ordering, calling
mlx5_lag_rescan_dev_locked() and mlx5_lag_reload_ib_reps_from_locked()
before mlx5_lag_set_vports_agg_speed().
A UVERBS_METHOD_QUERY_PORT_SPEED issued in that window reaches
mlx5_ib_query_port_speed_from_bond(), reads agg_speed_mbps == 0 and gets
-EINVAL back to userspace, where the pre-patch code returned a speed.
On teardown there is a similar ordering in mlx5_deactivate_lag():
mlx5_lag_reset_vports_speed(ldev); /* zeroes agg_speed_mbps */
dev0 = mlx5_lag_pf(ldev, master_idx)->dev;
ldev->mode = MLX5_LAG_MODE_NONE;
mlx5_ib samples mlx5_lag_is_roce()/mlx5_lag_is_sriov() under lag_lock and
only then takes ldev->lock, so it can observe "LAG active" followed by a
zeroed cache.
Note also that the peer helper mlx5_ib_query_port_speed_from_vport()
treats an unavailable speed as a fallback rather than an error:
if (vport_state == VPORT_STATE_DOWN || max_tx_speed == 0)
/* Value 0 indicates field not supported, fallback */
return mlx5_ib_query_port_speed_from_port(dev, port_num, speed);
Should from_bond() fall back to mlx5_ib_query_port_speed_from_port() as
well instead of propagating -EINVAL?
quoted hunk
++ if (ret == -EINVAL)+ mlx5_core_dbg(mdev, "aggregated speed is unknown\n");+ return ret;+}+EXPORT_SYMBOL_GPL(mlx5_lag_query_aggregated_speed);
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-15 01:51:26
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, limit aggregated TX speed by PCIe bandwidth
This adds a new helper mlx5_pcie_bandwidth() that derives a PCIe bandwidth
from PCI_EXP_LNKSTA, and uses it to clamp each uplink's contribution to the
aggregated LAG TX speed in mlx5_lag_get_devices_oper_speed(),
mlx5_lag_get_devices_max_speed() and mlx5_lag_reset_vports_speed(). A few
questions below about how the bandwidth is computed and where the clamp is
applied.
[Severity: Medium]
Is speed * width the same quantity the PCI core calls PCIe bandwidth?
pcie_link_speed_mbps() returns the raw per-lane transfer rate, while
pcie_bandwidth_available() in drivers/pci/pci.c applies the line encoding
correction and walks upstream taking the minimum:
drivers/pci/pci.h:
#define PCIE_SPEED2MBS_ENC(speed) \
((speed) == PCIE_SPEED_64_0GT ? 64000*1/1 : \
(speed) == PCIE_SPEED_32_0GT ? 32000*128/130 : \
...
(speed) == PCIE_SPEED_5_0GT ? 5000*8/10 : \
(speed) == PCIE_SPEED_2_5GT ? 2500*8/10 : \
drivers/pci/pci.c:pcie_bandwidth_available() {
...
next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
/* Check if current device limits the total bandwidth */
if (!bw || next_bw <= bw) {
...
dev = pci_upstream_bridge(dev);
}
For a Gen2 x8 link the helper here yields 5000 * 8 = 40000 Mb/s, whereas the
core helper yields 4000 * 8 = 32000 Mb/s. Does that make
min(speed, pci_bw) a no-op for a 40GbE uplink on a bus that cannot sustain
it? On Gen3 and later the difference is about 1.5%, which is still larger
than the MLX5_MAX_TX_SPEED_UNIT (100 Mb/s) granularity the value is
programmed at.
The same driver already computes this quantity for the same pdev with the
core helper:
drivers/net/ethernet/mellanox/mlx5/core/en/params.c:slow_pci_heuristic() {
...
pci_bw = pcie_bandwidth_available(mdev->pdev, NULL, NULL, NULL);
mlx5_core_dbg(mdev, "Max link speed = %d, PCI BW = %d\n", link_speed,
pci_bw);
...
}
Should mlx5_pcie_bandwidth() use pcie_bandwidth_available() as well, so the
driver does not carry two different definitions of PCI bandwidth for one
device?
[Severity: Low]
Can the width and speed here come from two different link states? The width
is taken from the local snapshot in lnksta, but the speed comes from a second
independent read of the very same register:
drivers/pci/pci.c:pcie_link_speed_mbps() {
err = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnksta);
if (err)
return err;
return pcie_dev_speed_mbps(to_pcie_link_speed(lnksta));
}
PCI_EXP_LNKSTA_NLW and PCI_EXP_LNKSTA_CLS both live in the word already held
in lnksta, and the callers hold only ldev->lock, which does not serialise
link retraining, autonomous width/speed changes, bandwidth control speed
changes or a function reset during health recovery. If the link changes
between the two reads, speed * width is a bandwidth the link never had.
Would deriving the speed from the existing snapshot with
pcie_dev_speed_mbps(to_pcie_link_speed(lnksta)) be both cheaper and
consistent?
quoted hunk
+ static bool is_eth_rep_supported(struct mlx5_core_dev *dev) { if (!IS_ENABLED(CONFIG_MLX5_ESWITCH))
[Severity: Medium]
Does clamping per uplink and then summing actually bound the aggregate for
the two PFs of a dual-port adapter? Both PFs sit on the same physical PCIe
link and read the same PCI_EXP_LNKSTA, so each contributes up to that single
link's bandwidth.
For a dual-port 200GbE adapter on Gen4 x16, mlx5_pcie_bandwidth() returns
16000 * 16 = 256000 for both PFs, each contributes min(200000, 256000) =
200000, and the aggregate becomes 400000 Mb/s, roughly 1.6x the shared link.
That aggregate is stored in ldev->agg_speed_mbps and then programmed into
every vport:
mlx5_lag_set_vports_agg_speed() {
...
speed = speed / MLX5_MAX_TX_SPEED_UNIT;
mlx5_ldev_for_each(pf_idx, 0, ldev) {
...
mlx5_lag_modify_device_vports_speed(mdev, speed);
}
Should the PCIe bandwidth instead be applied to the summed value, i.e. after
aggregation, so PFs sharing one link are not counted twice? Only the
take_max (active-backup) path stays bounded today.
[Severity: Medium]
The same double counting applies in this fallback when take_max is false,
which is the case for LACP and XOR and is forced for MPESW.
[Severity: Low]
Unrelated to the clamp itself, but the caller's comment no longer matches
this helper:
mlx5_lag_update_agg_speed() {
...
/* If speed is not set, use the sum of max speeds of all PFs */
if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed))
return;
}
With take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP,
the fallback returns max(*max_speed, speed) rather than a sum in
active-backup mode. The comment was accurate in the baseline, where
mlx5_lag_sum_devices_max_speed() unconditionally did *sum_speed += speed;
should it be updated to describe the policy dependent behaviour?
[Severity: Medium]
This is the value restored into vport max_tx_speed on LAG teardown, so if
mlx5_pcie_bandwidth() over-estimates the bus bandwidth (raw rate, no
encoding correction, endpoint link only), the restored value is the
over-estimated one rather than what FW originally programmed. Does that
still match the commit message claim that FW initializes max_tx_speed to the
minimum of the port speed and the PCI bandwidth?
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-15 01:51:28
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
{RDMA,net}/mlx5: notify RoCE LAG speed change via driver event
This adds a new MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE driver event that the
LAG layer raises from mlx5_lag_update_agg_speed() when the cached
aggregate speed changes, and the mlx5_ib lag_event() handler turns it
into an IB_EVENT_DEVICE_SPEED_CHANGE for the IB device. A couple of
questions below about the emit condition and the commit message.
In RoCE LAG we don't modify any max_tx_speed, so no
IB_EVENT_DEVICE_SPEED_CHANGE is ever fired.
[Severity: Medium]
Is this description of the existing behaviour accurate? handle_port_change()
in drivers/infiniband/hw/mlx5/main.c already dispatches this event, and it
does so before the Ethernet link layer early return:
case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
case MLX5_PORT_CHANGE_SUBTYPE_INITIALIZED:
if (ibdev->ib_active) {
struct ib_event speed_event = {};
speed_event.device = &ibdev->ib_dev;
speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
ib_dispatch_event(&speed_event);
}
/* In RoCE, port up/down events are handled in
* mlx5_netdev_event().
*/
if (mlx5_ib_port_link_layer(&ibdev->ib_dev, port) ==
IB_LINK_LAYER_ETHERNET)
return -EINVAL;
So for the master device of a RoCE LAG whose own physical port changes
state, the event is fired today.
The gap this patch appears to close is narrower: aggregate changes driven by
a port EQE that only lands on a peer mdev whose IB device was removed by
mlx5_lag_remove_devices(), and bonding state changes (tracker tx_enabled,
v2p remap) that update the cache asynchronously from the port EQE, where a
consumer querying on the existing event can still read the old aggregate.
Since the patch carries a Fixes: tag and is a stable candidate, would it be
worth rewording this to describe the case that is actually uncovered instead
of saying no such event is ever fired?
[Severity: Medium]
Should the emit be gated on mlx5_lag_is_roce_lag() here? The same cached
value that was just updated is also what the IB layer reports for the
switchdev uplink representor, in mlx5_ib_query_port_speed_rep():
if (rep->vport == MLX5_VPORT_UPLINK) {
if (mlx5_lag_is_sriov(mdev))
return mlx5_ib_query_port_speed_from_bond(dev,
port_num,
speed);
mlx5_lag_is_roce_lag() is a "no SR-IOV and legacy eswitch" predicate:
mlx5_ldev_for_each(i, 0, ldev) {
pf = mlx5_lag_pf(ldev, i);
roce_lag = roce_lag && !mlx5_sriov_is_enabled(pf->dev);
}
...
roce_lag = roce_lag && is_mdev_legacy_mode(pf->dev);
so in exactly the SR-IOV/shared-FDB LAG configuration where the uplink
representor reader is used, the gate is always false and no
IB_EVENT_DEVICE_SPEED_CHANGE is delivered even though the value exported
through QUERY_PORT_SPEED changed.
The reasoning given in the commit message applies to that reader too, since
mlx5_lag_modify_device_vports_speed() skips the uplink vport:
if (vport->vport == MLX5_VPORT_UPLINK)
continue;
The path I have in mind is: both PFs in switchdev mode and bonded, LAG comes
up in MLX5_LAG_MODE_SRIOV, then the peer PF link goes down and the bond work
runs mlx5_modify_lag() -> mlx5_lag_set_vports_agg_speed() ->
mlx5_lag_update_agg_speed(), which stores a different agg_speed_mbps with no
notification, while the peer's physical port EQE never reaches this IB
device's handle_port_change().
Separately, mlx5_lag_notify_speed_change() only calls the MLX5_LAG_P1 device's
priv.lag_nh, but each IB device registers on its own mdev's chain:
dev->lag_events.notifier_call = lag_event;
blocking_notifier_chain_register(&dev->mdev->priv.lag_nh,
&dev->lag_events);
In SR-IOV LAG the slave IB devices are not removed, so would the second PF's
IB device be reachable at all here even if the gate were relaxed?
[ ... ]
From: Or Har-Toov <hidden> Date: 2026-09-30 21:22:27
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, split aggregate speed into oper and max helpers
This splits the single mlx5_lag_sum_devices_speed() helper into
mlx5_lag_get_devices_oper_speed() and mlx5_lag_get_devices_max_speed().
The oper helper skips ports that are not forwarding traffic, while the max
helper takes the best single port for active-backup and sums the ports for
XOR, LACP and MPESW.
A few questions on the new helpers below.
quoted
net/mlx5: Lag, split aggregate speed into oper and max helpers
The oper function skips a port for two reasons:
- the bonding driver set tx_enabled=false - LACP converging or
active-backup standby slave, where carrier may still be up but the
port is intentionally not forwarding traffic.
- the link is down.
[Severity: Low]
The changelog lists two skip reasons, but the new oper helper also adds a
third, MPESW-only gate that queries the vNIC vport state per PF:
if (mpesw) {
if (mlx5_query_vport_state(pf_mdev, opmod, 0) !=
VPORT_STATE_UP)
continue;
}
Since the value returned by this helper ends up in every vport's
max_tx_speed, would it be worth spelling out the MPESW gate and its effect
on the programmed speed in the changelog too?
The MPESW case is also a port down. It is just that in lag we have this information in the tracker and in mpesw we need to query it from FW.
I will explain better in V2's commit message
[Severity: Medium]
Can a failed firmware query be mistaken for a down vport here?
mlx5_query_vport_state() in core/vport.c returns 0 on command failure:
err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
if (err)
return 0;
return MLX5_GET(query_vport_state_out, out, state);
and include/linux/mlx5/device.h has VPORT_STATE_DOWN = 0x0, so a timeout,
-EIO on a device in error state, or -EBUSY is indistinguishable from a real
"vport down" reply.
The PF is then silently dropped from the sum and the helper still returns 0,
unlike the mlx5_port_oper_linkspeed() failure a few lines below which aborts
the whole computation.
The under-computed value reaches the hardware through
mlx5_lag_set_vports_agg_speed() -> mlx5_lag_modify_device_vports_speed(),
which stores it in vport->agg_max_tx_speed and calls
mlx5_modify_vport_max_tx_speed() for every enabled vport, so with one
failing PF in a two-PF MPESW LAG the rate limit is roughly halved until the
next port change event. If every query fails the sum stays 0 and the
capability-max fallback below is used instead.
This path is reachable from ordinary events via
mlx5_lag_mpesw_port_change_event() -> mlx5_mpesw_speed_update_work() ->
mlx5_lag_set_vports_agg_speed(). Should the query failure be distinguished
from VPORT_STATE_DOWN and propagated like the linkspeed error?
good point, fix in V2
quoted
+ } else if (!ldev->tracker.netdev_state[pf_idx].tx_enabled ||+ !ldev->tracker.netdev_state[pf_idx].link_up) {+ continue;+ }
[Severity: Low]
Is this else-if arm reachable at this point of the series? The only caller
in this commit is mlx5_lag_set_vports_agg_speed(), which calls the helper
exclusively under:
if (ldev->mode == MLX5_LAG_MODE_MPESW) {
if (mlx5_lag_get_devices_oper_speed(ldev, &speed))
return;
so mpesw is always true inside the helper and the tx_enabled/link_up skip
described in the changelog has no effect yet. The later patch
"{RDMA,net}/mlx5: cache and use TX-enabled aggregate speed for vports"
introduces mlx5_lag_update_agg_speed(), which calls the oper helper for all
LAG modes and makes this branch live, so this is only an intra-series
ordering observation.
Removed the sriov case for now and will add it in a later patch
[Severity: Low]
This is a pre-existing issue, not introduced by this patch, but these reads
of ldev->tracker.netdev_state[pf_idx].tx_enabled/.link_up (and the
tracker.tx_type read in mlx5_lag_get_devices_max_speed() below) take only
ldev->lock, while the writers use different conventions:
mlx5_lag_netdev_event() republishes the whole struct from RTNL context with
neither ldev->lock nor lag_lock held:
ldev->tracker = tracker;
if (changed)
mlx5_queue_bond_work(ldev, 0);
and mlx5_ldev_add_netdev()/mlx5_ldev_remove_netdev() write the same fields
under lag_lock. The same pattern already exists in the baseline, where
mlx5_do_bond() snapshots ldev->tracker under ldev->lock and
mlx5_infer_tx_enabled() consumes these fields to program the port mapping.
Any mixed snapshot here looks self-correcting because the event path requeues
the bond work, which recomputes the aggregate speed. Is it worth settling on
one lock for the tracker fields while this code is being reworked?
This is not related to this series...
quoted
- ret = get_speed(pf_mdev, &speed);+ ret = mlx5_port_oper_linkspeed(pf_mdev, &speed); if (ret) { mlx5_core_dbg(pf_mdev,- "Failed to get device speed using %ps. Device %s speed is not available (err=%d)\n",- get_speed, dev_name(pf_mdev->device),- ret);+ "Failed to get device %s oper speed (err=%d)\n",+ dev_name(pf_mdev->device), ret); return ret; }
+ ret = mlx5_port_max_linkspeed(pf_mdev, &speed);+ if (ret) {+ mlx5_core_dbg(pf_mdev,+ "Failed to get device %s max speed (err=%d)\n",+ dev_name(pf_mdev->device), ret);+ return ret;+ }++ *max_speed = take_max ?+ max(*max_speed, speed) : *max_speed + speed;+ }
[Severity: Low]
Both new helpers sum per-port Ethernet link speeds from
mlx5_port_max_linkspeed()/mlx5_port_oper_linkspeed(), which derive their
value purely from the PTYS protocol capabilities, with no bound from the
device's PCIe link bandwidth. The changelog says the max helper "represents
the maximum achievable speed", yet the summed value can exceed what the host
DMA path can sustain, and that value is programmed as the vport rate limit.
The follow-up patch "net/mlx5: Lag, limit aggregated TX speed by PCIe
bandwidth" adds the missing clamp in both helpers:
pci_bw = mlx5_pcie_bandwidth(pf_mdev);
if (pci_bw)
speed = min(speed, pci_bw);
so this is closed inside the series; was there a reason for keeping the
clamp in a separate patch rather than adding it here?
These are 2 different bugs so better to have them in separated patches>>
@@ -1505,7 +1540,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) int pf_idx; if (ldev->mode == MLX5_LAG_MODE_MPESW) {- if (mlx5_lag_sum_devices_oper_speed(ldev, &speed))+ if (mlx5_lag_get_devices_oper_speed(ldev, &speed)) return; } else { speed = ldev->tracker.bond_speed_mbps;
@@ -1514,7 +1549,7 @@ void mlx5_lag_set_vports_agg_speed(struct mlx5_lag *ldev) } /* If speed is not set, use the sum of max speeds of all PFs */- if (!speed && mlx5_lag_sum_devices_max_speed(ldev, &speed))+ if (!speed && mlx5_lag_get_devices_max_speed(ldev, &speed)) return;
[Severity: Low]
Should this comment be updated? mlx5_lag_get_devices_max_speed() no longer
always sums, since:
take_max = ldev->tracker.tx_type == NETDEV_LAG_TX_TYPE_ACTIVEBACKUP;
...
*max_speed = take_max ?
max(*max_speed, speed) : *max_speed + speed;
For an active-backup bond in a non-MPESW mode the fallback returns the best
single port speed, not "the sum of max speeds of all PFs", and that is the
value programmed into vport max_tx_speed.
The comment also survives unchanged in the follow-up patch that moves this
code into mlx5_lag_update_agg_speed(), so nothing later in the series
corrects it.
From: Or Har-Toov <hidden> Date: 2026-10-01 14:49:09
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.
@@ -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