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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · 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_ethtool_get_ringparam() now reports tcp-data-split from
  the live SHAMPO state (priv->channels.params.packet_merge.type).
- [Medium] With HW-GRO off (the default, en_main.c:5949 clears
  NETIF_F_GRO_HW), GET now reports 'disabled', so hds.py _get_hds_mode()…
- [Medium] net/mlx5e: ethtool tcp-data-split API contract violation on
  unsupported hardware
- [Low] The patch puts back exactly the tcp_data_split assignment in
  mlx5e_ethtool_get_ringparam() that commit 46bcce5dfd33 ('net/mlx5e:…

--

net/mlx5e: advertise tcp-data-split support

This patch makes mlx5e_ethtool_get_ringparam() report tcp-data-split
again. The value is ENABLED when the channel packet merge type is SHAMPO
and DISABLED otherwise. Commit 46bcce5dfd33 dropped this reporting.
After commit d347b28c492e started reporting hds_thresh, the hds.py
selftests hit a KeyError on tcp-data-split.

> The ability to set tcp-data-split was introduced in
> commit 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split settings")
> This commit dropped reporting from mlx5e_ethtool_get_ringparam()
> without explaining why.

[Severity: Low]
The assignment added here is the same one that 46bcce5dfd33 removed from
mlx5e_ethtool_get_ringparam(). The message also names both commits that
led to the selftest failure.

Should this have a Fixes: tag, for example:

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

so that stable and backport tooling picks it up?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 261c466a4d362..62060a6fa185c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -378,6 +378,11 @@ void mlx5e_ethtool_get_ringparam(struct mlx5e_priv *priv,
>  
>  	kernel_param->hds_thresh = 0;
>  	kernel_param->hds_thresh_max = 0;
> +
> +	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]
What does this report on a device that cannot do SHAMPO at all?

If the SHAMPO capability bits, such as MLX5_CAP_GEN(mdev, shampo),
are not set, HW-GRO can never be turned on. In that case
packet_merge.type can never be MLX5E_PACKET_MERGE_SHAMPO, so this
assignment always reports ETHTOOL_TCP_DATA_SPLIT_DISABLED.

The ethtool netlink code leaves ETHTOOL_A_RINGS_TCP_DATA_SPLIT out of
the RINGS_GET reply only when the value is
ETHTOOL_TCP_DATA_SPLIT_UNKNOWN. Userspace therefore treats the presence
of the attribute as "the device supports header-data split".

With this change every mlx5e device advertises the feature as
supported but off. That includes devices where
mlx5e_ethtool_set_tcp_data_split() can only reject a later
"tcp-data-split on", because GRO HW is not available.

hds.py shows the effect. _get_hds_mode() skips only when
'tcp-data-split' is missing from the reply. On such hardware, get_hds
and the xdp tests would now run instead of skipping, and the enable
paths would fail with EINVAL.

Should the value stay ETHTOOL_TCP_DATA_SPLIT_UNKNOWN when the device
lacks SHAMPO support? For example, the driver could check the SHAMPO
capability, or whether NETIF_F_GRO_HW is in priv->netdev->hw_features,
before reporting ENABLED or DISABLED.

[Severity: Medium]
Can this report "enabled" right after a "tcp-data-split off" request has
succeeded?

GET now reflects the live SHAMPO state. The set path does not always
turn SHAMPO off when DISABLED is requested. In
mlx5e_ethtool_set_tcp_data_split():

	/* Might need to disable HW-GRO if it was kept on due to hds. */
	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
	    dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED)
		netdev_update_features(priv->netdev);

	return true;

Suppose the user has run "ethtool -K ethX rx-gro-hw on", so
NETIF_F_GRO_HW is in wanted_features:

- If hds_config is UNKNOWN, netdev_update_features() is never called.
- If hds_config is ENABLED, mlx5e_fix_features() only stops forcing
  GRO_HW on. GRO_HW stays on because it is still wanted.

In both cases SHAMPO stays active.

mlx5e_ethtool_set_ringparam() then returns early because the ring sizes
did not change:

	if (log_rq_size == priv->channels.params.log_rq_mtu_frames &&
	    log_sq_size == priv->channels.params.log_sq_size)
		return 0;

and ethnl_set_rings() commits the request:

	dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;

On the next RINGS_GET, rings_prepare_data() prefills tcp_data_split
from dev->cfg->hds_config, which is DISABLED. This new assignment then
overwrites it with ENABLED.

With rx-gro-hw on, wouldn't set_hds_disable() in hds.py still fail?
It would now fail on ksft_eq('disabled', rings['tcp-data-split'])
instead of the KeyError.

Should the driver actually turn SHAMPO off for DISABLED, or reject the
request?

[Severity: Medium]
HW-GRO is off by default, so this now reports "disabled" and
_get_hds_mode() in hds.py no longer skips. Is that why enabled_set_xdp
and ioctl_enabled_set_xdp go from SKIP to FAIL in the "After" log in the
commit message?

Both tests always request:

    netnl.rings_set({'header': {'dev-index': cfg.ifindex},
                     'tcp-data-split': 'enabled'})

but mlx5e_ethtool_set_tcp_data_split() rejects that unless HW-GRO is
already on:

	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
	    !(dev->features & NETIF_F_GRO_HW)) {
		NL_SET_ERR_MSG_MOD(extack,
				   "TCP-data-split is not supported when GRO HW is disabled");
		return false;
	}

set_hds_enable() gets the same -EINVAL. The test maps it to a skip, and
the existing skip message looks copy-pasted from set_hds_disable():

        if e.error == errno.EINVAL:
            raise KsftSkipEx("disabling of HDS not supported by the device")

So the rejected enable shows up as a skip with the wrong reason, and the
enable path is never tested.

Should the driver turn on HW-GRO when tcp-data-split=enabled is
requested? Or should the tests enable rx-gro-hw first, or skip on EINVAL?
As things stand, the state the driver reports does not match what the
selftests expect.

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