Re: [PATCH net-next V4 11/14] net/mlx5e: psp: Dynamically reconfigure based on SHAMPO mode

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In mlx5e_update_nic_rx()
  (drivers/net/ethernet/mellanox/mlx5/core/en_main.c:6289-6300) the…

--

net/mlx5e: psp: Dynamically reconfigure based on SHAMPO mode

This adds an mlx5e_update_nic_rx() -> mlx5e_accel_update_rx() ->
mlx5e_psp_update_rx() hook so PSP steering can switch between standard and
decap modes when the SHAMPO mode changes.  It also enables decap at fs
creation time when shampo_enabled() says so.

[ ... ]

[ ... ]

[Severity: Medium]
Should the new PSP hook be gated on the TIR refresh result?  The two
operations look unrelated (TIR self-loopback refresh versus the PSP check
rule destination), so a failing mlx5e_refresh_tirs() now also skips the PSP
steering reconfiguration.

mlx5e_refresh_tirs() does have reachable failure modes on devices where
MLX5_CAP_GEN(mdev, tis_tir_td_order) is 0, since it then calls
mlx5e_modify_tirs_lb() in en_common.c, which returns -ENOMEM when
mlx5e_tir_builder_alloc() fails and otherwise propagates the MODIFY_TIR
firmware error:

drivers/net/ethernet/mellanox/mlx5/core/en_common.c:mlx5e_modify_tirs_lb() {
	builder = mlx5e_tir_builder_alloc(true);
	if (!builder)
		return -ENOMEM;
	...
		err = mlx5e_tir_modify(tir, builder);
		if (err) {
	...
			break;
}

The live reconfiguration caller does not look at the return value either,
and it has already committed the new channel params by then:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_switch_priv_channels() {
	priv->channels = *new_chs;
	...
	priv->profile->update_rx(priv);
	...
	mlx5e_activate_priv_channels(priv);
}

In that case the datapath runs with the new packet_merge.type while
fs->decap_enabled still reflects the old mode, and nothing re-evaluates it,
because shampo_enabled() is only consulted from mlx5e_psp_update_rx() and
accel_psp_fs_create().  When SHAMPO has just been enabled the PSP check rule
keeps pointing at fs->rx.ft rather than fs->decap.ft, so HW GRO for PSP stays
unavailable, and unlike accel_psp_fs_rx_reconfigure()'s own error path there
is no mlx5_core_warn() to indicate it.

The RX path itself appears to tolerate the mismatch, since
mlx5e_psp_is_decap() keys off the per-CQE flow tag and en_rx.c flushes the
SHAMPO session for non-decap PSP packets, and the state self-heals on the
next channel switch or open.  Would calling mlx5e_accel_update_rx(priv)
unconditionally, or before mlx5e_refresh_tirs(), be preferable here?

This condition is still present at the end of the series
(a9e41034d482a6cbfe0487f9ed8050c5bf5c3ce1), where mlx5e_update_nic_rx() is
unchanged.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908065244.3799142-1-tariqt%40nvidia.com