Re: [dpdk-dev] [PATCH v2 5/7] net/mlx5: convert to new Rx offloads API

[Nelio Laranjeiro <nelio.laranjeiro@6wind.com>]

Hi Shahaf,

Please see bellow,

On Wed, Jan 03, 2018 at 09:16:15AM +0200, Shahaf Shuler wrote:
> Ethdev Rx offloads API has changed since:
> 
> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
> 
> This commit support the new Rx offloads API.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---

I have to remove my acked-by on this patch, there is an issue.

<snip/>
> +/**
> + * Checks if the per-queue offload configuration is valid.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + * @param offloads
> + *   Per-queue offloads configuration.
> + *
> + * @return
> + *   1 if the configuration is valid, 0 otherwise.
> + */
> +static int
> +priv_is_rx_queue_offloads_allowed(struct priv *priv, uint64_t offloads)
> +{
> +	uint64_t port_offloads = priv->dev->data->dev_conf.rxmode.offloads;
> +	uint64_t queue_supp_offloads =
> +		mlx5_priv_get_rx_queue_offloads(priv);
> +	uint64_t port_supp_offloads = mlx5_priv_get_rx_port_offloads(priv);
> +
> +	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> +	    offloads)
> +		return 0;
> +	if (((port_offloads ^ offloads) & port_supp_offloads))

port_offloads is wrongly named, it should be device_offloads and
this make things clearer.

Seems the mask is wrong as port_supp_offloads is a subset of
device_offloads.
For my understanding you want this statement to be true when the request
offloads is not supported, in such situation it should be a negative
mask i.e. ~(port_supp_offloads | queue_supp_offloads).

> +		return 0;
> +	return 1;
> +}

The same issue is present in Tx side.

Regards,

-- 
Nélio Laranjeiro
6WIND