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

[netdev-bot+sashiko@kernel.org]

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