* Re: [PATCH net-next] mlxsw: Disable LAG collector when requested
2026-11-11 17:09 [PATCH net-next] mlxsw: Disable LAG collector when requested Ido Schimmel
@ 2026-11-12 17:17 ` netdev-bot+sashiko
2026-11-12 19:20 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-11-12 17:17 UTC (permalink / raw)
To: idosch; +Cc: netdev, davem, kuba, pabeni, edumazet, andrew+netdev, petrm
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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).
- [Medium] Before this patch, mlxsw_reg_slcor_col_disable_pack() did
nothing in practice.
- [Low] mlxsw: missing Fixes trailer for
mlxsw_reg_slcor_col_disable_pack() bug fix
--
mlxsw: Disable LAG collector when requested
This commit changes mlxsw_reg_slcor_col_disable_pack() so that it packs
MLXSW_REG_SLCOR_COL_LAG_COLLECTOR_DISABLED instead of the "collector
enabled" value. A collector disable request now takes effect in the ASIC,
and packets received on a disabled LAG port are dropped in hardware.
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
> index 01fcbd1c8d015..9dbeabf5536d1 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
> +++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
> @@ -1491,7 +1491,7 @@ static inline void mlxsw_reg_slcor_col_disable_pack(char *payload,
> u16 local_port, u16 lag_id)
> {
> mlxsw_reg_slcor_pack(payload, local_port, lag_id,
> - MLXSW_REG_SLCOR_COL_LAG_COLLECTOR_ENABLED);
> + MLXSW_REG_SLCOR_COL_LAG_COLLECTOR_DISABLED);
> }
[Severity: Low]
This fixes a real bug in mlxsw_reg_slcor_col_disable_pack(), but the
commit message has no Fixes: trailer. As far as I can tell the helper
has packed MLXSW_REG_SLCOR_COL_LAG_COLLECTOR_ENABLED since it was added
in commit d1d40be084c6 ("mlxsw: reg: Add link aggregation configuration
registers definitions"). The LAG code in spectrum.c has used it to
disable collection since commit 0d65fc13042f ("mlxsw: spectrum:
Implement LAG port join/leave").
The commit message says this is not a regression, and the patch is
aimed at net-next, so leaving the tag out may be deliberate. Without
it, though, stable tooling has nothing pointing at where the bug came
from. The same goes for anyone bisecting LAG ingress behaviour on
Spectrum.
Should this carry a Fixes: tag against d1d40be084c6? If the intent is
to keep it out of stable because it changes ingress behaviour on
existing LAG setups, could the commit message say so explicitly?
[Severity: Medium]
Now that this write takes effect, can mlxsw turn off the hardware
collector on an 802.3ad port that the bond still treats as collecting?
In drivers/net/ethernet/mellanox/mlxsw/spectrum.c, one tx_enabled bit
controls both the distributor and the collector:
mlxsw_sp_port_lag_changed() {
if (info->tx_enabled)
return mlxsw_sp_port_lag_col_dist_enable(mlxsw_sp_port);
else
return mlxsw_sp_port_lag_col_dist_disable(mlxsw_sp_port);
}
That bit comes from bond_lower_state_changed(), which only reports
whether the port is distributing:
info.tx_enabled = bond_is_active_slave(slave);
struct netdev_lag_lower_state_info has no collecting field. So the driver
can't tell "collecting but not distributing" apart from "disabled".
Also, mlxsw_sp_master_lag_check() only checks tx_type. Bonds with
coupled_control=0 are still offloaded.
With independent control, ad_mux_machine() can move a port from
AD_MUX_DISTRIBUTING to AD_MUX_COLLECTING, for example when the partner
clears its Collecting bit. On entry to that state the bond does:
case AD_MUX_COLLECTING:
port->actor_oper_port_state |= LACP_STATE_COLLECTING;
port->actor_oper_port_state &= ~LACP_STATE_DISTRIBUTING;
port->actor_oper_port_state |= LACP_STATE_SYNCHRONIZATION;
ad_enable_collecting(port);
ad_disable_distributing(port, update_slave_arr);
ad_disable_distributing() calls bond_set_slave_tx_disabled_flags(). That
function only sets the backup state and leaves slave->inactive at 0. As a
result, bond_should_deliver_exact_match() still accepts frames on this
port in software.
The lower state notification still carries tx_enabled=0, so the driver
runs:
mlxsw_sp_port_lag_changed()
mlxsw_sp_port_lag_col_dist_disable()
mlxsw_sp_lag_col_port_disable()
mlxsw_reg_slcor_col_disable_pack() /* now COLLECTOR_DISABLED */
Before this patch, the ASIC kept collecting in this state, which matched
the bond.
A conformant partner only clears Collecting while it is in ATTACHED and
not distributing, so this doesn't look like a lasting black hole. The
problem is the window when the partner starts sending again:
- A coupled control partner moves to COLLECTING_DISTRIBUTING as soon as
it sees our Sync.
- An independent control partner moves to DISTRIBUTING as soon as it sees
our Collecting.
Would the partner's first frames be dropped in hardware during that
window? The drops would last until our mux machine reaches
AD_MUX_DISTRIBUTING and the deferred bond_slave_state_notify() delivers
tx_enabled=1.
This also seems to conflict with the commit message:
"Packets received on a disabled LAG port are now dropped, as they are
in the bond's software datapath."
In AD_MUX_COLLECTING, the bond's software datapath doesn't drop these
packets. Is that statement accurate for this state?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111170925.2809513-1-idosch%40nvidia.com
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next] mlxsw: Disable LAG collector when requested
2026-11-11 17:09 [PATCH net-next] mlxsw: Disable LAG collector when requested Ido Schimmel
2026-11-12 17:17 ` netdev-bot+sashiko
@ 2026-11-12 19:20 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-11-12 19:20 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, davem, kuba, pabeni, edumazet, andrew+netdev, petrm
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 11 Nov 2026 19:09:25 +0200 you wrote:
> mlxsw_reg_slcor_col_disable_pack() packs the "collector enabled" value
> instead of "collector disabled". As a result, once a LAG port is enabled
> for collection, it keeps receiving packets even after the LAG master
> disables it (e.g., when the port loses link or the LACP partner falls
> out of sync).
>
> Fix by packing the correct value. Packets received on a disabled LAG
> port are now dropped, as they are in the bond's software datapath.
>
> [...]
Here is the summary with links:
- [net-next] mlxsw: Disable LAG collector when requested
https://git.kernel.org/netdev/net-next/c/073d54dc3ccc
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread