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

["Wei Hu (Xavier)" <xavier.huwei@huawei.com>]

Hi, Thomas Monjalon


On 2020/7/7 22:11, Thomas Monjalon wrote:
> 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
The patch V2, V3, V4 had Cc'ed all the related maintainers.
I will cc the related mainters based on this V6 patch.
It's a good method, I will use it. Thanks.

>
>> @@ -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"?

The parameter offload_mask of rte_eth_dev_set_vlan_offload() function
includes some Rx VLAN offload, and some of them maybe are not enabled
in rte_eth_dev_configure().

Thanks, xavier

>
>> +	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;
>> +	}
>
>
> .
>