Re: [PATCH net-next v3 2/7] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski <kuba@kernel.org>]

On Thu,  3 Oct 2024 16:06:15 +0000 Taehee Yoo wrote:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index fdecdf8894b3..e9ef65dd2e7b 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -829,12 +829,16 @@ static void bnxt_get_ringparam(struct net_device *dev,
>  	if (bp->flags & BNXT_FLAG_AGG_RINGS) {
>  		ering->rx_max_pending = BNXT_MAX_RX_DESC_CNT_JUM_ENA;
>  		ering->rx_jumbo_max_pending = BNXT_MAX_RX_JUM_DESC_CNT;
> -		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
>  	} else {
>  		ering->rx_max_pending = BNXT_MAX_RX_DESC_CNT;
>  		ering->rx_jumbo_max_pending = 0;
> -		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
>  	}
> +
> +	if (bp->flags & BNXT_FLAG_HDS)
> +		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
> +	else
> +		kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;

This breaks previous behavior. The HDS reporting from get was
introduced to signal to user space whether the page flip based
TCP zero-copy (the one added some years ago not the recent one)
will be usable with this NIC.

When HW-GRO is enabled HDS will be working.

I think that the driver should only track if the user has set the value
to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS
to disabled is not useful, don't support it.

>  	ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
>  
>  	ering->rx_pending = bp->rx_ring_size;
> @@ -854,9 +858,25 @@ static int bnxt_set_ringparam(struct net_device *dev,
>  	    (ering->tx_pending < BNXT_MIN_TX_DESC_CNT))
>  		return -EINVAL;
>  
> +	if (kernel_ering->tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
> +	    BNXT_RX_PAGE_MODE(bp)) {
> +		NL_SET_ERR_MSG_MOD(extack, "tcp-data-split can not be enabled with XDP");
> +		return -EINVAL;
> +	}

Technically just if the XDP does not support multi-buffer.
Any chance we could do this check in the core?