DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/i40e: issue with ADD VLAN from Guest
@ 2020-12-09 17:55 Souvik Dey
  2020-12-11  3:07 ` Guo, Jia
  2020-12-12 13:05 ` [dpdk-dev] [PATCH v2] " Souvik Dey
  0 siblings, 2 replies; 13+ messages in thread
From: Souvik Dey @ 2020-12-09 17:55 UTC (permalink / raw)
  To: beilei.xing, jia.guo, qi.z.zhang; +Cc: dev, Souvik Dey

In case of i40evf pmd, when ADD_VLAN is sent down the linux i40e driver,
along with add the vlan the kernel driver also enables the vlan stripping
by default.
This might have issues if the app configured DEV_RX_OFFLOAD_VLAN_STRIP
as off at the port configuration. The app after adding the VLAN will
expect the VLAN to be coming in the received packets but, due to
VLAN_STRIP enabled at the PF, it will get stripped.
This behavior of the Linux driver causes confussion with the DPDK app
using i40e_pmd. So it is better to reconfigure the vlan_offload, which
checks for DEV_RX_OFFLOAD_VLAN_STRIP flag in the dev_conf and enables or
disables the vlan strip in the PF.
Also rte_eth_dev_set_vlan_offload() to disable VLAN_STRIP cannot be used
from the application, as this will only work for the first time when
original and current confi mismatch, but for all subsequent call it will
ignore it.

Signed-off-by: Souvik Dey <sodey@rbbn.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..2ecf74b 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+		return err;
+	}
+	/*
+	 * In linux kernel driver on receiving ADD_VLAN it enables
+	 * VLAN_STRIP by default. So reconfigure the vlan_offload
+	 * as it was done by the app earlier.
+	 */
+	err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+	if (err)
+		PMD_DRV_LOG(ERR, "fail to execute command disable_vlan_strip "
+			"as a part of OP_ADD_VLAN");
 
 	return err;
 }
-- 
2.9.3.windows.1


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

* Re: [dpdk-dev] [PATCH] net/i40e: issue with ADD VLAN from Guest
  2020-12-09 17:55 [dpdk-dev] [PATCH] net/i40e: issue with ADD VLAN from Guest Souvik Dey
@ 2020-12-11  3:07 ` Guo, Jia
  2020-12-12 12:26   ` Dey, Souvik
  2020-12-12 13:05 ` [dpdk-dev] [PATCH v2] " Souvik Dey
  1 sibling, 1 reply; 13+ messages in thread
From: Guo, Jia @ 2020-12-11  3:07 UTC (permalink / raw)
  To: Souvik Dey, Xing, Beilei, Zhang, Qi Z; +Cc: dev

hi, souvik

From: Souvik Dey <sodey@rbbn.com>
Sent: Thursday, December 10, 2020 1:55 AM
To: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org; Souvik Dey <sodey@rbbn.com>
Subject: [PATCH] net/i40e: issue with ADD VLAN from Guest

In case of i40evf pmd, when ADD_VLAN is sent down the linux i40e driver,
along with add the vlan the kernel driver also enables the vlan stripping
by default.
This might have issues if the app configured DEV_RX_OFFLOAD_VLAN_STRIP
as off at the port configuration. The app after adding the VLAN will
expect the VLAN to be coming in the received packets but, due to
VLAN_STRIP enabled at the PF, it will get stripped.
This behavior of the Linux driver causes confussion with the DPDK app
using i40e_pmd. So it is better to reconfigure the vlan_offload, which
checks for DEV_RX_OFFLOAD_VLAN_STRIP flag in the dev_conf and enables or
disables the vlan strip in the PF.
Also rte_eth_dev_set_vlan_offload() to disable VLAN_STRIP cannot be used
from the application, as this will only work for the first time when
original and current confi mismatch, but for all subsequent call it will
ignore it.

Thanks for your patch and the clear detail interpret, what you said is reset the configuration about vlan strip that
would be change by the pf driver when adding vlan from vf, so I think concentrate<http://www.baidu.com/link?url=FKtXEQdgI9iuD4HN0uZIpf5hHCbYVGlPJx-jzQ-bGjXQiZutjIi6XT6rPlEoGfM13HvgvT9Gt88OAPmEKXNitp-rKK4k50M6di1HF4bDMxa&wd=&eqid=e7f451dc0001a611000000045fd2e0e1> the commit log to be more simple
in your way would be enough.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
---
drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..2ecf74b 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
args.out_buffer = vf->aq_resp;
args.out_size = I40E_AQ_BUF_SZ;
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+ return err;

Would it be more code clean to use “goto err;”?

+ }
+ /*

/* -> /**, that would what I suggest.

+ * In linux kernel driver on receiving ADD_VLAN it enables
+ * VLAN_STRIP by default. So reconfigure the vlan_offload
+ * as it was done by the app earlier.
+ */
+ err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+ if (err)
+ PMD_DRV_LOG(ERR, "fail to execute command disable_vlan_strip "
+ "as a part of OP_ADD_VLAN");

If it is not coming as a command, please refine this log, such as “fail to set vlan strip.”


return err;
}
--
2.9.3.windows.1

________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* Re: [dpdk-dev] [PATCH] net/i40e: issue with ADD VLAN from Guest
  2020-12-11  3:07 ` Guo, Jia
@ 2020-12-12 12:26   ` Dey, Souvik
  0 siblings, 0 replies; 13+ messages in thread
From: Dey, Souvik @ 2020-12-12 12:26 UTC (permalink / raw)
  To: Guo, Jia, Xing, Beilei, Zhang, Qi Z; +Cc: dev

Hi Guo,
              Thanks for the comments. I will upload a v2 of the patch.

--
Regards,
Souvik

From: Guo, Jia <jia.guo@intel.com>
Sent: Thursday, December 10, 2020 10:08 PM
To: Dey, Souvik <sodey@rbbn.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: RE: [PATCH] net/i40e: issue with ADD VLAN from Guest

________________________________
NOTICE: This email was received from an EXTERNAL sender
________________________________

hi, souvik

From: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Sent: Thursday, December 10, 2020 1:55 AM
To: Xing, Beilei <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>; Guo, Jia <jia.guo@intel.com<mailto:jia.guo@intel.com>>; Zhang, Qi Z <qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Subject: [PATCH] net/i40e: issue with ADD VLAN from Guest

In case of i40evf pmd, when ADD_VLAN is sent down the linux i40e driver,
along with add the vlan the kernel driver also enables the vlan stripping
by default.
This might have issues if the app configured DEV_RX_OFFLOAD_VLAN_STRIP
as off at the port configuration. The app after adding the VLAN will
expect the VLAN to be coming in the received packets but, due to
VLAN_STRIP enabled at the PF, it will get stripped.
This behavior of the Linux driver causes confussion with the DPDK app
using i40e_pmd. So it is better to reconfigure the vlan_offload, which
checks for DEV_RX_OFFLOAD_VLAN_STRIP flag in the dev_conf and enables or
disables the vlan strip in the PF.
Also rte_eth_dev_set_vlan_offload() to disable VLAN_STRIP cannot be used
from the application, as this will only work for the first time when
original and current confi mismatch, but for all subsequent call it will
ignore it.


Thanks for your patch and the clear detail interpret, what you said is reset the configuration about vlan strip that
would be change by the pf driver when adding vlan from vf, so I think concentrate<http://www.baidu.com/link?url=FKtXEQdgI9iuD4HN0uZIpf5hHCbYVGlPJx-jzQ-bGjXQiZutjIi6XT6rPlEoGfM13HvgvT9Gt88OAPmEKXNitp-rKK4k50M6di1HF4bDMxa&wd=&eqid=e7f451dc0001a611000000045fd2e0e1> the commit log to be more simple
in your way would be enough.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
---
drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..2ecf74b 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
args.out_buffer = vf->aq_resp;
args.out_size = I40E_AQ_BUF_SZ;
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+ return err;

Would it be more code clean to use “goto err;”?

+ }
+ /*

/* -> /**, that would what I suggest.

+ * In linux kernel driver on receiving ADD_VLAN it enables
+ * VLAN_STRIP by default. So reconfigure the vlan_offload
+ * as it was done by the app earlier.
+ */
+ err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+ if (err)
+ PMD_DRV_LOG(ERR, "fail to execute command disable_vlan_strip "
+ "as a part of OP_ADD_VLAN");

If it is not coming as a command, please refine this log, such as “fail to set vlan strip.”


return err;
}
--
2.9.3.windows.1


________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* [dpdk-dev] [PATCH v2] net/i40e: issue with ADD VLAN from Guest
  2020-12-09 17:55 [dpdk-dev] [PATCH] net/i40e: issue with ADD VLAN from Guest Souvik Dey
  2020-12-11  3:07 ` Guo, Jia
@ 2020-12-12 13:05 ` Souvik Dey
  2020-12-15  2:24   ` Guo, Jia
  2020-12-15 13:28   ` [dpdk-dev] [PATCH v3] " Souvik Dey
  1 sibling, 2 replies; 13+ messages in thread
From: Souvik Dey @ 2020-12-12 13:05 UTC (permalink / raw)
  To: beilei.xing, jia.guo, qi.z.zhang; +Cc: dev, Souvik Dey

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com>
---
v2:
* Simplied the commit log.
* goto err; is not required as it has only one more return path
and there is no cleanup required apart from just return err.
* Updated the comment start from /* -> /**
* Changed the error log in case vlan strip command fails.
---
 drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..d3dbcb5 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+		return err;
+	}
+	/**
+	 * In linux kernel driver on receiving ADD_VLAN it enables
+	 * VLAN_STRIP by default. So reconfigure the vlan_offload
+	 * as it was done by the app earlier.
+	 */
+	err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+	if (err)
+		PMD_DRV_LOG(ERR, "fail to set vlan_strip "
+			"as a part of OP_ADD_VLAN");
 
 	return err;
 }
-- 
2.9.3.windows.1


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: issue with ADD VLAN from Guest
  2020-12-12 13:05 ` [dpdk-dev] [PATCH v2] " Souvik Dey
@ 2020-12-15  2:24   ` Guo, Jia
  2020-12-15 13:16     ` Dey, Souvik
  2020-12-15 13:28   ` [dpdk-dev] [PATCH v3] " Souvik Dey
  1 sibling, 1 reply; 13+ messages in thread
From: Guo, Jia @ 2020-12-15  2:24 UTC (permalink / raw)
  To: Souvik Dey, Xing, Beilei, Zhang, Qi Z; +Cc: dev


From: Souvik Dey <sodey@rbbn.com>
Sent: Saturday, December 12, 2020 9:05 PM
To: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org; Souvik Dey <sodey@rbbn.com>
Subject: [PATCH v2] net/i40e: issue with ADD VLAN from Guest

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
---
v2:
* Simplied the commit log.
* goto err; is not required as it has only one more return path
and there is no cleanup required apart from just return err.
* Updated the comment start from /* -> /**
* Changed the error log in case vlan strip command fails.
---
drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..d3dbcb5 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
args.out_buffer = vf->aq_resp;
args.out_size = I40E_AQ_BUF_SZ;
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+ return err;
+ }
+ /**
+ * In linux kernel driver on receiving ADD_VLAN it enables
+ * VLAN_STRIP by default. So reconfigure the vlan_offload
+ * as it was done by the app earlier.
+ */
+ err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+ if (err)
+ PMD_DRV_LOG(ERR, "fail to set vlan_strip "
+ "as a part of OP_ADD_VLAN");

What I mean is that “PMD_DRV_LOG(ERR, "fail to set vlan strip");” is enough, since it will make the driver log to be
more simple. And also avoid a quoted string split across lines which let the second line could not hit your expectation.
Other look good, if no other comment, please add my ACK in your next version, thanks.

return err;
}
--
2.9.3.windows.1

________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* Re: [dpdk-dev] [PATCH v2] net/i40e: issue with ADD VLAN from Guest
  2020-12-15  2:24   ` Guo, Jia
@ 2020-12-15 13:16     ` Dey, Souvik
  2020-12-15 13:30       ` Dey, Souvik
  0 siblings, 1 reply; 13+ messages in thread
From: Dey, Souvik @ 2020-12-15 13:16 UTC (permalink / raw)
  To: Guo, Jia, Xing, Beilei, Zhang, Qi Z; +Cc: dev

Hi Guo,

Ok sure will remove the extra log and submit the version 3. I have not received any other comments , so will also add your ACK in the next version then.

--
Regards,
Souvik

From: Guo, Jia <jia.guo@intel.com>
Sent: Monday, December 14, 2020 9:24 PM
To: Dey, Souvik <sodey@rbbn.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: RE: [PATCH v2] net/i40e: issue with ADD VLAN from Guest

________________________________
NOTICE: This email was received from an EXTERNAL sender
________________________________


From: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Sent: Saturday, December 12, 2020 9:05 PM
To: Xing, Beilei <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>; Guo, Jia <jia.guo@intel.com<mailto:jia.guo@intel.com>>; Zhang, Qi Z <qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Subject: [PATCH v2] net/i40e: issue with ADD VLAN from Guest

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
---
v2:
* Simplied the commit log.
* goto err; is not required as it has only one more return path
and there is no cleanup required apart from just return err.
* Updated the comment start from /* -> /**
* Changed the error log in case vlan strip command fails.
---
drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..d3dbcb5 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
args.out_buffer = vf->aq_resp;
args.out_size = I40E_AQ_BUF_SZ;
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+ return err;
+ }
+ /**
+ * In linux kernel driver on receiving ADD_VLAN it enables
+ * VLAN_STRIP by default. So reconfigure the vlan_offload
+ * as it was done by the app earlier.
+ */
+ err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+ if (err)
+ PMD_DRV_LOG(ERR, "fail to set vlan_strip "
+ "as a part of OP_ADD_VLAN");


What I mean is that “PMD_DRV_LOG(ERR, "fail to set vlan strip");” is enough, since it will make the driver log to be
more simple. And also avoid a quoted string split across lines which let the second line could not hit your expectation.
Other look good, if no other comment, please add my ACK in your next version, thanks.

return err;
}
--
2.9.3.windows.1


________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from Guest
  2020-12-12 13:05 ` [dpdk-dev] [PATCH v2] " Souvik Dey
  2020-12-15  2:24   ` Guo, Jia
@ 2020-12-15 13:28   ` Souvik Dey
  2020-12-16  2:09     ` Guo, Jia
  1 sibling, 1 reply; 13+ messages in thread
From: Souvik Dey @ 2020-12-15 13:28 UTC (permalink / raw)
  To: beilei.xing, jia.guo, qi.z.zhang; +Cc: dev, Souvik Dey

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com>
---
v3:
* Changed the error log in case vlan strip command fails
as per the latest comment.
---
v2:
* Simplied the commit log.
* goto err; is not required as it has only one more return path
and there is no cleanup required apart from just return err.
* Updated the comment start from /* -> /**
* Changed the error log in case vlan strip command fails.
---

 drivers/net/i40e/i40e_ethdev_vf.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..2faee1d 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,18 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
 	args.out_buffer = vf->aq_resp;
 	args.out_size = I40E_AQ_BUF_SZ;
 	err = i40evf_execute_vf_cmd(dev, &args);
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+		return err;
+	}
+	/**
+	 * In linux kernel driver on receiving ADD_VLAN it enables
+	 * VLAN_STRIP by default. So reconfigure the vlan_offload
+	 * as it was done by the app earlier.
+	 */
+	err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+	if (err)
+		PMD_DRV_LOG(ERR, "fail to set vlan_strip");
 
 	return err;
 }
-- 
2.9.3.windows.1


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

* Re: [dpdk-dev] [PATCH v2] net/i40e: issue with ADD VLAN from Guest
  2020-12-15 13:16     ` Dey, Souvik
@ 2020-12-15 13:30       ` Dey, Souvik
  0 siblings, 0 replies; 13+ messages in thread
From: Dey, Souvik @ 2020-12-15 13:30 UTC (permalink / raw)
  To: Guo, Jia, Xing, Beilei, Zhang, Qi Z; +Cc: dev

Hi Guo,
Raised the v3 of the patch , but was not sure how to put the ACK. If you don’t mind can you please ACK the same. Thanks.
https://patchwork.dpdk.org/patch/85210/

--
Regards,
Souvik

From: Dey, Souvik
Sent: Tuesday, December 15, 2020 8:16 AM
To: Guo, Jia <jia.guo@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: RE: [PATCH v2] net/i40e: issue with ADD VLAN from Guest

Hi Guo,

Ok sure will remove the extra log and submit the version 3. I have not received any other comments , so will also add your ACK in the next version then.

--
Regards,
Souvik

From: Guo, Jia <jia.guo@intel.com<mailto:jia.guo@intel.com>>
Sent: Monday, December 14, 2020 9:24 PM
To: Dey, Souvik <sodey@rbbn.com<mailto:sodey@rbbn.com>>; Xing, Beilei <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>; Zhang, Qi Z <qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>
Subject: RE: [PATCH v2] net/i40e: issue with ADD VLAN from Guest

________________________________
NOTICE: This email was received from an EXTERNAL sender
________________________________


From: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Sent: Saturday, December 12, 2020 9:05 PM
To: Xing, Beilei <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>; Guo, Jia <jia.guo@intel.com<mailto:jia.guo@intel.com>>; Zhang, Qi Z <qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Subject: [PATCH v2] net/i40e: issue with ADD VLAN from Guest

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
---
v2:
* Simplied the commit log.
* goto err; is not required as it has only one more return path
and there is no cleanup required apart from just return err.
* Updated the comment start from /* -> /**
* Changed the error log in case vlan strip command fails.
---
drivers/net/i40e/i40e_ethdev_vf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..d3dbcb5 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,19 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
args.out_buffer = vf->aq_resp;
args.out_size = I40E_AQ_BUF_SZ;
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+ return err;
+ }
+ /**
+ * In linux kernel driver on receiving ADD_VLAN it enables
+ * VLAN_STRIP by default. So reconfigure the vlan_offload
+ * as it was done by the app earlier.
+ */
+ err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+ if (err)
+ PMD_DRV_LOG(ERR, "fail to set vlan_strip "
+ "as a part of OP_ADD_VLAN");

What I mean is that “PMD_DRV_LOG(ERR, "fail to set vlan strip");” is enough, since it will make the driver log to be
more simple. And also avoid a quoted string split across lines which let the second line could not hit your expectation.
Other look good, if no other comment, please add my ACK in your next version, thanks.

return err;
}
--
2.9.3.windows.1

________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from Guest
  2020-12-15 13:28   ` [dpdk-dev] [PATCH v3] " Souvik Dey
@ 2020-12-16  2:09     ` Guo, Jia
  2020-12-23 10:51       ` Zhang, Qi Z
  2021-01-04 16:42       ` Ferruh Yigit
  0 siblings, 2 replies; 13+ messages in thread
From: Guo, Jia @ 2020-12-16  2:09 UTC (permalink / raw)
  To: Souvik Dey, Xing, Beilei, Zhang, Qi Z; +Cc: dev

Acked-by: Jeff Guo <jia.guo@intel.com<mailto:jia.guo@intel.com>>

From: Souvik Dey <sodey@rbbn.com>
Sent: Tuesday, December 15, 2020 9:28 PM
To: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org; Souvik Dey <sodey@rbbn.com>
Subject: [PATCH v3] net/i40e: issue with ADD VLAN from Guest

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
---
v3:
* Changed the error log in case vlan strip command fails
as per the latest comment.
---
v2:
* Simplied the commit log.
* goto err; is not required as it has only one more return path
and there is no cleanup required apart from just return err.
* Updated the comment start from /* -> /**
* Changed the error log in case vlan strip command fails.
---

drivers/net/i40e/i40e_ethdev_vf.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index c26b036..2faee1d 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1078,8 +1078,18 @@ i40evf_add_vlan(struct rte_eth_dev *dev, uint16_t vlanid)
args.out_buffer = vf->aq_resp;
args.out_size = I40E_AQ_BUF_SZ;
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err) {
PMD_DRV_LOG(ERR, "fail to execute command OP_ADD_VLAN");
+ return err;
+ }
+ /**
+ * In linux kernel driver on receiving ADD_VLAN it enables
+ * VLAN_STRIP by default. So reconfigure the vlan_offload
+ * as it was done by the app earlier.
+ */
+ err = i40evf_vlan_offload_set(dev, ETH_VLAN_STRIP_MASK);
+ if (err)
+ PMD_DRV_LOG(ERR, "fail to set vlan_strip");

return err;
}
--
2.9.3.windows.1

________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from Guest
  2020-12-16  2:09     ` Guo, Jia
@ 2020-12-23 10:51       ` Zhang, Qi Z
  2021-01-04 16:42       ` Ferruh Yigit
  1 sibling, 0 replies; 13+ messages in thread
From: Zhang, Qi Z @ 2020-12-23 10:51 UTC (permalink / raw)
  To: Guo, Jia, Souvik Dey, Xing, Beilei; +Cc: dev


From: Guo, Jia <jia.guo@intel.com>
Sent: Wednesday, December 16, 2020 10:10 AM
To: Souvik Dey <sodey@rbbn.com>; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Cc: dev@dpdk.org
Subject: RE: [PATCH v3] net/i40e: issue with ADD VLAN from Guest


Acked-by: Jeff Guo <jia.guo@intel.com<mailto:jia.guo@intel.com>>

From: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Sent: Tuesday, December 15, 2020 9:28 PM
To: Xing, Beilei <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>; Guo, Jia <jia.guo@intel.com<mailto:jia.guo@intel.com>>; Zhang, Qi Z <qi.z.zhang@intel.com<mailto:qi.z.zhang@intel.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Subject: [PATCH v3] net/i40e: issue with ADD VLAN from Guest

Reset the configuration of vlan strip that would be change
by the pf kernel driver when adding vlan from vf.
Application cannot use rte_eth_dev_set_vlan_offload() to set
the VLAN_STRIP, as this will only work for the first time when
original and current config mismatch, but for all subsequent call
it will be ignored.

Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
Applied to dpdk-next-net-intel

Thanks
Qi
________________________________
Notice: This e-mail together with any attachments may contain information of Ribbon Communications Inc. that is confidential and/or proprietary for the sole use of the intended recipient. Any review, disclosure, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please notify the sender immediately and then delete all copies, including any attachments.
________________________________

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

* Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from Guest
  2020-12-16  2:09     ` Guo, Jia
  2020-12-23 10:51       ` Zhang, Qi Z
@ 2021-01-04 16:42       ` Ferruh Yigit
  2021-01-05  3:07         ` Guo, Jia
  1 sibling, 1 reply; 13+ messages in thread
From: Ferruh Yigit @ 2021-01-04 16:42 UTC (permalink / raw)
  To: Guo, Jia, Souvik Dey, Xing, Beilei, Zhang, Qi Z
  Cc: dev, Kevin Traynor, Luca Boccassi

On 12/16/2020 2:09 AM, Guo, Jia wrote:
> Acked-by: Jeff Guo <jia.guo@intel.com<mailto:jia.guo@intel.com>>
> 
> From: Souvik Dey <sodey@rbbn.com>
> Sent: Tuesday, December 15, 2020 9:28 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Souvik Dey <sodey@rbbn.com>
> Subject: [PATCH v3] net/i40e: issue with ADD VLAN from Guest
> 
> Reset the configuration of vlan strip that would be change
> by the pf kernel driver when adding vlan from vf.
> Application cannot use rte_eth_dev_set_vlan_offload() to set
> the VLAN_STRIP, as this will only work for the first time when
> original and current config mismatch, but for all subsequent call
> it will be ignored.
> 
> Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>

I suggest title: "net/i40e: fix VLAN stripping in VF"
Will update the title and some wording in the commit log while merging.

And I assume this should be backported, so will add stable@dpdk.org tag,
but can you please confirm the Linux PF behavior was always same?

And if the Linux PF behavior was always same, should we add the DPDK commit as 
fixes commit, @Qi, @Jeff, what do you think?

Other question is, does Linux PF and DPDK PF behave differently on enabling VLAN 
stripping and should it be unified to be consistent?


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

* Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from Guest
  2021-01-04 16:42       ` Ferruh Yigit
@ 2021-01-05  3:07         ` Guo, Jia
  2021-01-05 10:45           ` Ferruh Yigit
  0 siblings, 1 reply; 13+ messages in thread
From: Guo, Jia @ 2021-01-05  3:07 UTC (permalink / raw)
  To: Yigit, Ferruh, Souvik Dey, Xing, Beilei, Zhang, Qi Z
  Cc: dev, Kevin Traynor, Luca Boccassi


> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, January 5, 2021 12:43 AM
> To: Guo, Jia <jia.guo@intel.com>; Souvik Dey <sodey@rbbn.com>; Xing,
> Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> <bluca@debian.org>
> Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from
> Guest
> 
> On 12/16/2020 2:09 AM, Guo, Jia wrote:
> > Acked-by: Jeff Guo <jia.guo@intel.com<mailto:jia.guo@intel.com>>
> >
> > From: Souvik Dey <sodey@rbbn.com>
> > Sent: Tuesday, December 15, 2020 9:28 PM
> > To: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> > <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Souvik Dey <sodey@rbbn.com>
> > Subject: [PATCH v3] net/i40e: issue with ADD VLAN from Guest
> >
> > Reset the configuration of vlan strip that would be change by the pf
> > kernel driver when adding vlan from vf.
> > Application cannot use rte_eth_dev_set_vlan_offload() to set the
> > VLAN_STRIP, as this will only work for the first time when original
> > and current config mismatch, but for all subsequent call it will be
> > ignored.
> >
> > Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
> 
> I suggest title: "net/i40e: fix VLAN stripping in VF"
> Will update the title and some wording in the commit log while merging.
> 
> And I assume this should be backported, so will add stable@dpdk.org tag,
> but can you please confirm the Linux PF behavior was always same?
> 
> And if the Linux PF behavior was always same, should we add the DPDK
> commit as fixes commit, @Qi, @Jeff, what do you think?
> 
> Other question is, does Linux PF and DPDK PF behave differently on enabling
> VLAN stripping and should it be unified to be consistent?

Yes, it does have different behave when enabling VLAN stripping in DPDK PF and Linux PF, and not always the same in linux PFs, but this patch set could be an workaround in DPDK PF for the compatibility.


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

* Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from Guest
  2021-01-05  3:07         ` Guo, Jia
@ 2021-01-05 10:45           ` Ferruh Yigit
  0 siblings, 0 replies; 13+ messages in thread
From: Ferruh Yigit @ 2021-01-05 10:45 UTC (permalink / raw)
  To: Guo, Jia, Souvik Dey, Xing, Beilei, Zhang, Qi Z
  Cc: dev, Kevin Traynor, Luca Boccassi

On 1/5/2021 3:07 AM, Guo, Jia wrote:
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, January 5, 2021 12:43 AM
>> To: Guo, Jia <jia.guo@intel.com>; Souvik Dey <sodey@rbbn.com>; Xing,
>> Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
>> <bluca@debian.org>
>> Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: issue with ADD VLAN from
>> Guest
>>
>> On 12/16/2020 2:09 AM, Guo, Jia wrote:
>>> Acked-by: Jeff Guo <jia.guo@intel.com<mailto:jia.guo@intel.com>>
>>>
>>> From: Souvik Dey <sodey@rbbn.com>
>>> Sent: Tuesday, December 15, 2020 9:28 PM
>>> To: Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
>>> <jia.guo@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>>> Cc: dev@dpdk.org; Souvik Dey <sodey@rbbn.com>
>>> Subject: [PATCH v3] net/i40e: issue with ADD VLAN from Guest
>>>
>>> Reset the configuration of vlan strip that would be change by the pf
>>> kernel driver when adding vlan from vf.
>>> Application cannot use rte_eth_dev_set_vlan_offload() to set the
>>> VLAN_STRIP, as this will only work for the first time when original
>>> and current config mismatch, but for all subsequent call it will be
>>> ignored.
>>>
>>> Signed-off-by: Souvik Dey <sodey@rbbn.com<mailto:sodey@rbbn.com>>
>>
>> I suggest title: "net/i40e: fix VLAN stripping in VF"
>> Will update the title and some wording in the commit log while merging.
>>
>> And I assume this should be backported, so will add stable@dpdk.org tag,
>> but can you please confirm the Linux PF behavior was always same?
>>
>> And if the Linux PF behavior was always same, should we add the DPDK
>> commit as fixes commit, @Qi, @Jeff, what do you think?
>>
>> Other question is, does Linux PF and DPDK PF behave differently on enabling
>> VLAN stripping and should it be unified to be consistent?
> 
> Yes, it does have different behave when enabling VLAN stripping in DPDK PF and Linux PF, and not always the same in linux PFs, but this patch set could be an workaround in DPDK PF for the compatibility.
> 

OK, and since the patch is safe from Linux PF point of view, I will add 
following fixline, which is the initial DPDK code that adds VF VLAN add support:

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Cc: stable@dpdk.org

--
Thanks,
ferruh

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

end of thread, other threads:[~2021-01-05 10:45 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09 17:55 [dpdk-dev] [PATCH] net/i40e: issue with ADD VLAN from Guest Souvik Dey
2020-12-11  3:07 ` Guo, Jia
2020-12-12 12:26   ` Dey, Souvik
2020-12-12 13:05 ` [dpdk-dev] [PATCH v2] " Souvik Dey
2020-12-15  2:24   ` Guo, Jia
2020-12-15 13:16     ` Dey, Souvik
2020-12-15 13:30       ` Dey, Souvik
2020-12-15 13:28   ` [dpdk-dev] [PATCH v3] " Souvik Dey
2020-12-16  2:09     ` Guo, Jia
2020-12-23 10:51       ` Zhang, Qi Z
2021-01-04 16:42       ` Ferruh Yigit
2021-01-05  3:07         ` Guo, Jia
2021-01-05 10:45           ` Ferruh Yigit

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