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

[sashiko-bot@kernel.org]

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")

> 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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111170558.1654691-1-dimitri.daskalakis1@gmail.com?part=1