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

[Dimitri Daskalakis <dimitri.daskalakis1@gmail.com>]

On 10/9/26 7:59 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
> 
> 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] The patch is titled "advertise tcp-data-split support" and
>   says it will "Add back tcp-data-split reporting", but…
> - [Medium] On an mlx5 device with rx-gro-hw on,
>   mlx5e_ethtool_get_ringparam() now reports tcp-data-split='enabled'…
> - [Low] The commit message says "This is not a fix" and carries no Fixes:
>   tag, but the patch fixes real defects that the message itself…
> - [Low] With rx-gro-hw on and dev->cfg->hds_config == UNKNOWN,
>   mlx5e_ethtool_get_ringparam() now reports tcp-data-split=enabled from…
> 
> --
> 
> net/mlx5e: advertise tcp-data-split support
> 
> This commit brings back tcp-data-split reporting in
> mlx5e_ethtool_get_ringparam() when SHAMPO is active. It also rejects
> requests to disable tcp-data-split. Finally, it moves
> netdev_update_features() so that it only runs after
> mlx5e_ethtool_set_ringparam() succeeds.
> 
>> This is not a fix since HDS/tcp-data-split is still functional with HW
>> GRO enabled.
> 
> [Severity: Low]
> Is this accurate? The patch seems to fix problems that the commit message
> itself describes:
> 
>   - the tcp-data-split reporting that 46bcce5dfd33 dropped from ethtool -g
>   - the KeyError in hds.set_hds_enable/disable since d347b28c492e
>   - the feature/config de-sync caused by calling netdev_update_features()
>     before mlx5e_ethtool_set_ringparam()
> 
> For the last one: in the baseline, mlx5e_ethtool_set_tcp_data_split()
> called netdev_update_features() before the ring sizes were validated.
> mlx5e_fix_features() looks at netdev->cfg_pending:
> 
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_fix_features() {
>     ...
> 	if (cfg && cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> 	    !(features & NETIF_F_GRO_HW)) {
>     ...
> }
> 
> Take a request like "ethtool -G ethX rx 1 tcp-data-split off" while
> hds_config is ENABLED. HW-GRO/SHAMPO could be turned off first. The
> rx_pending minimum check then fails, and the ethtool core discards
> cfg_pending. dev->cfg->hds_config stays ENABLED, but the HW is no longer
> splitting.
> 
> There is no Fixes: tag, so stable tooling won't pick these fixes up for
> kernels that contain 46bcce5dfd33 and d347b28c492e. Could the reporting
> restore and the netdev_update_features() reordering be split into their
> own patches with Fixes: tags? The uAPI behavior change that rejects
> tcp-data-split off could then stay separate.
> 

The lack of tcp-data-split reporting that d347b28c492e introduced does not
cause any user facing issues, so this does not warrant a standalone Fix.

The early call to netdev_update_features() is a real bug. A user can get
themselves into a state where all ring changes are blocked. I've been able
to verify this on a 6.16 kernel. Fortunately this state is not terminal,
users can recover by setting rx-gro-hw on. I'll send the Fix to net.

>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> index 261c466a4d362..c30d751cac2fa 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> @@ -378,6 +378,9 @@ void mlx5e_ethtool_get_ringparam(struct mlx5e_priv *priv,
>>  
>>  	kernel_param->hds_thresh = 0;
>>  	kernel_param->hds_thresh_max = 0;
>> +
>> +	if (priv->channels.params.packet_merge.type == MLX5E_PACKET_MERGE_SHAMPO)
>> +		kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
>>  }
> 
> [Severity: Medium]
> The subject says "advertise tcp-data-split support" and the message says
> "Add back tcp-data-split reporting". However, this only ever writes
> ENABLED, and only when SHAMPO is active. The code removed by 46bcce5dfd33
> also reported DISABLED when SHAMPO was not active.
> 
> HW-GRO is off by default on mlx5, since mlx5e_build_nic_netdev() clears
> NETIF_F_GRO_HW. In that case:
> 
>   - rings_prepare_data() pre-fills tcp_data_split from
>     dev->cfg->hds_config (UNKNOWN)
>   - the driver leaves it untouched
>   - rings_fill_reply() leaves out the attribute:
> 
> net/ethtool/rings.c:rings_fill_reply() {
>     ...
> 	    (kr->tcp_data_split &&
> 	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
>     ...
> }
> 
> So user space sees tcp-data-split as unsupported, even though the device
> advertises ETHTOOL_RING_USE_TCP_DATA_SPLIT and is in fact not splitting.
> The post-patch log in the changelog shows this:
> 
>   ok 1 hds.get_hds # SKIP tcp-data-split not supported by device
> 
> mlx5e_ethtool_set_tcp_data_split() now also rejects
> ETHTOOL_TCP_DATA_SPLIT_DISABLED unconditionally. On a default-configured
> device, the off state can then be neither reported nor requested.
> 
> Would it be safe to report ETHTOOL_TCP_DATA_SPLIT_DISABLED when SHAMPO is
> not active? Both ethtool_ringparam_get_cfg() and the netlink set path
> replace the driver-reported value with dev->cfg->hds_config.
> 

It's safe to report ETHTOOL_TCP_DATA_SPLIT_DISABLED based on the SHAMPO
state. The set path safely ignores what get returns. Will update this in
the next version.

> [Severity: Low]
> With rx-gro-hw on and dev->cfg->hds_config == UNKNOWN, this now reports
> enabled. Can hds.set_hds_enable then leave hds_config stuck at ENABLED
> after the test finishes?
> 
> _defer_reset_hds() saves tcp-data-split as 'enabled' (from SHAMPO state)
> set_hds_enable() writes tcp-data-split 'enabled'
>   ethnl_set_rings()
>     dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
> _hds_reset() at cleanup:
>     if cur.get('tcp-data-split') != rings.get('tcp-data-split'):
> 
> Both values are 'enabled', so 'unknown' is never written back.
> 
> After that, mlx5e_fix_features() keeps NETIF_F_GRO_HW forced on, and
> "ethtool -K rx-gro-hw off" no longer takes effect. dev_xdp_attach() also
> rejects single-buffer XDP:
> 
> 	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
> 	    prog && !prog->aux->xdp_has_frags) {
> 
> In a full hds.py run, the later set_xdp tests write 'unknown' and hide
> this. It shows up when set_hds_enable runs alone, or as the last test that
> touches HDS.
> 
> The root cause is that the test compares reported state rather than
> config, and netdevsim has the same pattern. This patch is what makes it
> reachable on mlx5, though. Should _hds_reset() or the driver reporting
> be adjusted?
> 

This issue is unique to mlx5 since tcp-data-split is tied to HW-GRO. We
shouldn't modify a generic test to account for this coupling.

> [Severity: Medium]
> On mlx5 with rx-gro-hw on, do hds.set_xdp and hds.ioctl_set_xdp now go
> from SKIP to FAIL?
> 
> Both tests see mode 'enabled', write 'unknown', and then attach a
> single-buffer XDP program:
> 
> tools/testing/selftests/drivers/net/hds.py:set_xdp() {
>     ...
>     if mode == 'enabled':
>         _defer_reset_hds(cfg, netnl)
>         netnl.rings_set({'header': {'dev-index': cfg.ifindex},
>                          'tcp-data-split': 'unknown'})
> 
>     _xdp_onoff(cfg)
> }
> 
> If cfg is already UNKNOWN, ethnl_set_rings() returns 0 on !mod and the
> 'unknown' write does nothing. Otherwise it only clears the config, and
> GRO_HW stays wanted. Either way SHAMPO stays active:
> 
> mlx5e_xdp_set()
>   mlx5e_xdp_allowed()
> 	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;
> 
> The log in the changelog only covers the HW-GRO off case, where these
> tests are skipped. Was the HW-GRO on configuration tested as well?
> 
> [ ... ]
> 

It's fine for these test to fail given mlx5's HW-GRO and tcp-data-split
coupling.