Re: [dpdk-dev] [PATCH v14 4/7] drivers/net: update Rx RSS hash offload capabilities

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/29/19 8:03 AM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Add DEV_RX_OFFLOAD_RSS_HASH flag for all PMDs that support RSS hash
> delivery.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Reviewed-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

[snip]

> diff --git a/drivers/net/sfc/sfc_ef10_essb_rx.c b/drivers/net/sfc/sfc_ef10_essb_rx.c
> index 63da807ea..d9d2ce6bd 100644
> --- a/drivers/net/sfc/sfc_ef10_essb_rx.c
> +++ b/drivers/net/sfc/sfc_ef10_essb_rx.c
> @@ -715,7 +715,8 @@ struct sfc_dp_rx sfc_ef10_essb_rx = {
>   	},
>   	.features		= SFC_DP_RX_FEAT_FLOW_FLAG |
>   				  SFC_DP_RX_FEAT_FLOW_MARK,
> -	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM,
> +	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM |
> +				  DEV_RX_OFFLOAD_RSS_HASH,
>   	.queue_offload_capa	= 0,
>   	.get_dev_info		= sfc_ef10_essb_rx_get_dev_info,
>   	.pool_ops_supported	= sfc_ef10_essb_rx_pool_ops_supported,
> diff --git a/drivers/net/sfc/sfc_ef10_rx.c b/drivers/net/sfc/sfc_ef10_rx.c
> index f2fc6e70a..9e527b7fb 100644
> --- a/drivers/net/sfc/sfc_ef10_rx.c
> +++ b/drivers/net/sfc/sfc_ef10_rx.c
> @@ -796,7 +796,8 @@ struct sfc_dp_rx sfc_ef10_rx = {
>   	.features		= SFC_DP_RX_FEAT_MULTI_PROCESS |
>   				  SFC_DP_RX_FEAT_INTR,
>   	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM |
> -				  DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM,
> +				  DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
> +				  DEV_RX_OFFLOAD_RSS_HASH,
>   	.queue_offload_capa	= DEV_RX_OFFLOAD_SCATTER,
>   	.get_dev_info		= sfc_ef10_rx_get_dev_info,
>   	.qsize_up_rings		= sfc_ef10_rx_qsize_up_rings,
> diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
> index e6809bb64..4af8f1977 100644
> --- a/drivers/net/sfc/sfc_rx.c
> +++ b/drivers/net/sfc/sfc_rx.c
> @@ -617,7 +617,8 @@ struct sfc_dp_rx sfc_efx_rx = {
>   		.hw_fw_caps	= 0,
>   	},
>   	.features		= SFC_DP_RX_FEAT_INTR,
> -	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM,
> +	.dev_offload_capa	= DEV_RX_OFFLOAD_CHECKSUM |
> +				  DEV_RX_OFFLOAD_RSS_HASH,
>   	.queue_offload_capa	= DEV_RX_OFFLOAD_SCATTER,
>   	.qsize_up_rings		= sfc_efx_rx_qsize_up_rings,
>   	.qcreate		= sfc_efx_rx_qcreate,

Thanks  a lot, above looks good to me now.

> @@ -1556,6 +1557,9 @@ sfc_rx_check_mode(struct sfc_adapter *sa, struct rte_eth_rxmode *rxmode)
>   		rxmode->offloads |= DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM;
>   	}
>   
> +	if (!(rxmode->offloads & DEV_RX_OFFLOAD_RSS_HASH))
> +		rxmode->offloads |= DEV_RX_OFFLOAD_RSS_HASH;
> +

I've tried to highlight on my previous review notes, but it looks like not
that successful. The right condition should check that offload is really
supported:

if ((offloads_supported & DEV_RX_OFFLOAD_RSS_HASH) &&
     (~rxmode->offloads & DEV_RX_OFFLOAD_RSS_HASH))
           rxmode->offloads |= DEV_RX_OFFLOAD_RSS_HASH;

Basically similar to DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM above
(including style of disabled offload check just to have common style).

Thanks.

>   	return rc;
>   }
>

[snip]