Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities

[Thomas Monjalon <thomas@monjalon.net>]

06/07/2020 09:06, Wei Hu (Xavier):
> Currently, there is a potential problem that calling the API function
> rte_eth_dev_set_vlan_offload to start VLAN hardware offloads which the
> driver does not support. If the PMD driver does not support certain VLAN
> hardware offloads and does not check for it, the hardware setting will
> not change, but the VLAN offloads in dev->data->dev_conf.rxmode.offloads
> will be turned on.
> 
> It is supposed to check the hardware capabilities to decide whether the
> relative callback needs to be called just like the behavior in the API
> function named rte_eth_dev_configure. And it is also needed to cleanup
> duplicated checks which are done in some PMDs. Also, note that it is
> behaviour change for some PMDs which simply ignore (with error/warning log
> message) unsupported VLAN offloads, but now it will fail.
> 
> Fixes: a4996bd89c42 ("ethdev: new Rx/Tx offloads API")
> Fixes: 0ebce6129bc6 ("net/dpaa2: support new ethdev offload APIs")
> Fixes: f9416bbafd98 ("net/enic: remove VLAN filter handler")
> Fixes: 4f7d9e383e5c ("fm10k: update vlan offload features")
> Fixes: fdba3bf15c7b ("net/hinic: add VLAN filter and offload")
> Fixes: b96fb2f0d22b ("net/i40e: handle QinQ strip")
> Fixes: d4a27a3b092a ("nfp: add basic features")
> Fixes: 56139e85abec ("net/octeontx: support VLAN filter offload")
> Fixes: ba1b3b081edf ("net/octeontx2: support VLAN offloads")
> Fixes: d87246a43759 ("net/qede: enable and disable VLAN filtering")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Hyong Youb Kim <hyonkim@cisco.com>
> Acked-by: Sachin Saxena <sachin.saxena@oss.nxp.com>
> Acked-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>
> Acked-by: Harman Kalra <hkalra@marvell.com>

Looks like a lot of reviews were already done. I missed this patch.
Please could you make sure API maintainers are Cc'ed?
You can use --cc-cmd devtools/get-maintainer.sh

> @@ -3317,6 +3319,25 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask)
>  	if (mask == 0)
>  		return ret;
>  
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	/*
> +	 * New added Rx VLAN offloading which are not enabled in
> +	 * rte_eth_dev_configure() must be within its device capabilities
> +	 */

What means "New added Rx VLAN offloading"?

> +	if ((dev_offloads & dev_info.rx_offload_capa) != dev_offloads) {
> +		new_offloads = dev_offloads & ~orig_offloads;
> +		RTE_ETHDEV_LOG(ERR,
> +			"Ethdev port_id=%u requested new added VLAN offloads "
> +			"0x%" PRIx64 " must be within Rx offloads capabilities "
> +			"0x%" PRIx64 " in %s()\n",
> +			port_id, new_offloads, dev_info.rx_offload_capa,
> +			__func__);
> +		return -EINVAL;
> +	}