Re: [PATCH net-next v4] net/mlx5e: advertise tcp-data-split support

[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] mlx5e_get_ringparam() now reports tcp-data-split as ENABLED
  whenever SHAMPO is active, even when dev->cfg->hds_config is still…
- [Low] The patch has no Fixes: tag, even though its commit message
  describes regressions caused by commit 46bcce5dfd33 ("net/mlx5e:…

--

net/mlx5e: advertise tcp-data-split support

On devices that support HW-GRO, mlx5e_get_ringparam() now reports
tcp-data-split as enabled when SHAMPO is active and as disabled otherwise.
The patch also rejects disabling tcp-data-split while HW-GRO is on.
mlx5e_fix_features() now keeps HW-GRO off when tcp-data-split is disabled.

> Since commit 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split
> settings"), mlx5 no longer reports tcp-data-split from
> mlx5e_ethtool_get_ringparam() and the reply carries only the stored
> config. With the default config (auto) the attribute is omitted, so the
> device looks like it does not support tcp-data-split, even while HW-GRO
> is on and SHAMPO is splitting headers.

[Severity: Low]
This isn't a bug, but should this patch carry a Fixes: tag?

The commit message names 46bcce5dfd33 as the cause. That commit removed
the assignment of kernel_param->tcp_data_split from the SHAMPO state in
mlx5e_ethtool_get_ringparam().

Since then, rings_prepare_data() seeds the value from
dev->cfg->hds_config. rings_fill_reply() only emits
ETHTOOL_A_RINGS_TCP_DATA_SPLIT when the value is non-zero, so in auto mode
the attribute is missing.

46bcce5dfd33 also enforced only one direction in mlx5e_fix_features().
This patch restores the reporting and adds the missing direction, so
would this be appropriate:

Fixes: 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split settings")

Without the tag, stable kernels that contain 46bcce5dfd33 would keep the
missing attribute in auto mode.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 1f534051cdfe4..8e7c748caf2d2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -388,6 +388,13 @@ static void mlx5e_get_ringparam(struct net_device *dev,
>  	struct mlx5e_priv *priv = netdev_priv(dev);
>  
>  	mlx5e_ethtool_get_ringparam(priv, param, kernel_param);
> +
> +	if (dev->hw_features & NETIF_F_GRO_HW)
> +		kernel_param->tcp_data_split =
> +			priv->channels.params.packet_merge.type ==
> +			MLX5E_PACKET_MERGE_SHAMPO ?
> +			ETHTOOL_TCP_DATA_SPLIT_ENABLED :
> +			ETHTOOL_TCP_DATA_SPLIT_DISABLED;
>  }

[Severity: Medium]
With this change, tcp-data-split is reported as enabled whenever SHAMPO
is active, even while dev->cfg->hds_config is still auto
(ETHTOOL_TCP_DATA_SPLIT_UNKNOWN). Does this break the XDP cases in
tools/testing/selftests/drivers/net/hds.py?

With HW-GRO on, set_xdp() and ioctl_set_xdp() now see 'enabled' and set
tcp-data-split back to 'unknown'. hds_config is already unknown, so
ethnl_set_rings() sees no change and returns without calling the
driver. HW-GRO and SHAMPO stay on.

The core checks in dev_xdp_attach() and netif_xdp_propagate() only
reject single-buffer XDP when hds_config is
ETHTOOL_TCP_DATA_SPLIT_ENABLED. The request therefore reaches the driver,
and mlx5e_xdp_allowed() rejects it:

	if (params->packet_merge.type != MLX5E_PACKET_MERGE_NONE) {
		netdev_warn(netdev, "can't set XDP while HW-GRO/LRO is on, disable them first\n");
		return -EINVAL;

That matches "not ok 9 hds.set_xdp" and "not ok 12 hds.ioctl_set_xdp" in
the posted run. Before this patch, ioctl_set_xdp found the attribute
missing in auto mode and skipped.

In auto mode with HDS reported as on, the ring param also can't be used
to turn HDS off. mlx5e_ethtool_set_tcp_data_split() now rejects
'disabled' while HW-GRO is on.

With HW-GRO off, enabled_set_xdp() and ioctl_enabled_set_xdp() call
rings_set() with tcp-data-split set to 'enabled' and no exception
handling. The driver now returns -EINVAL for that request.

The "SKIP enabling of HDS not supported by the device" lines in the
posted results don't seem to exist anywhere under
tools/testing/selftests/. Did those results come from a locally modified
hds.py?

Because of the _hds_reset() logic in hds.py, some of these cases could
already fail before this patch, depending on test order.

Should hds.py be updated together with this change? If not, could the
commit message mention the expected hds.py failures on mlx5?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111170558.1654691-1-dimitri.daskalakis1%40gmail.com