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.
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c
> index 84133cbf8689..c40d2ddf6af2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c
[ ... ]
> @@ -1355,3 +1363,14 @@ void mlx5e_psp_cleanup(struct mlx5e_priv *priv)
> priv->psp = NULL;
> kfree(psp);
> }
> +
> +void mlx5e_psp_update_rx(struct mlx5e_priv *priv)
> +{
> + struct mlx5e_psp *psp = priv->psp;
> +
> + netdev_assert_locked(priv->netdev);
> + if (!psp || !psp->fs->check.ft)
> + return;
> +
> + accel_psp_fs_rx_reconfigure(psp->fs, shampo_enabled(priv));
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index fc110a7d16e8..b5449d849838 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6288,7 +6288,15 @@ static void mlx5e_nic_disable(struct mlx5e_priv *priv)
>
> static int mlx5e_update_nic_rx(struct mlx5e_priv *priv)
> {
> - return mlx5e_refresh_tirs(priv->mdev, false, false);
> + int err;
> +
> + err = mlx5e_refresh_tirs(priv->mdev, false, false);
> + if (err)
> + return err;
> +
> + mlx5e_accel_update_rx(priv);
> +
> + return 0;
> }
[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
