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

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

On 11/12/26 9:06 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] net/mlx5e: missing HW-GRO cap check in
>   mlx5e_ethtool_set_tcp_data_split()
> - [Low] net/mlx5e: missing Fixes trailer for commit 46bcce5dfd33
> --
> 
> commit 10e87fc2f39f7859cabd43f8c342a731e4043353
> Author: Dimitri Daskalakis <daskald@meta.com>
> 
>> net/mlx5e: advertise tcp-data-split support
>>
>> 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.
>>
>> On mlx5, header-data split is performed by SHAMPO, which is used only
>> when HW-GRO is enabled. The driver already keeps HW-GRO on while
>> tcp-data-split is enabled, but nothing enforces the other direction.
>> A user can disable tcp-data-split and then enable rx-gro-hw, leaving
>> the device splitting while the config says it is off.
>>
>> To make the feature handling consistent, report tcp-data-split as on
>> when SHAMPO is active and off otherwise. Only allow disabling
>> tcp-data-split while HW-GRO is off, and keep HW-GRO off in
>> mlx5e_fix_features() when tcp-data-split is already disabled.
>>
>> Signed-off-by: Dimitri Daskalakis <daskald@meta.com>
> 
> [Severity: Low]
> As reported by Björn Töpel on v3 [1], should this include a Fixes tag for
> stable backports:
> 
>   Fixes: 46bcce5dfd33 ("net/mlx5e: Support ethtool tcp-data-split settings")
> 

The fix was for updating netdev features after a successful ring param
change. That was sent here [1].

As for the rest of this patch, it's more of a user behavior change than a
fix. The HW splitting capability still works when GRO is enabled, it's just
reporting to the stack/user that gets corrected.

[1]
https://lore.kernel.org/all/20261017005401.1582755-1-dimitri.daskalakis1@gmail.com/

>> 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;
>>  }
>>  
>>  static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
>> @@ -399,7 +406,14 @@ static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
>>  	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");
>> +				   "TCP-data-split can not be enabled without HW-GRO, set it to 'auto' and enable HW-GRO first");
>> +		return false;
>> +	}
>> +
>> +	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
>> +	    dev->features & NETIF_F_GRO_HW) {
>> +		NL_SET_ERR_MSG_MOD(extack,
>> +				   "TCP-data-split can not be disabled while HW-GRO is on, set it to 'auto' and disable HW-GRO first");
>>  		return false;
>>  	}
> 
> [Severity: Medium]
> Does mlx5e_ethtool_set_tcp_data_split() allow setting
> ETHTOOL_TCP_DATA_SPLIT_DISABLED on devices that do not support
> NETIF_F_GRO_HW?
> 
> On hardware without SHAMPO support, dev->features & NETIF_F_GRO_HW is
> always 0, so requesting ETHTOOL_TCP_DATA_SPLIT_DISABLED succeeds and saves
> that value in dev->cfg->hds_config.  Because rings_prepare_data() copies
> dev->cfg->hds_config into kernel_ringparam.tcp_data_split before calling
> mlx5e_get_ringparam(), and mlx5e_get_ringparam() leaves tcp_data_split
> untouched when NETIF_F_GRO_HW is not in dev->hw_features, subsequent
> ETHTOOL_MSG_RINGS_GET queries will report tcp-data-split as supported and
> disabled on unsupported hardware.  Requesting ETHTOOL_TCP_DATA_SPLIT_ENABLED
> on such devices also returns an extack telling the user to enable HW-GRO
> first when the device cannot support it.
> 
> Rejecting non-ETHTOOL_TCP_DATA_SPLIT_UNKNOWN values in
> mlx5e_ethtool_set_tcp_data_split() when !(dev->hw_features & NETIF_F_GRO_HW)
> would prevent this.
>

I don't think it's worth the extra code. Presumably these devices cannot
split anyway, so it's fine for a user to disable the feature.

> [1] https://lore.kernel.org/r/87a4ons7dd.fsf@all.your.base.are.belong.to.us
>