DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v4 0/2] ethdev: minor bugfixes
@ 2020-06-22  8:08 Wei Hu (Xavier)
  2020-06-22  8:08 ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier)
  2020-06-22  8:09 ` [dpdk-dev] [PATCH v4 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-06-22  8:08 UTC (permalink / raw)
  To: thomas, ferruh.yigit, arybchenko, anatoly.burakov,
	hemant.agrawal, sachin.saxena, johndale, hyonkim, qi.z.zhang,
	xiao.w.wang, xuanziyang2, cloud.wangxiaoyun, zhouguoyang,
	beilei.xing, jia.guo, heinrich.kuhn, hkalra, jerinj, ndabilpuram,
	kirankumark, rmody, shshaikh
  Cc: dev, 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 v4 1/2] ethdev: fix data room size verification in Rx queue setup
  2020-06-22  8:08 [dpdk-dev] [PATCH v4 0/2] ethdev: minor bugfixes Wei Hu (Xavier)
@ 2020-06-22  8:08 ` Wei Hu (Xavier)
  2020-06-22  8:24   ` Sachin Saxena (OSS)
  2020-06-22  8:09 ` [dpdk-dev] [PATCH v4 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-06-22  8:08 UTC (permalink / raw)
  To: thomas, ferruh.yigit, arybchenko, anatoly.burakov,
	hemant.agrawal, sachin.saxena, johndale, hyonkim, qi.z.zhang,
	xiao.w.wang, xuanziyang2, cloud.wangxiaoyun, zhouguoyang,
	beilei.xing, jia.guo, heinrich.kuhn, hkalra, jerinj, ndabilpuram,
	kirankumark, rmody, shshaikh
  Cc: dev, 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>
---
v3 -> v4: No change.
v2 -> v3: 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 8e10a6f..b0b0474 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1822,7 +1822,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 v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-22  8:08 [dpdk-dev] [PATCH v4 0/2] ethdev: minor bugfixes Wei Hu (Xavier)
  2020-06-22  8:08 ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier)
@ 2020-06-22  8:09 ` Wei Hu (Xavier)
  2020-06-22  8:27   ` Sachin Saxena (OSS)
                     ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Wei Hu (Xavier) @ 2020-06-22  8:09 UTC (permalink / raw)
  To: thomas, ferruh.yigit, arybchenko, anatoly.burakov,
	hemant.agrawal, sachin.saxena, johndale, hyonkim, qi.z.zhang,
	xiao.w.wang, xuanziyang2, cloud.wangxiaoyun, zhouguoyang,
	beilei.xing, jia.guo, heinrich.kuhn, hkalra, jerinj, ndabilpuram,
	kirankumark, rmody, shshaikh
  Cc: dev, 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")
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>
---
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 2f031ec..17a7a49 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -147,7 +147,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();
 
@@ -155,7 +155,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 &
@@ -168,14 +168,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 32d5397..ef8900d 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -374,18 +374,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 f537ab2..f5b854e 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
--- a/drivers/net/hinic/hinic_pmd_ethdev.c
+++ b/drivers/net/hinic/hinic_pmd_ethdev.c
@@ -1701,12 +1701,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 970a31c..9211cf5 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -1036,9 +1036,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 b0b0474..bf79749 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -3260,12 +3260,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];
@@ -3319,6 +3321,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 v4 1/2] ethdev: fix data room size verification in Rx queue setup
  2020-06-22  8:08 ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier)
@ 2020-06-22  8:24   ` Sachin Saxena (OSS)
  0 siblings, 0 replies; 12+ messages in thread
From: Sachin Saxena (OSS) @ 2020-06-22  8:24 UTC (permalink / raw)
  To: Wei Hu (Xavier), hemant.agrawal; +Cc: dev

Acked-by: Sachin Saxena<sachin.saxena@oss.nxp.com>


On 22-Jun-20 1:38 PM, Wei Hu (Xavier) wrote:
> 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>
> ---
> v3 -> v4: No change.
> v2 -> v3: 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 8e10a6f..b0b0474 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1822,7 +1822,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,


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-22  8:09 ` [dpdk-dev] [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier)
@ 2020-06-22  8:27   ` Sachin Saxena (OSS)
  2020-06-22  9:08   ` Wangxiaoyun (Cloud)
  2020-06-29  1:34   ` Wei Hu (Xavier)
  2 siblings, 0 replies; 12+ messages in thread
From: Sachin Saxena (OSS) @ 2020-06-22  8:27 UTC (permalink / raw)
  To: Wei Hu (Xavier), Hemant Agrawal; +Cc: dev

Acked-by: Sachin Saxena<sachin.saxena@oss.nxp.com>


On 22-Jun-20 1:39 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")
> 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>
> ---
> 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 2f031ec..17a7a49 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -147,7 +147,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();
>   
> @@ -155,7 +155,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 &
> @@ -168,14 +168,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 32d5397..ef8900d 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -374,18 +374,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 f537ab2..f5b854e 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3260,12 +3260,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];
> @@ -3319,6 +3321,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 v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-22  8:09 ` [dpdk-dev] [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier)
  2020-06-22  8:27   ` Sachin Saxena (OSS)
@ 2020-06-22  9:08   ` Wangxiaoyun (Cloud)
  2020-06-29  1:34   ` Wei Hu (Xavier)
  2 siblings, 0 replies; 12+ messages in thread
From: Wangxiaoyun (Cloud) @ 2020-06-22  9:08 UTC (permalink / raw)
  To: Wei Hu (Xavier),
	thomas, ferruh.yigit, arybchenko, anatoly.burakov,
	hemant.agrawal, sachin.saxena, johndale, hyonkim, qi.z.zhang,
	xiao.w.wang, xuanziyang2, zhouguoyang, beilei.xing, jia.guo,
	heinrich.kuhn, hkalra, jerinj, ndabilpuram, kirankumark, rmody,
	shshaikh
  Cc: dev, yin.yinshi, luoxingyu, zhouguoyang

Acked-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>

在 2020/6/22 16:09, 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")
> 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>
> ---
> 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 2f031ec..17a7a49 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -147,7 +147,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();
>   
> @@ -155,7 +155,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 &
> @@ -168,14 +168,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 32d5397..ef8900d 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -374,18 +374,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 f537ab2..f5b854e 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3260,12 +3260,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];
> @@ -3319,6 +3321,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 v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-22  8:09 ` [dpdk-dev] [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier)
  2020-06-22  8:27   ` Sachin Saxena (OSS)
  2020-06-22  9:08   ` Wangxiaoyun (Cloud)
@ 2020-06-29  1:34   ` Wei Hu (Xavier)
  2020-06-29 14:47     ` [dpdk-dev] [EXT] " Harman Kalra
  2020-07-03  1:28     ` [dpdk-dev] " Wei Hu (Xavier)
  2 siblings, 2 replies; 12+ messages in thread
From: Wei Hu (Xavier) @ 2020-06-29  1:34 UTC (permalink / raw)
  To: beilei.xing, jia.guo, xiao.w.wang, qi.z.zhang, heinrich.kuhn,
	hkalra, jerinj, ndabilpuram, kirankumark, rmody, shshaikh
  Cc: Wei Hu (Xavier), dev

Hi, Heinrich Kuhn & Harman Kalra & Jerin Jacob & Nithin Dabilpuram & 
Kiran Kumar K &

Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & Jeff Guo.

Could you please help to review this patch and give 'Acked-by' if there 
are no objects.

Thanks, Xavier

On 2020/6/22 16:09, 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")
> 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>
> ---
> 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 2f031ec..17a7a49 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -147,7 +147,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();
>   
> @@ -155,7 +155,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 &
> @@ -168,14 +168,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 32d5397..ef8900d 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -374,18 +374,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 f537ab2..f5b854e 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3260,12 +3260,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];
> @@ -3319,6 +3321,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] [EXT] Re: [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-29  1:34   ` Wei Hu (Xavier)
@ 2020-06-29 14:47     ` Harman Kalra
  2020-07-01  2:32       ` Wei Hu (Xavier)
  2020-07-03  1:28     ` [dpdk-dev] " Wei Hu (Xavier)
  1 sibling, 1 reply; 12+ messages in thread
From: Harman Kalra @ 2020-06-29 14:47 UTC (permalink / raw)
  To: Wei Hu (Xavier)
  Cc: beilei.xing, jia.guo, xiao.w.wang, qi.z.zhang, heinrich.kuhn,
	jerinj, ndabilpuram, kirankumark, rmody, shshaikh, dev

Acked-by: Harman Kalra <hkalra@marvell.com>

On Mon, Jun 29, 2020 at 09:34:00AM +0800, Wei Hu (Xavier) wrote:
> External Email
> 
> ----------------------------------------------------------------------
> Hi, Heinrich Kuhn & Harman Kalra & Jerin Jacob & Nithin Dabilpuram & Kiran
> Kumar K &
> 
> Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & Jeff Guo.
> 
> Could you please help to review this patch and give 'Acked-by' if there are
> no objects.
> 
> Thanks, Xavier
> 
> On 2020/6/22 16:09, 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")
> > 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>
> > ---
> > 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 2f031ec..17a7a49 100644
> > --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> > +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> > @@ -147,7 +147,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();
> > @@ -155,7 +155,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 &
> > @@ -168,14 +168,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 32d5397..ef8900d 100644
> > --- a/drivers/net/enic/enic_ethdev.c
> > +++ b/drivers/net/enic/enic_ethdev.c
> > @@ -374,18 +374,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 f537ab2..f5b854e 100644
> > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
> > --- a/drivers/net/hinic/hinic_pmd_ethdev.c
> > +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
> > @@ -1701,12 +1701,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 970a31c..9211cf5 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
> > --- a/drivers/net/qede/qede_ethdev.c
> > +++ b/drivers/net/qede/qede_ethdev.c
> > @@ -1036,9 +1036,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 b0b0474..bf79749 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -3260,12 +3260,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];
> > @@ -3319,6 +3321,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] [EXT] Re: [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-29 14:47     ` [dpdk-dev] [EXT] " Harman Kalra
@ 2020-07-01  2:32       ` Wei Hu (Xavier)
  0 siblings, 0 replies; 12+ messages in thread
From: Wei Hu (Xavier) @ 2020-07-01  2:32 UTC (permalink / raw)
  To: Harman Kalra
  Cc: beilei.xing, jia.guo, xiao.w.wang, qi.z.zhang, heinrich.kuhn,
	jerinj, ndabilpuram, kirankumark, rmody, shshaikh, dev,
	xavier >> Wei Hu (Xavier)



On 2020/6/29 22:47, Harman Kalra wrote:
> Acked-by: Harman Kalra <hkalra@marvell.com>
Hi,Harman Kalra
  Thanks
Xavier
>
> On Mon, Jun 29, 2020 at 09:34:00AM +0800, Wei Hu (Xavier) wrote:
>> External Email
>>
>> ----------------------------------------------------------------------
>> Hi, Heinrich Kuhn & Harman Kalra & Jerin Jacob & Nithin Dabilpuram & Kiran
>> Kumar K &
>>
>> Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & Jeff Guo.
>>
>> Could you please help to review this patch and give 'Acked-by' if there are
>> no objects.
>>
>> Thanks, Xavier
>>
>> On 2020/6/22 16:09, 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")
>>> 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>
>>> ---
>>> 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 2f031ec..17a7a49 100644
>>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>>> @@ -147,7 +147,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();
>>> @@ -155,7 +155,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 &
>>> @@ -168,14 +168,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 32d5397..ef8900d 100644
>>> --- a/drivers/net/enic/enic_ethdev.c
>>> +++ b/drivers/net/enic/enic_ethdev.c
>>> @@ -374,18 +374,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 f537ab2..f5b854e 100644
>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
>>> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
>>> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
>>> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
>>> --- a/drivers/net/qede/qede_ethdev.c
>>> +++ b/drivers/net/qede/qede_ethdev.c
>>> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -3260,12 +3260,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];
>>> @@ -3319,6 +3321,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 v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-06-29  1:34   ` Wei Hu (Xavier)
  2020-06-29 14:47     ` [dpdk-dev] [EXT] " Harman Kalra
@ 2020-07-03  1:28     ` Wei Hu (Xavier)
  2020-07-05 11:22       ` Jeff Guo
  1 sibling, 1 reply; 12+ messages in thread
From: Wei Hu (Xavier) @ 2020-07-03  1:28 UTC (permalink / raw)
  To: beilei.xing, jia.guo, xiao.w.wang, qi.z.zhang, heinrich.kuhn,
	jerinj, ndabilpuram, kirankumark, rmody, shshaikh
  Cc: dev, Wei Hu (Xavier)

Hi, Heinrich Kuhn & Jerin Jacob & Nithin Dabilpuram & Kiran Kumar K &

   Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & 
Jeff Guo.

Could you please help to review this patch and give 'Acked-by' if there 
are no objects.

Thanks, Xavier


On 2020/6/29 9:34, Wei Hu (Xavier) wrote:
> Hi, Heinrich Kuhn & Harman Kalra & Jerin Jacob & Nithin Dabilpuram & 
> Kiran Kumar K &
>
> Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & 
> Jeff Guo.
>
> Could you please help to review this patch and give 'Acked-by' if 
> there are no objects.
>
> Thanks, Xavier
>
> On 2020/6/22 16:09, 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")
>> 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>
>> ---
>> 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 2f031ec..17a7a49 100644
>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>> @@ -147,7 +147,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();
>>   @@ -155,7 +155,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 &
>> @@ -168,14 +168,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 32d5397..ef8900d 100644
>> --- a/drivers/net/enic/enic_ethdev.c
>> +++ b/drivers/net/enic/enic_ethdev.c
>> @@ -374,18 +374,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 f537ab2..f5b854e 100644
>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
>> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
>> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
>> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
>> --- a/drivers/net/i40e/i40e_ethdev.c
>> +++ b/drivers/net/i40e/i40e_ethdev.c
>> @@ -3877,11 +3877,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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
>> --- a/drivers/net/qede/qede_ethdev.c
>> +++ b/drivers/net/qede/qede_ethdev.c
>> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -3260,12 +3260,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];
>> @@ -3319,6 +3321,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 v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-07-03  1:28     ` [dpdk-dev] " Wei Hu (Xavier)
@ 2020-07-05 11:22       ` Jeff Guo
  2020-07-06  6:32         ` Wei Hu (Xavier)
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff Guo @ 2020-07-05 11:22 UTC (permalink / raw)
  To: Wei Hu (Xavier),
	beilei.xing, xiao.w.wang, qi.z.zhang, heinrich.kuhn, jerinj,
	ndabilpuram, kirankumark, rmody, shshaikh
  Cc: dev

hi, xavier

On 7/3/2020 9:28 AM, Wei Hu (Xavier) wrote:
> Hi, Heinrich Kuhn & Jerin Jacob & Nithin Dabilpuram & Kiran Kumar K &
>
>   Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & 
> Jeff Guo.
>
> Could you please help to review this patch and give 'Acked-by' if 
> there are no objects.
>
> Thanks, Xavier
>
>
> On 2020/6/29 9:34, Wei Hu (Xavier) wrote:
>> Hi, Heinrich Kuhn & Harman Kalra & Jerin Jacob & Nithin Dabilpuram & 
>> Kiran Kumar K &
>>
>> Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & 
>> Jeff Guo.
>>
>> Could you please help to review this patch and give 'Acked-by' if 
>> there are no objects.
>>
>> Thanks, Xavier
>>
>> On 2020/6/22 16:09, 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")
>>> 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>
>>> ---
>>> 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 2f031ec..17a7a49 100644
>>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>>> @@ -147,7 +147,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();
>>>   @@ -155,7 +155,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 &
>>> @@ -168,14 +168,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 32d5397..ef8900d 100644
>>> --- a/drivers/net/enic/enic_ethdev.c
>>> +++ b/drivers/net/enic/enic_ethdev.c
>>> @@ -374,18 +374,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 f537ab2..f5b854e 100644
>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
>>> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
>>> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
>>> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -3877,11 +3877,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;
>>> -    }
>>> -


This part of change looks good if it checking capability by device 
capability info, but please add the related history patch into the Fixes 
commit log if

it is not involved by " a4996bd89c42 ("ethdev: new Rx/Tx offloads API") 
", other is the same i think.


>>>       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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
>>> --- a/drivers/net/qede/qede_ethdev.c
>>> +++ b/drivers/net/qede/qede_ethdev.c
>>> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -3260,12 +3260,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];
>>> @@ -3319,6 +3321,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 v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities
  2020-07-05 11:22       ` Jeff Guo
@ 2020-07-06  6:32         ` Wei Hu (Xavier)
  0 siblings, 0 replies; 12+ messages in thread
From: Wei Hu (Xavier) @ 2020-07-06  6:32 UTC (permalink / raw)
  To: Jeff Guo, beilei.xing, xiao.w.wang, qi.z.zhang, heinrich.kuhn,
	jerinj, ndabilpuram, kirankumark, rmody, shshaikh
  Cc: dev, Wei Hu (Xavier)

Hi, Jeff Guo


On 2020/7/5 19:22, Jeff Guo wrote:
> hi, xavier
>
> On 7/3/2020 9:28 AM, Wei Hu (Xavier) wrote:
>> Hi, Heinrich Kuhn & Jerin Jacob & Nithin Dabilpuram & Kiran Kumar K &
>>
>>   Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & 
>> Jeff Guo.
>>
>> Could you please help to review this patch and give 'Acked-by' if 
>> there are no objects.
>>
>> Thanks, Xavier
>>
>>
>> On 2020/6/29 9:34, Wei Hu (Xavier) wrote:
>>> Hi, Heinrich Kuhn & Harman Kalra & Jerin Jacob & Nithin Dabilpuram & 
>>> Kiran Kumar K &
>>>
>>> Rasesh Mody & Shahed Shaikh & Qi Zhang & Xiao Wang & Beilei Xing & 
>>> Jeff Guo.
>>>
>>> Could you please help to review this patch and give 'Acked-by' if 
>>> there are no objects.
>>>
>>> Thanks, Xavier
>>>
>>> On 2020/6/22 16:09, 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")
>>>> 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>
>>>> ---
>>>> 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 2f031ec..17a7a49 100644
>>>> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
>>>> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
>>>> @@ -147,7 +147,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();
>>>>   @@ -155,7 +155,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 &
>>>> @@ -168,14 +168,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 32d5397..ef8900d 100644
>>>> --- a/drivers/net/enic/enic_ethdev.c
>>>> +++ b/drivers/net/enic/enic_ethdev.c
>>>> @@ -374,18 +374,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 f537ab2..f5b854e 100644
>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>>> @@ -1575,28 +1575,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 0c3e1c0..0009a61 100644
>>>> --- a/drivers/net/hinic/hinic_pmd_ethdev.c
>>>> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c
>>>> @@ -1701,12 +1701,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 970a31c..9211cf5 100644
>>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>>> @@ -3877,11 +3877,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;
>>>> -    }
>>>> -
>
>
> This part of change looks good if it checking capability by device 
> capability info, but please add the related history patch into the 
> Fixes commit log if
>
> it is not involved by " a4996bd89c42 ("ethdev: new Rx/Tx offloads 
> API") ", other is the same i think.
>
OK,  I will add them into Fixes  commit log in v5.
Thanks, Xavier
>
>>>>       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 2460ee1..b7a91ae 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 = &eth_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 c4f8f12..04d17c7 100644
>>>> --- a/drivers/net/qede/qede_ethdev.c
>>>> +++ b/drivers/net/qede/qede_ethdev.c
>>>> @@ -1036,9 +1036,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 b0b0474..bf79749 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -3260,12 +3260,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];
>>>> @@ -3319,6 +3321,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

end of thread, other threads:[~2020-07-06  6:32 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22  8:08 [dpdk-dev] [PATCH v4 0/2] ethdev: minor bugfixes Wei Hu (Xavier)
2020-06-22  8:08 ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix data room size verification in Rx queue setup Wei Hu (Xavier)
2020-06-22  8:24   ` Sachin Saxena (OSS)
2020-06-22  8:09 ` [dpdk-dev] [PATCH v4 2/2] ethdev: fix VLAN offloads set if no relative capabilities Wei Hu (Xavier)
2020-06-22  8:27   ` Sachin Saxena (OSS)
2020-06-22  9:08   ` Wangxiaoyun (Cloud)
2020-06-29  1:34   ` Wei Hu (Xavier)
2020-06-29 14:47     ` [dpdk-dev] [EXT] " Harman Kalra
2020-07-01  2:32       ` Wei Hu (Xavier)
2020-07-03  1:28     ` [dpdk-dev] " Wei Hu (Xavier)
2020-07-05 11:22       ` Jeff Guo
2020-07-06  6:32         ` Wei Hu (Xavier)

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).