Re: [PATCH net-next v2] 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 (2) · Low: 1 (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] In mlx5e_ethtool_get_ringparam() (en_ethtool.c:382-384),
  tcp_data_split is only ever set to ENABLED, and only while…
- [Medium] With HW GRO enabled and HDS left in auto mode (cfg UNKNOWN),
  mlx5e_ethtool_get_ringparam() now reports tcp-data-split='enabled'.
- [Medium] mlx5e_ethtool_set_tcp_data_split() now calls
  netdev_update_features() whenever dev->cfg->hds_config differs from…
- [Low] The commit message says 'This is not a fix', yet part of the
  patch restores the reporting that 46bcce5dfd33 removed.

Pre-existing issues:
- [Medium] The KeyError cited in the commit message comes from the
  selftest helper _hds_reset() (hds.py:64-82).
- [Medium] mlx5e_ethtool_get_ringparam() reports tcp-data-split=enabled
  whenever SHAMPO is active, whether dev->cfg->hds_config is UNKNOWN or…
- [Low] In hds.py set_hds_enable(), an EINVAL from the *enable* request
  is reported with the skip reason 'disabling of HDS not supported by…

--

net/mlx5e: advertise tcp-data-split support

This patch brings back tcp-data-split reporting in
mlx5e_ethtool_get_ringparam() when SHAMPO packet merge is active.
It also rejects requests to disable tcp-data-split. Finally, it changes
mlx5e_ethtool_set_tcp_data_split() to call netdev_update_features()
whenever the requested value differs from the current hds_config.

> Between these two commits, the hds selftest helper _defer_reset_hds()
> would skip the reset because neither hds-thresh or tcp-data-split was
> present in the ring config. After the second commit this introduced
> a KeyError in the hds.set_hds_enable/disable tests.

[Severity: Medium]
This is a pre-existing issue, but the KeyError itself comes from
_hds_reset() in tools/testing/selftests/drivers/net/hds.py. After
resetting to "unknown", that function reads the attribute directly:

        arg['tcp-data-split'] = "unknown"
        netnl.rings_set(arg)
        cur = netnl.rings_get({'header': {'dev-index': cfg.ifindex}})
        if cur['tcp-data-split'] == rings['tcp-data-split']:

This patch doesn't change the helper, and mlx5 still seems able to reach
it.

For example: rx-gro-hw on, tcp-data-split on, then rx-gro-hw off, so
HW-GRO is kept on because of hds. In that state set_xdp() records
"enabled" and then sets "unknown".

The new hds_config != tcp_data_split path calls netdev_update_features().
That drops HW-GRO and SHAMPO, so the attribute disappears from the GET
reply. The deferred _hds_reset() then sends "unknown" again and hits a
KeyError on cur['tcp-data-split'].

Would using .get() in _hds_reset() be more robust? Any other driver that
accepts "disabled" and reports unknown in auto mode can hit the same
KeyError.

> This is not a fix since the feature is still functional with
> HW GRO enabled.

[Severity: Low]
Part of this patch puts back the reporting that commit 46bcce5dfd33
removed from mlx5e_ethtool_get_ringparam(), which was the
SHAMPO ? ENABLED : DISABLED assignment. Should this have a Fixes: tag for
46bcce5dfd33?

The condition for calling netdev_update_features() also changes. It was
(disabled requested && hds_config enabled) and is now (hds_config !=
requested). That makes "auto" the only way to release HW-GRO that hds
kept on.

The commit message doesn't say this. The new extack text, "TCP-data-split
can not be disabled", also doesn't point users to "auto". Could both be
mentioned?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 261c466a4d362..f08a968acec49 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -378,6 +378,10 @@ void mlx5e_ethtool_get_ringparam(struct mlx5e_priv *priv,
>  
>  	kernel_param->hds_thresh = 0;
>  	kernel_param->hds_thresh_max = 0;
> +
> +	if (priv->netdev->hw_features & NETIF_F_GRO_HW &&
> +	    priv->channels.params.packet_merge.type == MLX5E_PACKET_MERGE_SHAMPO)
> +		kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;

[Severity: Medium]
There is only a branch for enabled here. When packet_merge.type is not
SHAMPO, tcp_data_split keeps the value the core copied from
dev->cfg->hds_config. That value is unknown by default, and
rings_fill_reply() leaves the attribute out of the reply in that case:

net/ethtool/rings.c:rings_fill_reply() {
    ...
	    (kr->tcp_data_split &&
	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
			 kr->tcp_data_split))) ||
    ...
}

mlx5e_build_nic_netdev() clears NETIF_F_GRO_HW by default. Doesn't that
mean a freshly probed device that supports HW GRO still looks as if it
doesn't support tcp-data-split? The "after" output in the changelog shows
hds.get_hds still skipping with "tcp-data-split not supported by device".

The v2 changelog says unknown is left only "if the device does not
support HW GRO", and the hw_features test looks meant for that. But SHAMPO
is only set in set_feature_hw_gro() on the enable path. So SHAMPO already
means NETIF_F_GRO_HW is in hw_features, and the hw_features test never
changes the result.

Should this report ETHTOOL_TCP_DATA_SPLIT_DISABLED when HW GRO is
supported but SHAMPO isn't active, like the code that 46bcce5dfd33
removed?

[Severity: Medium]
This is a pre-existing limitation of the selftest rather than something
this patch introduces. With this reporting, enabled is returned whenever
SHAMPO is active, whether dev->cfg->hds_config is unknown or enabled.

With HW GRO on and hds_config unknown, hds.py set_hds_enable() records
"enabled", sets "enabled" (hds_config becomes enabled), and then checks
for "enabled". The deferred _hds_reset() only acts when the reported
values differ:

    if cur.get('tcp-data-split') != rings.get('tcp-data-split'):

So hds_config is never set back to unknown, and mlx5e_fix_features() keeps
forcing HW-GRO on:

	if (cfg && cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
	    !(features & NETIF_F_GRO_HW)) {

The core then rejects single-buffer XDP with -EBUSY. In a full hds.py run,
later tests reset to "unknown" and hide this. Running set_hds_enable on
its own leaves the device changed. netdevsim has the same gap, since it
also reports enabled when hds_config is unknown.

[Severity: Medium]
With rx-gro-hw on and HDS left in auto mode, this now reports "enabled",
so hds.py set_xdp() and ioctl_set_xdp() no longer skip. set_xdp() does:

    mode = _get_hds_mode(cfg, netnl)
    if mode == 'enabled':
        _defer_reset_hds(cfg, netnl)
        netnl.rings_set({'header': {'dev-index': cfg.ifindex},
                         'tcp-data-split': 'unknown'})

    _xdp_onoff(cfg)

hds_config is already unknown, so mlx5e_ethtool_set_tcp_data_split()
doesn't call netdev_update_features(). HW-GRO is still in wanted_features
anyway, so packet_merge stays SHAMPO. mlx5e_xdp_allowed() then rejects
the program:

	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;
	}

Does this turn two tests that used to skip into failures whenever
rx-gro-hw is on? The tests expect XDP installation to work in auto mode,
and the "after" run looks like it was done with HW GRO off.

>  static void mlx5e_get_ringparam(struct net_device *dev,
> @@ -403,9 +407,14 @@ static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
>  		return false;
>  	}
>  

[Severity: Low]
This isn't a bug introduced by this patch. The check just above returns
EINVAL for an enable request when HW GRO is off:

	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
	    !(dev->features & NETIF_F_GRO_HW)) {

hds.py set_hds_enable() reports that EINVAL with a skip reason copied from
set_hds_disable():

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

The "after" output shows "hds.set_hds_enable # SKIP disabling of HDS not
supported by the device", which names the wrong operation.

> +	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "TCP-data-split can not be disabled");
> +		return false;
> +	}
> +
>  	/* 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)
> +	if (dev->cfg->hds_config != tcp_data_split)
>  		netdev_update_features(priv->netdev);

[Severity: Medium]
With the wider condition, a change from enabled to unknown ("auto") now
calls netdev_update_features() here. That happens before
mlx5e_set_ringparam() validates the ring sizes:

	if (!mlx5e_ethtool_set_tcp_data_split(priv,
					      kernel_param->tcp_data_split,
					      extack))
		return -EINVAL;

	return mlx5e_ethtool_set_ringparam(priv, param, extack);

What happens if mlx5e_ethtool_set_ringparam() fails after this?

ethnl_set_rings() has already set cfg_pending->hds_config to unknown.
mlx5e_fix_features() reads cfg_pending, so it skips the "Keeping HW-GRO
enabled" override. If the user had earlier turned rx-gro-hw off while hds
kept it on, set_feature_hw_gro(false) switches packet_merge from SHAMPO to
none.

A request like "ethtool -G eth0 tcp-data-split auto rx 1" then fails the
minimum rx size check in mlx5e_ethtool_set_ringparam().
mlx5e_validate_params() or mlx5e_safe_switch_params() can fail at the
same point too. ethnl_default_set_doit() skips the swap when ret < 0:

	if (ret < 0)
		goto out_ops;

	swap(dev->cfg, dev->cfg_pending);

So dev->cfg->hds_config stays enabled while the device no longer splits
headers. GET still reports enabled, single-buffer XDP stays blocked with
-EBUSY, and the core's hds_config check for memory providers passes. The
next unrelated netdev_update_features() forces GRO_HW back on.

The disabled path had the same ordering before, and this patch now
rejects that request, so the trigger has moved to "auto". Could the
feature update happen only after the ring parameters are applied
successfully?

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