Re: [PATCH net-next V2 11/11] net/mlx5e: Support ethtool tcp-data-split settings

[Saeed Mahameed <saeed@kernel.org>]

On 22 May 15:55, Jakub Kicinski wrote:
>On Fri, 23 May 2025 00:41:26 +0300 Tariq Toukan wrote:
>> +	/* if HW GRO is not enabled due to external limitations but is wanted,
>> +	 * report HDS state as unknown so it won't get turned off explicitly.
>> +	 */
>> +	if (kernel_param->tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
>> +	    priv->netdev->wanted_features & NETIF_F_GRO_HW)
>> +		kernel_param->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
>
>The kernel_param->tcp_data_split here is the user config, right?
>It would be cleaner to not support setting SPLIT_DISABLED.
>Nothing requires that, you can support just setting AUTO and ENABLED.
>

I think I agree, AUTO might require some extra work on the driver side to
figure out current internal mode, but it actually makes more sense than
just doing "UNKNOWN", UKNOWN here means that HW GRO needs to be enabled
when disabling TCP HDR split, and we still don't know if that will work..

Cosmin will you look into this ? 

>> +
>
>nit: extra empty line, please run checkpatch
>
>>  }
>>
>>  static void mlx5e_get_ringparam(struct net_device *dev,
>> @@ -383,6 +391,43 @@ static void mlx5e_get_ringparam(struct net_device *dev,
>>  	mlx5e_ethtool_get_ringparam(priv, param, kernel_param);
>>  }
>>
>> +static bool mlx5e_ethtool_set_tcp_data_split(struct mlx5e_priv *priv,
>> +					     u8 tcp_data_split)
>> +{
>> +	bool enable = (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_ENABLED);
>> +	struct net_device *dev = priv->netdev;
>> +
>> +	if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
>> +		return true;
>> +
>> +	if (enable && !(dev->hw_features & NETIF_F_GRO_HW)) {
>> +		netdev_warn(dev, "TCP-data-split is not supported when GRO HW is not supported\n");
>> +		return false; /* GRO HW is not supported */
>> +	}
>> +
>> +	if (enable && (dev->features & NETIF_F_GRO_HW)) {
>> +		/* Already enabled */
>> +		dev->wanted_features |= NETIF_F_GRO_HW;
>> +		return true;
>> +	}
>> +
>> +	if (!enable && !(dev->features & NETIF_F_GRO_HW)) {
>> +		/* Already disabled */
>> +		dev->wanted_features &= ~NETIF_F_GRO_HW;
>> +		return true;
>> +	}
>> +
>> +	/* Try enable or disable GRO HW */
>> +	if (enable)
>> +		dev->wanted_features |= NETIF_F_GRO_HW;
>> +	else
>> +		dev->wanted_features &= ~NETIF_F_GRO_HW;
>
>Why are you modifying wanted_features? wanted_features is what
>*user space* wanted! You should probably operate on hw_features ?
>Tho, may be cleaner to return an error and an extack if the user
>tries to set HDS and GRO to conflicting values.
>

hw_features is hw capabilities, it doesn't mean on/off.. so no we can't
rely on that.

To enable TCP_DATA_SPLIT we tie it to GRO_HW, so we enable GRO_HW when
TCP_DATA_SPLIT is set to on and vise-versa. I agree not the cleanest.. 
But it is good for user-visibility as you would see both ON if you query
from user, which is the actual state. This is the only way to set HW_GRO
to on by driver and not lose previous state when we turn the other bit
on/off.

Yes, I guess such logic should be in the stack, although I don't see
anything wrong here in terms of correctness.