Re: [dpdk-dev] [PATCH V3 2/2] net/failsafe: convert to new Rx offloads API

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

My remarks for this one are mostly the same,
I will try to repeat them faithfully...

"convert to" -> "use"

On Wed, Jan 10, 2018 at 04:40:23PM +0200, Moti Haimovsky wrote:

...

> +static bool
> +fs_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)

function name can be shortened (s/_are//).

> +{
> +	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> +	uint64_t queue_supp_offloads = PRIV(dev)->infos.rx_queue_offload_capa;
> +	uint64_t port_supp_offloads = PRIV(dev)->infos.rx_offload_capa;
> +

Please separate variable definition and initialization.

> +	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> +	     offloads)
> +		return false;
> +	/* Verify we have no conflict with port offloads */
> +	if ((port_offloads ^ offloads) & port_supp_offloads)
> +		return false;
> +	return true;
> +}
> +
>  static void
>  fs_rx_queue_release(void *queue)
>  {
> @@ -290,6 +313,17 @@
>  		fs_rx_queue_release(rxq);
>  		dev->data->rx_queues[rx_queue_id] = NULL;
>  	}
> +	/* Verify application offloads are valid for our port and queue. */
> +	if (!fs_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
> +		rte_errno = ENOTSUP;
> +		ERROR("%p: Rx queue offloads 0x%lx don't match port "

Here, the device pointer should not be displayed.
It may be a larger issue with the fail-safe, that should be fixed (when
using multiple fail-safe instances), but for now, all outputs should
follow the same format.

Otherwise, use PRIx64 for displaying uint64_t.

> +		      "offloads 0x%lx or supported offloads 0x%lx",
> +		      (void *)dev, rx_conf->offloads,
> +		      dev->data->dev_conf.rxmode.offloads,
> +		      PRIV(dev)->infos.rx_offload_capa |
> +		      PRIV(dev)->infos.rx_queue_offload_capa);
> +		return -rte_errno;
> +	}
>  	rxq = rte_zmalloc(NULL,
>  			  sizeof(*rxq) +
>  			  sizeof(rte_atomic64_t) * PRIV(dev)->subs_tail,
> @@ -589,12 +623,16 @@
>  			   sizeof(default_infos));
>  	} else {
>  		uint32_t rx_offload_capa;
> +		uint32_t rxq_offload_capa;
>  
>  		rx_offload_capa = default_infos.rx_offload_capa;
> +		rxq_offload_capa = default_infos.rx_queue_offload_capa;
>  		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
>  			rte_eth_dev_info_get(PORT_ID(sdev),
>  					&PRIV(dev)->infos);
>  			rx_offload_capa &= PRIV(dev)->infos.rx_offload_capa;
> +			rxq_offload_capa &=
> +					PRIV(dev)->infos.rx_queue_offload_capa;
>  		}
>  		sdev = TX_SUBDEV(dev);
>  		rte_eth_dev_info_get(PORT_ID(sdev), &PRIV(dev)->infos);
> -- 
> 1.8.3.1
> 

Regards,
-- 
Gaëtan Rivet
6WIND