* [dpdk-dev] [PATCH v6 0/2] ethdev: minor bugfixes @ 2020-07-06 7:06 Wei Hu (Xavier) 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier) 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier) 0 siblings, 2 replies; 12+ messages in thread From: Wei Hu (Xavier) @ 2020-07-06 7:06 UTC (permalink / raw) To: dev; +Cc: xavier.huwei This series are minor bugfixes for rte_ethdev.c. Wei Hu (Xavier) (2): ethdev: fix data room size verification in Rx queue setup ethdev: fix VLAN offloads set if no relative capabilities drivers/net/dpaa2/dpaa2_ethdev.c | 12 +++--------- drivers/net/enic/enic_ethdev.c | 12 ------------ drivers/net/fm10k/fm10k_ethdev.c | 23 ++--------------------- drivers/net/hinic/hinic_pmd_ethdev.c | 6 ------ drivers/net/i40e/i40e_ethdev.c | 5 ----- drivers/net/nfp/nfp_net.c | 5 ----- drivers/net/octeontx/octeontx_ethdev_ops.c | 10 ---------- drivers/net/octeontx2/otx2_vlan.c | 5 ----- drivers/net/qede/qede_ethdev.c | 3 --- lib/librte_ethdev/rte_ethdev.c | 23 ++++++++++++++++++++++- 10 files changed, 27 insertions(+), 77 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in Rx queue setup 2020-07-06 7:06 [dpdk-dev] [PATCH v6 0/2] ethdev: minor bugfixes Wei Hu (Xavier) @ 2020-07-06 7:06 ` Wei Hu (Xavier) 2020-07-07 13:37 ` Slava Ovsiienko 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier) 1 sibling, 1 reply; 12+ messages in thread From: Wei Hu (Xavier) @ 2020-07-06 7:06 UTC (permalink / raw) To: dev; +Cc: xavier.huwei In the rte_eth_rx_queue_setup API function, the local variable named mbp_buf_size, which is the data room size of the input parameter mp, is checked to guarantee that each memory chunck used for net device in the mbuf is bigger than the min_rx_bufsize. But if mbp_buf_size is less than RTE_PKTMBUF_HEADROOM, the value of the following statement will be a large number since the mbp_buf_size is a unsigned value. mbp_buf_size - RTE_PKTMBUF_HEADROOM As a result, it will cause a segment fault in this situation. This patch fixes it by modify the check condition to guarantee that the local varibale named mbp_buf_size is bigger than RTE_PKTMBUF_HEADROOM. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com> Acked-by: Sachin Saxena <sachin.saxena@oss.nxp.com> --- v2 -> v6: No change. v1 -> v2: Simplify the check condition of mbp_buf_size according to Andrew Rybchenko's comment. --- lib/librte_ethdev/rte_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index d06b7f9..50c3f18 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -1820,7 +1820,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, } mbp_buf_size = rte_pktmbuf_data_room_size(mp); - if ((mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) { + if (mbp_buf_size < dev_info.min_rx_bufsize + RTE_PKTMBUF_HEADROOM) { RTE_ETHDEV_LOG(ERR, "%s mbuf_data_room_size %d < %d (RTE_PKTMBUF_HEADROOM=%d + min_rx_bufsize(dev)=%d)\n", mp->name, (int)mbp_buf_size, -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in Rx queue setup 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier) @ 2020-07-07 13:37 ` Slava Ovsiienko 0 siblings, 0 replies; 12+ messages in thread From: Slava Ovsiienko @ 2020-07-07 13:37 UTC (permalink / raw) To: Wei Hu (Xavier), dev Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier) > Sent: Monday, July 6, 2020 10:07 > To: dev@dpdk.org > Cc: xavier.huwei@huawei.com > Subject: [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in > Rx queue setup > > In the rte_eth_rx_queue_setup API function, the local variable named > mbp_buf_size, which is the data room size of the input parameter mp, is > checked to guarantee that each memory chunck used for net device in the > mbuf is bigger than the min_rx_bufsize. But if mbp_buf_size is less than > RTE_PKTMBUF_HEADROOM, the value of the following statement will be a > large number since the mbp_buf_size is a unsigned value. > mbp_buf_size - RTE_PKTMBUF_HEADROOM > As a result, it will cause a segment fault in this situation. > > This patch fixes it by modify the check condition to guarantee that the local > varibale named mbp_buf_size is bigger than RTE_PKTMBUF_HEADROOM. > > Fixes: af75078fece3 ("first public release") > Cc: stable@dpdk.org > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com> > Acked-by: Sachin Saxena <sachin.saxena@oss.nxp.com> > --- > v2 -> v6: No change. > v1 -> v2: Simplify the check condition of mbp_buf_size according to > Andrew Rybchenko's comment. > --- > lib/librte_ethdev/rte_ethdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index d06b7f9..50c3f18 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -1820,7 +1820,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, > uint16_t rx_queue_id, > } > mbp_buf_size = rte_pktmbuf_data_room_size(mp); > > - if ((mbp_buf_size - RTE_PKTMBUF_HEADROOM) < > dev_info.min_rx_bufsize) { > + if (mbp_buf_size < dev_info.min_rx_bufsize + > RTE_PKTMBUF_HEADROOM) { > RTE_ETHDEV_LOG(ERR, > "%s mbuf_data_room_size %d < %d > (RTE_PKTMBUF_HEADROOM=%d + min_rx_bufsize(dev)=%d)\n", > mp->name, (int)mbp_buf_size, > -- > 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-06 7:06 [dpdk-dev] [PATCH v6 0/2] ethdev: minor bugfixes Wei Hu (Xavier) 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier) @ 2020-07-06 7:06 ` Wei Hu (Xavier) 2020-07-07 7:35 ` Jeff Guo 2020-07-07 14:11 ` Thomas Monjalon 1 sibling, 2 replies; 12+ messages in thread From: Wei Hu (Xavier) @ 2020-07-06 7:06 UTC (permalink / raw) To: dev; +Cc: xavier.huwei 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> --- v5 -> v6: add the related history patch into the Fixes commit log. v4 -> v5: no change. v3 -> v4: Delete "next_mask" label and modify the function that when the offload is not supported the function fail. v2 -> v3: Add __rte_unused to avoid unused parameter 'dev' and 'mask' warning. v1 -> v2: Cleanup duplicated checks which are done in some PMDs. --- drivers/net/dpaa2/dpaa2_ethdev.c | 12 +++--------- drivers/net/enic/enic_ethdev.c | 12 ------------ drivers/net/fm10k/fm10k_ethdev.c | 23 ++--------------------- drivers/net/hinic/hinic_pmd_ethdev.c | 6 ------ drivers/net/i40e/i40e_ethdev.c | 5 ----- drivers/net/nfp/nfp_net.c | 5 ----- drivers/net/octeontx/octeontx_ethdev_ops.c | 10 ---------- drivers/net/octeontx2/otx2_vlan.c | 5 ----- drivers/net/qede/qede_ethdev.c | 3 --- lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ 10 files changed, 26 insertions(+), 76 deletions(-) diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c index a1f1919..489d744 100644 --- a/drivers/net/dpaa2/dpaa2_ethdev.c +++ b/drivers/net/dpaa2/dpaa2_ethdev.c @@ -145,7 +145,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) { struct dpaa2_dev_priv *priv = dev->data->dev_private; struct fsl_mc_io *dpni = dev->process_private; - int ret; + int ret = 0; PMD_INIT_FUNC_TRACE(); @@ -153,7 +153,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) /* VLAN Filter not avaialble */ if (!priv->max_vlan_filters) { DPAA2_PMD_INFO("VLAN filter not available"); - goto next_mask; + return -ENOTSUP; } if (dev->data->dev_conf.rxmode.offloads & @@ -166,14 +166,8 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) if (ret < 0) DPAA2_PMD_INFO("Unable to set vlan filter = %d", ret); } -next_mask: - if (mask & ETH_VLAN_EXTEND_MASK) { - if (dev->data->dev_conf.rxmode.offloads & - DEV_RX_OFFLOAD_VLAN_EXTEND) - DPAA2_PMD_INFO("VLAN extend offload not supported"); - } - return 0; + return ret; } static int diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c index 6a3580f..30a599d 100644 --- a/drivers/net/enic/enic_ethdev.c +++ b/drivers/net/enic/enic_ethdev.c @@ -367,18 +367,6 @@ static int enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask) enic->ig_vlan_strip_en = 0; } - if ((mask & ETH_VLAN_FILTER_MASK) && - (offloads & DEV_RX_OFFLOAD_VLAN_FILTER)) { - dev_warning(enic, - "Configuration of VLAN filter is not supported\n"); - } - - if ((mask & ETH_VLAN_EXTEND_MASK) && - (offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)) { - dev_warning(enic, - "Configuration of extended VLAN is not supported\n"); - } - return enic_set_vlan_strip(enic); } diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c index cb0dd3b..b574693 100644 --- a/drivers/net/fm10k/fm10k_ethdev.c +++ b/drivers/net/fm10k/fm10k_ethdev.c @@ -1562,28 +1562,9 @@ fm10k_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on) } static int -fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask) +fm10k_vlan_offload_set(struct rte_eth_dev *dev __rte_unused, + int mask __rte_unused) { - if (mask & ETH_VLAN_STRIP_MASK) { - if (!(dev->data->dev_conf.rxmode.offloads & - DEV_RX_OFFLOAD_VLAN_STRIP)) - PMD_INIT_LOG(ERR, "VLAN stripping is " - "always on in fm10k"); - } - - if (mask & ETH_VLAN_EXTEND_MASK) { - if (dev->data->dev_conf.rxmode.offloads & - DEV_RX_OFFLOAD_VLAN_EXTEND) - PMD_INIT_LOG(ERR, "VLAN QinQ is not " - "supported in fm10k"); - } - - if (mask & ETH_VLAN_FILTER_MASK) { - if (!(dev->data->dev_conf.rxmode.offloads & - DEV_RX_OFFLOAD_VLAN_FILTER)) - PMD_INIT_LOG(ERR, "VLAN filter is always on in fm10k"); - } - return 0; } diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c b/drivers/net/hinic/hinic_pmd_ethdev.c index 0d3d62b..67e6afc 100644 --- a/drivers/net/hinic/hinic_pmd_ethdev.c +++ b/drivers/net/hinic/hinic_pmd_ethdev.c @@ -1709,12 +1709,6 @@ static int hinic_vlan_offload_set(struct rte_eth_dev *dev, int mask) nic_dev->proc_dev_name, dev->data->port_id); } - if (mask & ETH_VLAN_EXTEND_MASK) { - PMD_DRV_LOG(ERR, "Don't support vlan qinq, device: %s, port_id: %d", - nic_dev->proc_dev_name, dev->data->port_id); - return -ENOTSUP; - } - return 0; } diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c index 472ce2a..c89290d 100644 --- a/drivers/net/i40e/i40e_ethdev.c +++ b/drivers/net/i40e/i40e_ethdev.c @@ -3865,11 +3865,6 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask) struct i40e_vsi *vsi = pf->main_vsi; struct rte_eth_rxmode *rxmode; - if (mask & ETH_QINQ_STRIP_MASK) { - PMD_DRV_LOG(ERR, "Strip qinq is not supported."); - return -ENOTSUP; - } - rxmode = &dev->data->dev_conf.rxmode; if (mask & ETH_VLAN_FILTER_MASK) { if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c index 88e3f01..474719b 100644 --- a/drivers/net/nfp/nfp_net.c +++ b/drivers/net/nfp/nfp_net.c @@ -2353,11 +2353,6 @@ nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask) hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private); new_ctrl = 0; - if ((mask & ETH_VLAN_FILTER_OFFLOAD) || - (mask & ETH_VLAN_EXTEND_OFFLOAD)) - PMD_DRV_LOG(INFO, "No support for ETH_VLAN_FILTER_OFFLOAD or" - " ETH_VLAN_EXTEND_OFFLOAD"); - /* Enable vlan strip if it is not configured yet */ if ((mask & ETH_VLAN_STRIP_OFFLOAD) && !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)) diff --git a/drivers/net/octeontx/octeontx_ethdev_ops.c b/drivers/net/octeontx/octeontx_ethdev_ops.c index ff627a6..dbe13ce 100644 --- a/drivers/net/octeontx/octeontx_ethdev_ops.c +++ b/drivers/net/octeontx/octeontx_ethdev_ops.c @@ -43,16 +43,6 @@ octeontx_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask) rxmode = &dev->data->dev_conf.rxmode; - if (mask & ETH_VLAN_EXTEND_MASK) { - octeontx_log_err("Extend offload not supported"); - return -ENOTSUP; - } - - if (mask & ETH_VLAN_STRIP_MASK) { - octeontx_log_err("VLAN strip offload not supported"); - return -ENOTSUP; - } - if (mask & ETH_VLAN_FILTER_MASK) { if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) { rc = octeontx_vlan_hw_filter(nic, true); diff --git a/drivers/net/octeontx2/otx2_vlan.c b/drivers/net/octeontx2/otx2_vlan.c index 322a565..7357b06 100644 --- a/drivers/net/octeontx2/otx2_vlan.c +++ b/drivers/net/octeontx2/otx2_vlan.c @@ -717,11 +717,6 @@ otx2_nix_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask) rxmode = ð_dev->data->dev_conf.rxmode; - if (mask & ETH_VLAN_EXTEND_MASK) { - otx2_err("Extend offload not supported"); - return -ENOTSUP; - } - if (mask & ETH_VLAN_STRIP_MASK) { if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) { offloads |= DEV_RX_OFFLOAD_VLAN_STRIP; diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c index 7f5a34b..f2ed03b 100644 --- a/drivers/net/qede/qede_ethdev.c +++ b/drivers/net/qede/qede_ethdev.c @@ -1031,9 +1031,6 @@ static int qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask) } } - if (mask & ETH_VLAN_EXTEND_MASK) - DP_ERR(edev, "Extend VLAN not supported\n"); - qdev->vlan_offload_mask = mask; DP_INFO(edev, "VLAN offload mask %d\n", mask); diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 50c3f18..078bb25 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -3258,12 +3258,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id, int rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) { + struct rte_eth_dev_info dev_info; struct rte_eth_dev *dev; int ret = 0; int mask = 0; int cur, org = 0; uint64_t orig_offloads; uint64_t dev_offloads; + uint64_t new_offloads; RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); dev = &rte_eth_devices[port_id]; @@ -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 + */ + 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; + } + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); dev->data->dev_conf.rxmode.offloads = dev_offloads; ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier) @ 2020-07-07 7:35 ` Jeff Guo 2020-07-07 13:38 ` Slava Ovsiienko 2020-07-07 14:11 ` Thomas Monjalon 1 sibling, 1 reply; 12+ messages in thread From: Jeff Guo @ 2020-07-07 7:35 UTC (permalink / raw) To: Wei Hu (Xavier), dev On 7/6/2020 3:06 PM, Wei Hu (Xavier) wrote: > 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> Acked-by: Jeff Guo <jia.guo@intel.com> > --- > v5 -> v6: add the related history patch into the Fixes commit log. > v4 -> v5: no change. > v3 -> v4: Delete "next_mask" label and modify the function that when the > offload is not supported the function fail. > v2 -> v3: Add __rte_unused to avoid unused parameter 'dev' and 'mask' > warning. > v1 -> v2: Cleanup duplicated checks which are done in some PMDs. > --- > drivers/net/dpaa2/dpaa2_ethdev.c | 12 +++--------- > drivers/net/enic/enic_ethdev.c | 12 ------------ > drivers/net/fm10k/fm10k_ethdev.c | 23 ++--------------------- > drivers/net/hinic/hinic_pmd_ethdev.c | 6 ------ > drivers/net/i40e/i40e_ethdev.c | 5 ----- > drivers/net/nfp/nfp_net.c | 5 ----- > drivers/net/octeontx/octeontx_ethdev_ops.c | 10 ---------- > drivers/net/octeontx2/otx2_vlan.c | 5 ----- > drivers/net/qede/qede_ethdev.c | 3 --- > lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ > 10 files changed, 26 insertions(+), 76 deletions(-) > > diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c > index a1f1919..489d744 100644 > --- a/drivers/net/dpaa2/dpaa2_ethdev.c > +++ b/drivers/net/dpaa2/dpaa2_ethdev.c > @@ -145,7 +145,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) > { > struct dpaa2_dev_priv *priv = dev->data->dev_private; > struct fsl_mc_io *dpni = dev->process_private; > - int ret; > + int ret = 0; > > PMD_INIT_FUNC_TRACE(); > > @@ -153,7 +153,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) > /* VLAN Filter not avaialble */ > if (!priv->max_vlan_filters) { > DPAA2_PMD_INFO("VLAN filter not available"); > - goto next_mask; > + return -ENOTSUP; > } > > if (dev->data->dev_conf.rxmode.offloads & > @@ -166,14 +166,8 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) > if (ret < 0) > DPAA2_PMD_INFO("Unable to set vlan filter = %d", ret); > } > -next_mask: > - if (mask & ETH_VLAN_EXTEND_MASK) { > - if (dev->data->dev_conf.rxmode.offloads & > - DEV_RX_OFFLOAD_VLAN_EXTEND) > - DPAA2_PMD_INFO("VLAN extend offload not supported"); > - } > > - return 0; > + return ret; > } > > static int > diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c > index 6a3580f..30a599d 100644 > --- a/drivers/net/enic/enic_ethdev.c > +++ b/drivers/net/enic/enic_ethdev.c > @@ -367,18 +367,6 @@ static int enicpmd_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask) > enic->ig_vlan_strip_en = 0; > } > > - if ((mask & ETH_VLAN_FILTER_MASK) && > - (offloads & DEV_RX_OFFLOAD_VLAN_FILTER)) { > - dev_warning(enic, > - "Configuration of VLAN filter is not supported\n"); > - } > - > - if ((mask & ETH_VLAN_EXTEND_MASK) && > - (offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)) { > - dev_warning(enic, > - "Configuration of extended VLAN is not supported\n"); > - } > - > return enic_set_vlan_strip(enic); > } > > diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c > index cb0dd3b..b574693 100644 > --- a/drivers/net/fm10k/fm10k_ethdev.c > +++ b/drivers/net/fm10k/fm10k_ethdev.c > @@ -1562,28 +1562,9 @@ fm10k_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on) > } > > static int > -fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask) > +fm10k_vlan_offload_set(struct rte_eth_dev *dev __rte_unused, > + int mask __rte_unused) > { > - if (mask & ETH_VLAN_STRIP_MASK) { > - if (!(dev->data->dev_conf.rxmode.offloads & > - DEV_RX_OFFLOAD_VLAN_STRIP)) > - PMD_INIT_LOG(ERR, "VLAN stripping is " > - "always on in fm10k"); > - } > - > - if (mask & ETH_VLAN_EXTEND_MASK) { > - if (dev->data->dev_conf.rxmode.offloads & > - DEV_RX_OFFLOAD_VLAN_EXTEND) > - PMD_INIT_LOG(ERR, "VLAN QinQ is not " > - "supported in fm10k"); > - } > - > - if (mask & ETH_VLAN_FILTER_MASK) { > - if (!(dev->data->dev_conf.rxmode.offloads & > - DEV_RX_OFFLOAD_VLAN_FILTER)) > - PMD_INIT_LOG(ERR, "VLAN filter is always on in fm10k"); > - } > - > return 0; > } > > diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c b/drivers/net/hinic/hinic_pmd_ethdev.c > index 0d3d62b..67e6afc 100644 > --- a/drivers/net/hinic/hinic_pmd_ethdev.c > +++ b/drivers/net/hinic/hinic_pmd_ethdev.c > @@ -1709,12 +1709,6 @@ static int hinic_vlan_offload_set(struct rte_eth_dev *dev, int mask) > nic_dev->proc_dev_name, dev->data->port_id); > } > > - if (mask & ETH_VLAN_EXTEND_MASK) { > - PMD_DRV_LOG(ERR, "Don't support vlan qinq, device: %s, port_id: %d", > - nic_dev->proc_dev_name, dev->data->port_id); > - return -ENOTSUP; > - } > - > return 0; > } > > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c > index 472ce2a..c89290d 100644 > --- a/drivers/net/i40e/i40e_ethdev.c > +++ b/drivers/net/i40e/i40e_ethdev.c > @@ -3865,11 +3865,6 @@ i40e_vlan_offload_set(struct rte_eth_dev *dev, int mask) > struct i40e_vsi *vsi = pf->main_vsi; > struct rte_eth_rxmode *rxmode; > > - if (mask & ETH_QINQ_STRIP_MASK) { > - PMD_DRV_LOG(ERR, "Strip qinq is not supported."); > - return -ENOTSUP; > - } > - > rxmode = &dev->data->dev_conf.rxmode; > if (mask & ETH_VLAN_FILTER_MASK) { > if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c > index 88e3f01..474719b 100644 > --- a/drivers/net/nfp/nfp_net.c > +++ b/drivers/net/nfp/nfp_net.c > @@ -2353,11 +2353,6 @@ nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask) > hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private); > new_ctrl = 0; > > - if ((mask & ETH_VLAN_FILTER_OFFLOAD) || > - (mask & ETH_VLAN_EXTEND_OFFLOAD)) > - PMD_DRV_LOG(INFO, "No support for ETH_VLAN_FILTER_OFFLOAD or" > - " ETH_VLAN_EXTEND_OFFLOAD"); > - > /* Enable vlan strip if it is not configured yet */ > if ((mask & ETH_VLAN_STRIP_OFFLOAD) && > !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)) > diff --git a/drivers/net/octeontx/octeontx_ethdev_ops.c b/drivers/net/octeontx/octeontx_ethdev_ops.c > index ff627a6..dbe13ce 100644 > --- a/drivers/net/octeontx/octeontx_ethdev_ops.c > +++ b/drivers/net/octeontx/octeontx_ethdev_ops.c > @@ -43,16 +43,6 @@ octeontx_dev_vlan_offload_set(struct rte_eth_dev *dev, int mask) > > rxmode = &dev->data->dev_conf.rxmode; > > - if (mask & ETH_VLAN_EXTEND_MASK) { > - octeontx_log_err("Extend offload not supported"); > - return -ENOTSUP; > - } > - > - if (mask & ETH_VLAN_STRIP_MASK) { > - octeontx_log_err("VLAN strip offload not supported"); > - return -ENOTSUP; > - } > - > if (mask & ETH_VLAN_FILTER_MASK) { > if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) { > rc = octeontx_vlan_hw_filter(nic, true); > diff --git a/drivers/net/octeontx2/otx2_vlan.c b/drivers/net/octeontx2/otx2_vlan.c > index 322a565..7357b06 100644 > --- a/drivers/net/octeontx2/otx2_vlan.c > +++ b/drivers/net/octeontx2/otx2_vlan.c > @@ -717,11 +717,6 @@ otx2_nix_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask) > > rxmode = ð_dev->data->dev_conf.rxmode; > > - if (mask & ETH_VLAN_EXTEND_MASK) { > - otx2_err("Extend offload not supported"); > - return -ENOTSUP; > - } > - > if (mask & ETH_VLAN_STRIP_MASK) { > if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) { > offloads |= DEV_RX_OFFLOAD_VLAN_STRIP; > diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c > index 7f5a34b..f2ed03b 100644 > --- a/drivers/net/qede/qede_ethdev.c > +++ b/drivers/net/qede/qede_ethdev.c > @@ -1031,9 +1031,6 @@ static int qede_vlan_offload_set(struct rte_eth_dev *eth_dev, int mask) > } > } > > - if (mask & ETH_VLAN_EXTEND_MASK) > - DP_ERR(edev, "Extend VLAN not supported\n"); > - > qdev->vlan_offload_mask = mask; > > DP_INFO(edev, "VLAN offload mask %d\n", mask); > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 50c3f18..078bb25 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -3258,12 +3258,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id, > int > rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) > { > + struct rte_eth_dev_info dev_info; > struct rte_eth_dev *dev; > int ret = 0; > int mask = 0; > int cur, org = 0; > uint64_t orig_offloads; > uint64_t dev_offloads; > + uint64_t new_offloads; > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > @@ -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 > + */ > + 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; > + } > + > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > dev->data->dev_conf.rxmode.offloads = dev_offloads; > ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-07 7:35 ` Jeff Guo @ 2020-07-07 13:38 ` Slava Ovsiienko 2020-07-08 3:53 ` Wei Hu (Xavier) 0 siblings, 1 reply; 12+ messages in thread From: Slava Ovsiienko @ 2020-07-07 13:38 UTC (permalink / raw) To: Jeff Guo, Wei Hu (Xavier), dev Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo > Sent: Tuesday, July 7, 2020 10:36 > To: Wei Hu (Xavier) <xavier.huwei@huawei.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no > relative capabilities > > > On 7/6/2020 3:06 PM, Wei Hu (Xavier) wrote: > > 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> > > > Acked-by: Jeff Guo <jia.guo@intel.com> > > > > --- > > v5 -> v6: add the related history patch into the Fixes commit log. > > v4 -> v5: no change. > > v3 -> v4: Delete "next_mask" label and modify the function that when the > > offload is not supported the function fail. > > v2 -> v3: Add __rte_unused to avoid unused parameter 'dev' and 'mask' > > warning. > > v1 -> v2: Cleanup duplicated checks which are done in some PMDs. > > --- > > drivers/net/dpaa2/dpaa2_ethdev.c | 12 +++--------- > > drivers/net/enic/enic_ethdev.c | 12 ------------ > > drivers/net/fm10k/fm10k_ethdev.c | 23 ++--------------------- > > drivers/net/hinic/hinic_pmd_ethdev.c | 6 ------ > > drivers/net/i40e/i40e_ethdev.c | 5 ----- > > drivers/net/nfp/nfp_net.c | 5 ----- > > drivers/net/octeontx/octeontx_ethdev_ops.c | 10 ---------- > > drivers/net/octeontx2/otx2_vlan.c | 5 ----- > > drivers/net/qede/qede_ethdev.c | 3 --- > > lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ > > 10 files changed, 26 insertions(+), 76 deletions(-) > > > > diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c > > b/drivers/net/dpaa2/dpaa2_ethdev.c > > index a1f1919..489d744 100644 > > --- a/drivers/net/dpaa2/dpaa2_ethdev.c > > +++ b/drivers/net/dpaa2/dpaa2_ethdev.c > > @@ -145,7 +145,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, > int mask) > > { > > struct dpaa2_dev_priv *priv = dev->data->dev_private; > > struct fsl_mc_io *dpni = dev->process_private; > > - int ret; > > + int ret = 0; > > > > PMD_INIT_FUNC_TRACE(); > > > > @@ -153,7 +153,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, > int mask) > > /* VLAN Filter not avaialble */ > > if (!priv->max_vlan_filters) { > > DPAA2_PMD_INFO("VLAN filter not available"); > > - goto next_mask; > > + return -ENOTSUP; > > } > > > > if (dev->data->dev_conf.rxmode.offloads & @@ -166,14 > +166,8 @@ > > dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) > > if (ret < 0) > > DPAA2_PMD_INFO("Unable to set vlan filter = %d", > ret); > > } > > -next_mask: > > - if (mask & ETH_VLAN_EXTEND_MASK) { > > - if (dev->data->dev_conf.rxmode.offloads & > > - DEV_RX_OFFLOAD_VLAN_EXTEND) > > - DPAA2_PMD_INFO("VLAN extend offload not > supported"); > > - } > > > > - return 0; > > + return ret; > > } > > > > static int > > diff --git a/drivers/net/enic/enic_ethdev.c > > b/drivers/net/enic/enic_ethdev.c index 6a3580f..30a599d 100644 > > --- a/drivers/net/enic/enic_ethdev.c > > +++ b/drivers/net/enic/enic_ethdev.c > > @@ -367,18 +367,6 @@ static int enicpmd_vlan_offload_set(struct > rte_eth_dev *eth_dev, int mask) > > enic->ig_vlan_strip_en = 0; > > } > > > > - if ((mask & ETH_VLAN_FILTER_MASK) && > > - (offloads & DEV_RX_OFFLOAD_VLAN_FILTER)) { > > - dev_warning(enic, > > - "Configuration of VLAN filter is not supported\n"); > > - } > > - > > - if ((mask & ETH_VLAN_EXTEND_MASK) && > > - (offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)) { > > - dev_warning(enic, > > - "Configuration of extended VLAN is not > supported\n"); > > - } > > - > > return enic_set_vlan_strip(enic); > > } > > > > diff --git a/drivers/net/fm10k/fm10k_ethdev.c > > b/drivers/net/fm10k/fm10k_ethdev.c > > index cb0dd3b..b574693 100644 > > --- a/drivers/net/fm10k/fm10k_ethdev.c > > +++ b/drivers/net/fm10k/fm10k_ethdev.c > > @@ -1562,28 +1562,9 @@ fm10k_vlan_filter_set(struct rte_eth_dev *dev, > uint16_t vlan_id, int on) > > } > > > > static int > > -fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask) > > +fm10k_vlan_offload_set(struct rte_eth_dev *dev __rte_unused, > > + int mask __rte_unused) > > { > > - if (mask & ETH_VLAN_STRIP_MASK) { > > - if (!(dev->data->dev_conf.rxmode.offloads & > > - DEV_RX_OFFLOAD_VLAN_STRIP)) > > - PMD_INIT_LOG(ERR, "VLAN stripping is " > > - "always on in fm10k"); > > - } > > - > > - if (mask & ETH_VLAN_EXTEND_MASK) { > > - if (dev->data->dev_conf.rxmode.offloads & > > - DEV_RX_OFFLOAD_VLAN_EXTEND) > > - PMD_INIT_LOG(ERR, "VLAN QinQ is not " > > - "supported in fm10k"); > > - } > > - > > - if (mask & ETH_VLAN_FILTER_MASK) { > > - if (!(dev->data->dev_conf.rxmode.offloads & > > - DEV_RX_OFFLOAD_VLAN_FILTER)) > > - PMD_INIT_LOG(ERR, "VLAN filter is always on in > fm10k"); > > - } > > - > > return 0; > > } > > > > diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c > > b/drivers/net/hinic/hinic_pmd_ethdev.c > > index 0d3d62b..67e6afc 100644 > > --- a/drivers/net/hinic/hinic_pmd_ethdev.c > > +++ b/drivers/net/hinic/hinic_pmd_ethdev.c > > @@ -1709,12 +1709,6 @@ static int hinic_vlan_offload_set(struct > rte_eth_dev *dev, int mask) > > nic_dev->proc_dev_name, dev->data->port_id); > > } > > > > - if (mask & ETH_VLAN_EXTEND_MASK) { > > - PMD_DRV_LOG(ERR, "Don't support vlan qinq, device: %s, > port_id: %d", > > - nic_dev->proc_dev_name, dev->data->port_id); > > - return -ENOTSUP; > > - } > > - > > return 0; > > } > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c > > b/drivers/net/i40e/i40e_ethdev.c index 472ce2a..c89290d 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -3865,11 +3865,6 @@ i40e_vlan_offload_set(struct rte_eth_dev > *dev, int mask) > > struct i40e_vsi *vsi = pf->main_vsi; > > struct rte_eth_rxmode *rxmode; > > > > - if (mask & ETH_QINQ_STRIP_MASK) { > > - PMD_DRV_LOG(ERR, "Strip qinq is not supported."); > > - return -ENOTSUP; > > - } > > - > > rxmode = &dev->data->dev_conf.rxmode; > > if (mask & ETH_VLAN_FILTER_MASK) { > > if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) > diff --git > > a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c index > > 88e3f01..474719b 100644 > > --- a/drivers/net/nfp/nfp_net.c > > +++ b/drivers/net/nfp/nfp_net.c > > @@ -2353,11 +2353,6 @@ nfp_net_vlan_offload_set(struct rte_eth_dev > *dev, int mask) > > hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > new_ctrl = 0; > > > > - if ((mask & ETH_VLAN_FILTER_OFFLOAD) || > > - (mask & ETH_VLAN_EXTEND_OFFLOAD)) > > - PMD_DRV_LOG(INFO, "No support for > ETH_VLAN_FILTER_OFFLOAD or" > > - " ETH_VLAN_EXTEND_OFFLOAD"); > > - > > /* Enable vlan strip if it is not configured yet */ > > if ((mask & ETH_VLAN_STRIP_OFFLOAD) && > > !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)) diff --git > > a/drivers/net/octeontx/octeontx_ethdev_ops.c > > b/drivers/net/octeontx/octeontx_ethdev_ops.c > > index ff627a6..dbe13ce 100644 > > --- a/drivers/net/octeontx/octeontx_ethdev_ops.c > > +++ b/drivers/net/octeontx/octeontx_ethdev_ops.c > > @@ -43,16 +43,6 @@ octeontx_dev_vlan_offload_set(struct rte_eth_dev > > *dev, int mask) > > > > rxmode = &dev->data->dev_conf.rxmode; > > > > - if (mask & ETH_VLAN_EXTEND_MASK) { > > - octeontx_log_err("Extend offload not supported"); > > - return -ENOTSUP; > > - } > > - > > - if (mask & ETH_VLAN_STRIP_MASK) { > > - octeontx_log_err("VLAN strip offload not supported"); > > - return -ENOTSUP; > > - } > > - > > if (mask & ETH_VLAN_FILTER_MASK) { > > if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) { > > rc = octeontx_vlan_hw_filter(nic, true); diff --git > > a/drivers/net/octeontx2/otx2_vlan.c > > b/drivers/net/octeontx2/otx2_vlan.c > > index 322a565..7357b06 100644 > > --- a/drivers/net/octeontx2/otx2_vlan.c > > +++ b/drivers/net/octeontx2/otx2_vlan.c > > @@ -717,11 +717,6 @@ otx2_nix_vlan_offload_set(struct rte_eth_dev > > *eth_dev, int mask) > > > > rxmode = ð_dev->data->dev_conf.rxmode; > > > > - if (mask & ETH_VLAN_EXTEND_MASK) { > > - otx2_err("Extend offload not supported"); > > - return -ENOTSUP; > > - } > > - > > if (mask & ETH_VLAN_STRIP_MASK) { > > if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) { > > offloads |= DEV_RX_OFFLOAD_VLAN_STRIP; diff -- > git > > a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c > > index 7f5a34b..f2ed03b 100644 > > --- a/drivers/net/qede/qede_ethdev.c > > +++ b/drivers/net/qede/qede_ethdev.c > > @@ -1031,9 +1031,6 @@ static int qede_vlan_offload_set(struct > rte_eth_dev *eth_dev, int mask) > > } > > } > > > > - if (mask & ETH_VLAN_EXTEND_MASK) > > - DP_ERR(edev, "Extend VLAN not supported\n"); > > - > > qdev->vlan_offload_mask = mask; > > > > DP_INFO(edev, "VLAN offload mask %d\n", mask); diff --git > > a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > > index 50c3f18..078bb25 100644 > > --- a/lib/librte_ethdev/rte_ethdev.c > > +++ b/lib/librte_ethdev/rte_ethdev.c > > @@ -3258,12 +3258,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t > port_id, > > int > > rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) > > { > > + struct rte_eth_dev_info dev_info; > > struct rte_eth_dev *dev; > > int ret = 0; > > int mask = 0; > > int cur, org = 0; > > uint64_t orig_offloads; > > uint64_t dev_offloads; > > + uint64_t new_offloads; > > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > dev = &rte_eth_devices[port_id]; > > @@ -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 > > + */ > > + 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; > > + } > > + > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, - > ENOTSUP); > > dev->data->dev_conf.rxmode.offloads = dev_offloads; > > ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-07 13:38 ` Slava Ovsiienko @ 2020-07-08 3:53 ` Wei Hu (Xavier) 0 siblings, 0 replies; 12+ messages in thread From: Wei Hu (Xavier) @ 2020-07-08 3:53 UTC (permalink / raw) To: thomas, Ferruh Yigit, anatoly.burakov, Andrew Rybchenko, heinrich.kuhn, jerinj, ndabilpuram, kirankumark, Rasesh Mody, shshaikh, Qi Zhang, xiao.w.wang Cc: Slava Ovsiienko, Jeff Guo, dev, Wei Hu (Xavier), hkalra, cloud.wangxiaoyun, sachin.saxena, hyonkim, arybchenko Hi, Thomas Monjalon & Anatoly Burakov & Heinrich Kuhn & Jerin Jacob & Nithin Dabilpuram & Kiran Kumar K & Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang. Could you please help to review this patch and give 'Acked-by' if there are no objects. Thanks, Xavier On 2020/7/7 21:38, Slava Ovsiienko wrote: > Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com> > >> -----Original Message----- >> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo >> Sent: Tuesday, July 7, 2020 10:36 >> To: Wei Hu (Xavier) <xavier.huwei@huawei.com>; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no >> relative capabilities >> >> >> On 7/6/2020 3:06 PM, Wei Hu (Xavier) wrote: >>> 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> >> >> Acked-by: Jeff Guo <jia.guo@intel.com> >> >> >>> --- >>> v5 -> v6: add the related history patch into the Fixes commit log. >>> v4 -> v5: no change. >>> v3 -> v4: Delete "next_mask" label and modify the function that when the >>> offload is not supported the function fail. >>> v2 -> v3: Add __rte_unused to avoid unused parameter 'dev' and 'mask' >>> warning. >>> v1 -> v2: Cleanup duplicated checks which are done in some PMDs. >>> --- >>> drivers/net/dpaa2/dpaa2_ethdev.c | 12 +++--------- >>> drivers/net/enic/enic_ethdev.c | 12 ------------ >>> drivers/net/fm10k/fm10k_ethdev.c | 23 ++--------------------- >>> drivers/net/hinic/hinic_pmd_ethdev.c | 6 ------ >>> drivers/net/i40e/i40e_ethdev.c | 5 ----- >>> drivers/net/nfp/nfp_net.c | 5 ----- >>> drivers/net/octeontx/octeontx_ethdev_ops.c | 10 ---------- >>> drivers/net/octeontx2/otx2_vlan.c | 5 ----- >>> drivers/net/qede/qede_ethdev.c | 3 --- >>> lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ >>> 10 files changed, 26 insertions(+), 76 deletions(-) >>> >>> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c >>> b/drivers/net/dpaa2/dpaa2_ethdev.c >>> index a1f1919..489d744 100644 >>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c >>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c >>> @@ -145,7 +145,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, >> int mask) >>> { >>> struct dpaa2_dev_priv *priv = dev->data->dev_private; >>> struct fsl_mc_io *dpni = dev->process_private; >>> - int ret; >>> + int ret = 0; >>> >>> PMD_INIT_FUNC_TRACE(); >>> >>> @@ -153,7 +153,7 @@ dpaa2_vlan_offload_set(struct rte_eth_dev *dev, >> int mask) >>> /* VLAN Filter not avaialble */ >>> if (!priv->max_vlan_filters) { >>> DPAA2_PMD_INFO("VLAN filter not available"); >>> - goto next_mask; >>> + return -ENOTSUP; >>> } >>> >>> if (dev->data->dev_conf.rxmode.offloads & @@ -166,14 >> +166,8 @@ >>> dpaa2_vlan_offload_set(struct rte_eth_dev *dev, int mask) >>> if (ret < 0) >>> DPAA2_PMD_INFO("Unable to set vlan filter = %d", >> ret); >>> } >>> -next_mask: >>> - if (mask & ETH_VLAN_EXTEND_MASK) { >>> - if (dev->data->dev_conf.rxmode.offloads & >>> - DEV_RX_OFFLOAD_VLAN_EXTEND) >>> - DPAA2_PMD_INFO("VLAN extend offload not >> supported"); >>> - } >>> >>> - return 0; >>> + return ret; >>> } >>> >>> static int >>> diff --git a/drivers/net/enic/enic_ethdev.c >>> b/drivers/net/enic/enic_ethdev.c index 6a3580f..30a599d 100644 >>> --- a/drivers/net/enic/enic_ethdev.c >>> +++ b/drivers/net/enic/enic_ethdev.c >>> @@ -367,18 +367,6 @@ static int enicpmd_vlan_offload_set(struct >> rte_eth_dev *eth_dev, int mask) >>> enic->ig_vlan_strip_en = 0; >>> } >>> >>> - if ((mask & ETH_VLAN_FILTER_MASK) && >>> - (offloads & DEV_RX_OFFLOAD_VLAN_FILTER)) { >>> - dev_warning(enic, >>> - "Configuration of VLAN filter is not supported\n"); >>> - } >>> - >>> - if ((mask & ETH_VLAN_EXTEND_MASK) && >>> - (offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)) { >>> - dev_warning(enic, >>> - "Configuration of extended VLAN is not >> supported\n"); >>> - } >>> - >>> return enic_set_vlan_strip(enic); >>> } >>> >>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >>> b/drivers/net/fm10k/fm10k_ethdev.c >>> index cb0dd3b..b574693 100644 >>> --- a/drivers/net/fm10k/fm10k_ethdev.c >>> +++ b/drivers/net/fm10k/fm10k_ethdev.c >>> @@ -1562,28 +1562,9 @@ fm10k_vlan_filter_set(struct rte_eth_dev *dev, >> uint16_t vlan_id, int on) >>> } >>> >>> static int >>> -fm10k_vlan_offload_set(struct rte_eth_dev *dev, int mask) >>> +fm10k_vlan_offload_set(struct rte_eth_dev *dev __rte_unused, >>> + int mask __rte_unused) >>> { >>> - if (mask & ETH_VLAN_STRIP_MASK) { >>> - if (!(dev->data->dev_conf.rxmode.offloads & >>> - DEV_RX_OFFLOAD_VLAN_STRIP)) >>> - PMD_INIT_LOG(ERR, "VLAN stripping is " >>> - "always on in fm10k"); >>> - } >>> - >>> - if (mask & ETH_VLAN_EXTEND_MASK) { >>> - if (dev->data->dev_conf.rxmode.offloads & >>> - DEV_RX_OFFLOAD_VLAN_EXTEND) >>> - PMD_INIT_LOG(ERR, "VLAN QinQ is not " >>> - "supported in fm10k"); >>> - } >>> - >>> - if (mask & ETH_VLAN_FILTER_MASK) { >>> - if (!(dev->data->dev_conf.rxmode.offloads & >>> - DEV_RX_OFFLOAD_VLAN_FILTER)) >>> - PMD_INIT_LOG(ERR, "VLAN filter is always on in >> fm10k"); >>> - } >>> - >>> return 0; >>> } >>> >>> diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c >>> b/drivers/net/hinic/hinic_pmd_ethdev.c >>> index 0d3d62b..67e6afc 100644 >>> --- a/drivers/net/hinic/hinic_pmd_ethdev.c >>> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c >>> @@ -1709,12 +1709,6 @@ static int hinic_vlan_offload_set(struct >> rte_eth_dev *dev, int mask) >>> nic_dev->proc_dev_name, dev->data->port_id); >>> } >>> >>> - if (mask & ETH_VLAN_EXTEND_MASK) { >>> - PMD_DRV_LOG(ERR, "Don't support vlan qinq, device: %s, >> port_id: %d", >>> - nic_dev->proc_dev_name, dev->data->port_id); >>> - return -ENOTSUP; >>> - } >>> - >>> return 0; >>> } >>> >>> diff --git a/drivers/net/i40e/i40e_ethdev.c >>> b/drivers/net/i40e/i40e_ethdev.c index 472ce2a..c89290d 100644 >>> --- a/drivers/net/i40e/i40e_ethdev.c >>> +++ b/drivers/net/i40e/i40e_ethdev.c >>> @@ -3865,11 +3865,6 @@ i40e_vlan_offload_set(struct rte_eth_dev >> *dev, int mask) >>> struct i40e_vsi *vsi = pf->main_vsi; >>> struct rte_eth_rxmode *rxmode; >>> >>> - if (mask & ETH_QINQ_STRIP_MASK) { >>> - PMD_DRV_LOG(ERR, "Strip qinq is not supported."); >>> - return -ENOTSUP; >>> - } >>> - >>> rxmode = &dev->data->dev_conf.rxmode; >>> if (mask & ETH_VLAN_FILTER_MASK) { >>> if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) >> diff --git >>> a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c index >>> 88e3f01..474719b 100644 >>> --- a/drivers/net/nfp/nfp_net.c >>> +++ b/drivers/net/nfp/nfp_net.c >>> @@ -2353,11 +2353,6 @@ nfp_net_vlan_offload_set(struct rte_eth_dev >> *dev, int mask) >>> hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private); >>> new_ctrl = 0; >>> >>> - if ((mask & ETH_VLAN_FILTER_OFFLOAD) || >>> - (mask & ETH_VLAN_EXTEND_OFFLOAD)) >>> - PMD_DRV_LOG(INFO, "No support for >> ETH_VLAN_FILTER_OFFLOAD or" >>> - " ETH_VLAN_EXTEND_OFFLOAD"); >>> - >>> /* Enable vlan strip if it is not configured yet */ >>> if ((mask & ETH_VLAN_STRIP_OFFLOAD) && >>> !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)) diff --git >>> a/drivers/net/octeontx/octeontx_ethdev_ops.c >>> b/drivers/net/octeontx/octeontx_ethdev_ops.c >>> index ff627a6..dbe13ce 100644 >>> --- a/drivers/net/octeontx/octeontx_ethdev_ops.c >>> +++ b/drivers/net/octeontx/octeontx_ethdev_ops.c >>> @@ -43,16 +43,6 @@ octeontx_dev_vlan_offload_set(struct rte_eth_dev >>> *dev, int mask) >>> >>> rxmode = &dev->data->dev_conf.rxmode; >>> >>> - if (mask & ETH_VLAN_EXTEND_MASK) { >>> - octeontx_log_err("Extend offload not supported"); >>> - return -ENOTSUP; >>> - } >>> - >>> - if (mask & ETH_VLAN_STRIP_MASK) { >>> - octeontx_log_err("VLAN strip offload not supported"); >>> - return -ENOTSUP; >>> - } >>> - >>> if (mask & ETH_VLAN_FILTER_MASK) { >>> if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) { >>> rc = octeontx_vlan_hw_filter(nic, true); diff --git >>> a/drivers/net/octeontx2/otx2_vlan.c >>> b/drivers/net/octeontx2/otx2_vlan.c >>> index 322a565..7357b06 100644 >>> --- a/drivers/net/octeontx2/otx2_vlan.c >>> +++ b/drivers/net/octeontx2/otx2_vlan.c >>> @@ -717,11 +717,6 @@ otx2_nix_vlan_offload_set(struct rte_eth_dev >>> *eth_dev, int mask) >>> >>> rxmode = ð_dev->data->dev_conf.rxmode; >>> >>> - if (mask & ETH_VLAN_EXTEND_MASK) { >>> - otx2_err("Extend offload not supported"); >>> - return -ENOTSUP; >>> - } >>> - >>> if (mask & ETH_VLAN_STRIP_MASK) { >>> if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) { >>> offloads |= DEV_RX_OFFLOAD_VLAN_STRIP; diff -- >> git >>> a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c >>> index 7f5a34b..f2ed03b 100644 >>> --- a/drivers/net/qede/qede_ethdev.c >>> +++ b/drivers/net/qede/qede_ethdev.c >>> @@ -1031,9 +1031,6 @@ static int qede_vlan_offload_set(struct >> rte_eth_dev *eth_dev, int mask) >>> } >>> } >>> >>> - if (mask & ETH_VLAN_EXTEND_MASK) >>> - DP_ERR(edev, "Extend VLAN not supported\n"); >>> - >>> qdev->vlan_offload_mask = mask; >>> >>> DP_INFO(edev, "VLAN offload mask %d\n", mask); diff --git >>> a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >>> index 50c3f18..078bb25 100644 >>> --- a/lib/librte_ethdev/rte_ethdev.c >>> +++ b/lib/librte_ethdev/rte_ethdev.c >>> @@ -3258,12 +3258,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t >> port_id, >>> int >>> rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >>> { >>> + struct rte_eth_dev_info dev_info; >>> struct rte_eth_dev *dev; >>> int ret = 0; >>> int mask = 0; >>> int cur, org = 0; >>> uint64_t orig_offloads; >>> uint64_t dev_offloads; >>> + uint64_t new_offloads; >>> >>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>> dev = &rte_eth_devices[port_id]; >>> @@ -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 >>> + */ >>> + 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; >>> + } >>> + >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, - >> ENOTSUP); >>> dev->data->dev_conf.rxmode.offloads = dev_offloads; >>> ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier) 2020-07-07 7:35 ` Jeff Guo @ 2020-07-07 14:11 ` Thomas Monjalon 2020-07-08 3:37 ` Wei Hu (Xavier) 1 sibling, 1 reply; 12+ messages in thread From: Thomas Monjalon @ 2020-07-07 14:11 UTC (permalink / raw) To: xavier.huwei Cc: dev, ferruh.yigit, arybchenko, tangchengchang, hyonkim, sachin.saxena, cloud.wangxiaoyun, hkalra, viacheslavo 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; > + } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-07 14:11 ` Thomas Monjalon @ 2020-07-08 3:37 ` Wei Hu (Xavier) 2020-07-08 10:14 ` Thomas Monjalon 0 siblings, 1 reply; 12+ messages in thread From: Wei Hu (Xavier) @ 2020-07-08 3:37 UTC (permalink / raw) To: Thomas Monjalon Cc: dev, ferruh.yigit, arybchenko, tangchengchang, hyonkim, sachin.saxena, cloud.wangxiaoyun, hkalra, viacheslavo 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; >> + } > > > . > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-08 3:37 ` Wei Hu (Xavier) @ 2020-07-08 10:14 ` Thomas Monjalon 2020-07-09 7:39 ` Wei Hu (Xavier) 0 siblings, 1 reply; 12+ messages in thread From: Thomas Monjalon @ 2020-07-08 10:14 UTC (permalink / raw) To: Wei Hu (Xavier) Cc: dev, ferruh.yigit, arybchenko, tangchengchang, hyonkim, sachin.saxena, cloud.wangxiaoyun, hkalra, viacheslavo 08/07/2020 05:37, Wei Hu (Xavier): > 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. [...] > >> @@ -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(). OK I don't understand why checking only new features. All enabled features must be within capabilities, right? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-08 10:14 ` Thomas Monjalon @ 2020-07-09 7:39 ` Wei Hu (Xavier) 2020-07-09 9:04 ` Thomas Monjalon 0 siblings, 1 reply; 12+ messages in thread From: Wei Hu (Xavier) @ 2020-07-09 7:39 UTC (permalink / raw) To: Thomas Monjalon Cc: dev, ferruh.yigit, arybchenko, tangchengchang, hyonkim, sachin.saxena, cloud.wangxiaoyun, hkalra, viacheslavo On 2020/7/8 18:14, Thomas Monjalon wrote: > 08/07/2020 05:37, Wei Hu (Xavier): >> 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. > [...] >>>> @@ -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(). > OK > > I don't understand why checking only new features. > All enabled features must be within capabilities, right? Yes，you are right. all enabled features must be within capabilities， Some features enabled in rte_eth_dev_configure() had been already checked, So the comment here emphasizes 'new added Rx VLAN offloading'. Thanks，Xavier > > > > . > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities 2020-07-09 7:39 ` Wei Hu (Xavier) @ 2020-07-09 9:04 ` Thomas Monjalon 0 siblings, 0 replies; 12+ messages in thread From: Thomas Monjalon @ 2020-07-09 9:04 UTC (permalink / raw) To: Wei Hu (Xavier) Cc: dev, ferruh.yigit, arybchenko, tangchengchang, hyonkim, sachin.saxena, cloud.wangxiaoyun, hkalra, viacheslavo 09/07/2020 09:39, Wei Hu (Xavier): > On 2020/7/8 18:14, Thomas Monjalon wrote: > > 08/07/2020 05:37, Wei Hu (Xavier): > >> 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. > > [...] > >>>> @@ -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(). > > OK > > > > I don't understand why checking only new features. > > All enabled features must be within capabilities, right? > Yes，you are right. all enabled features must be within capabilities， > Some features enabled in rte_eth_dev_configure() had been already checked, > So the comment here emphasizes 'new added Rx VLAN offloading'. I feel this precision more confusing than helpful. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-09 9:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-06 7:06 [dpdk-dev] [PATCH v6 0/2] ethdev: minor bugfixes Wei Hu (Xavier) 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier) 2020-07-07 13:37 ` Slava Ovsiienko 2020-07-06 7:06 ` [dpdk-dev] [PATCH v6 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier) 2020-07-07 7:35 ` Jeff Guo 2020-07-07 13:38 ` Slava Ovsiienko 2020-07-08 3:53 ` Wei Hu (Xavier) 2020-07-07 14:11 ` Thomas Monjalon 2020-07-08 3:37 ` Wei Hu (Xavier) 2020-07-08 10:14 ` Thomas Monjalon 2020-07-09 7:39 ` Wei Hu (Xavier) 2020-07-09 9:04 ` Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).