From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id AEDC5A00BE; Wed, 8 Jul 2020 05:37:20 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0E5721DD62; Wed, 8 Jul 2020 05:37:19 +0200 (CEST) Received: from huawei.com (szxga06-in.huawei.com [45.249.212.32]) by dpdk.org (Postfix) with ESMTP id CC7E51DD48 for ; Wed, 8 Jul 2020 05:37:16 +0200 (CEST) Received: from DGGEMS408-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id 7B7ECB68F7C021E09F28; Wed, 8 Jul 2020 11:37:15 +0800 (CST) Received: from [10.69.31.206] (10.69.31.206) by DGGEMS408-HUB.china.huawei.com (10.3.19.208) with Microsoft SMTP Server id 14.3.487.0; Wed, 8 Jul 2020 11:37:06 +0800 To: Thomas Monjalon References: <1594019191-54524-1-git-send-email-xavier.huwei@huawei.com> <1594019191-54524-3-git-send-email-xavier.huwei@huawei.com> <3633587.h79vMgjPB0@thomas> CC: , , , , , , , , From: "Wei Hu (Xavier)" Message-ID: <40e3a1b0-3c7e-1807-67e1-41dd2850e350@huawei.com> Date: Wed, 8 Jul 2020 11:37:06 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <3633587.h79vMgjPB0@thomas> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.69.31.206] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> Signed-off-by: Wei Hu (Xavier) >> Acked-by: Andrew Rybchenko >> Acked-by: Hyong Youb Kim >> Acked-by: Sachin Saxena >> Acked-by: Xiaoyun wang >> Acked-by: Harman Kalra > 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; >> + } > > > . >