[dpdk-dev] [PATCH v3 1/2] net/dpaa: fix the ethdev offload checks

[Ferruh Yigit]

On 4/24/2018 4:06 PM, Hemant Agrawal wrote:
> From: Sunil Kumar Kori <sunil.kori at nxp.com>
> 
> Fixes: 16e2c27f4fc7 ("net/dpaa: support new ethdev offload APIs")
> 
> Signed-off-by: Sunil Kumar Kori <sunil.kori at nxp.com>
> ---
>  drivers/net/dpaa/dpaa_ethdev.c | 89 +++++++++++++++++++++++++++---------------
>  1 file changed, 57 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
> index b2740b4..32d36f2 100644
> --- a/drivers/net/dpaa/dpaa_ethdev.c
> +++ b/drivers/net/dpaa/dpaa_ethdev.c
> @@ -45,6 +45,33 @@
>  #include <fsl_bman.h>
>  #include <fsl_fman.h>
>  
> +/* Supported Rx offloads */
> +static uint64_t dev_rx_offloads_sup =
> +		DEV_RX_OFFLOAD_JUMBO_FRAME;
> +
> +/* Rx offloads which cannot be disabled */
> +static uint64_t dev_rx_offloads_nodis =
> +		DEV_RX_OFFLOAD_IPV4_CKSUM |
> +		DEV_RX_OFFLOAD_UDP_CKSUM |
> +		DEV_RX_OFFLOAD_TCP_CKSUM |
> +		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
> +		DEV_RX_OFFLOAD_CRC_STRIP |
> +		DEV_RX_OFFLOAD_SCATTER;
> +
> +/* Supported Tx offloads */
> +static uint64_t dev_tx_offloads_sup;
> +
> +/* Tx offloads which cannot be disabled */
> +static uint64_t dev_tx_offloads_nodis =
> +		DEV_TX_OFFLOAD_IPV4_CKSUM |
> +		DEV_TX_OFFLOAD_UDP_CKSUM |
> +		DEV_TX_OFFLOAD_TCP_CKSUM |
> +		DEV_TX_OFFLOAD_SCTP_CKSUM |
> +		DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM |
> +		DEV_TX_OFFLOAD_MULTI_SEGS |
> +		DEV_TX_OFFLOAD_MT_LOCKFREE |
> +		DEV_TX_OFFLOAD_MBUF_FAST_FREE;
> +
>  /* Keep track of whether QMAN and BMAN have been globally initialized */
>  static int is_global_init;
>  /* At present we only allow up to 4 push mode queues - as each of this queue
> @@ -143,35 +170,41 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
>  {
>  	struct dpaa_if *dpaa_intf = dev->data->dev_private;
>  	struct rte_eth_conf *eth_conf = &dev->data->dev_conf;
> -	struct rte_eth_dev_info dev_info;
>  	uint64_t rx_offloads = eth_conf->rxmode.offloads;
>  	uint64_t tx_offloads = eth_conf->txmode.offloads;
>  
>  	PMD_INIT_FUNC_TRACE();
>  
> -	dpaa_eth_dev_info(dev, &dev_info);
> -	if (((~(dev_info.rx_offload_capa) & rx_offloads) != 0)) {
> -		DPAA_PMD_ERR("Some Rx offloads are not supported "
> -			"requested 0x%" PRIx64 " supported 0x%" PRIx64,
> -			rx_offloads, dev_info.rx_offload_capa);
> -		return -ENOTSUP;
> +	/* Rx offloads validation */
> +	if (dev_rx_offloads_nodis & ~rx_offloads) {
> +		DPAA_PMD_WARN(
> +		"Rx offloads non configurable - requested 0x%" PRIx64
> +		" ignored 0x%" PRIx64,
> +			rx_offloads, dev_rx_offloads_nodis);
>  	}
> -
> -	if (((~(dev_info.tx_offload_capa) & tx_offloads) != 0)) {
> -		DPAA_PMD_ERR("Some Tx offloads are not supported "
> -			"requested 0x%" PRIx64 " supported 0x%" PRIx64,
> -			tx_offloads, dev_info.tx_offload_capa);
> +	if (~(dev_rx_offloads_sup | dev_rx_offloads_nodis) & rx_offloads) {
> +		DPAA_PMD_ERR(
> +		"Rx offloads non supported - requested 0x%" PRIx64
> +		" supported 0x%" PRIx64,
> +			rx_offloads,
> +			dev_rx_offloads_sup | dev_rx_offloads_nodis);
>  		return -ENOTSUP;
>  	}

Hi Hemant,

Overall this looks good to me, thanks.

Only I would like to ask if you prefer to replace nodis and not_supported checks.

Because with current order, if an offlaod requested that both has not supported
offload and not enable all nodis offloads, this will print both logs and return
error. Since it will return error, do you really need "non configurable" log?

If you replace checks, if any not supported offload requested it will only print
log for it and return error without checking/caring nodis offloads.

It is up to you, please let me know if you want to go with existing set.

Thanks,
ferruh