Re: [PATCH net-next] mlxsw: Disable LAG collector when requested

[netdev-bot+sashiko@kernel.org]

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