net/i40e: stop lldp before setting local lldp MIB

PATCH Superseded Chas Williams 2 participants 9 messages Aug 1, 2018 – Sep 13, 2018 Network drivers

lore

Share & tools

lore

Chas Williams

author

2018-08-01 04:07 UTC #0 lore raw

PATCH 1/1 net/i40e: stop lldp before setting local lldp MIB

From: "Charles (Chas) Williams" <[email protected]>

Quoted 1 line

From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction

Update:

Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.

This results in the harmless, but annoying, diagnostic:

default dcb config fails. err = -53, aq_err = 1.

So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.

Signed-off-by Chas Williams <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/i40e/i40e_ethdev.c +1 -0

index a340540ef..03bedf5c1 100644
@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev *dev, bool sw_dcb)
 		 * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
 		 * adminq status. Otherwise, it should return success.
 		 */
+		i40e_aq_stop_lldp(hw, TRUE, NULL);
 		if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
 		    hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {
 			memset(&hw->local_dcbx_config, 0,

Signature · 1 line

2.14.4

Zhang, Qi Z

2018-08-01 14:00 UTC #1 lore raw

Hi Williams:

Quoted 36 lines

-----Original Message-----
From: Chas Williams [mailto:[email protected]]
Sent: Wednesday, August 1, 2018 12:07 PM
To: [email protected]
Cc: Xing, Beilei <[email protected]>; Zhang, Qi Z <[email protected]>; Charles (Chas) Williams <[email protected]>
Subject: [PATCH] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.
This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.
Signed-off-by Chas Williams <[email protected]>
---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/i40e/i40e_ethdev.c +0 -0
index a340540ef..03bedf5c1 100644

drivers/net/i40e/i40e_ethdev.c i40e_dcb_init_configure(struct rte_eth_dev · line 11241

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +1 -0

@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev
 *dev, bool sw_dcb)
 		 * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
 		 * adminq status. Otherwise, it should return success.
 		 */
+		i40e_aq_stop_lldp(hw, TRUE, NULL);

I40e_aq_stop_lldp is intended to be removed with below two patches.

commit c6431c891d9e9691e3205fe5c5350071cbaeb852
commit fcbd40d4327b36725c4de9f33f57809edc359f4a

Regards.
Qi

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +0 -0
 		if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
 		    hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {
memset(&hw->local_dcbx_config, 0,
--
2.14.4

Chas Williams

author

2018-08-01 15:31 UTC #2 lore raw

Quoted 48 lines · Qi Z

Hi Williams:
Quoted 5 lines
-----Original Message-----
From: Chas Williams [mailto:[email protected]]
Sent: Wednesday, August 1, 2018 12:07 PM
To: [email protected]
Cc: Xing, Beilei <[email protected]>; Zhang, Qi Z <
[email protected]>;
Quoted 21 lines
Charles (Chas) Williams <[email protected]>
Subject: [PATCH] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.
This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.
Review
Quoted 6 lines
Signed-off-by Chas Williams <[email protected]>
---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c
drivers/net/i40e/i40e_ethdev.c i40e_dcb_init_configure(struct rte_eth_dev · line 11241
Quoted 9 lines
index a340540ef..03bedf5c1 100644
drivers/net/i40e/i40e_ethdev.c +1 -0
@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev
 *dev, bool sw_dcb)
               * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
               * adminq status. Otherwise, it should return success.
               */
+             i40e_aq_stop_lldp(hw, TRUE, NULL);
I40e_aq_stop_lldp is intended to be removed with below two patches.

Quoted 3 lines

commit c6431c891d9e9691e3205fe5c5350071cbaeb852
commit fcbd40d4327b36725c4de9f33f57809edc359f4a

Yeah about that. The i40e driver seems to go out of its way to program the flow
control outside of LLDP see i40e_update_flow_control()
and i40e_flow_ctrl_set().
So it's not clear to me what is going on here with DPDK and LLDP. Do you really
want LLDP running in this mode? The other branch
in i40e_dcb_init_configure()
explicitly starts LLDP, so I assume at some point, LLDP wasn't/isn't running. How did
it get this way? With respect to item #70, shouldn't you therefore always start
LLDP to workaround the bug?

Item#70 is fixed in later NVM's (6.01 or later). Perhaps a check to see if the NVM
is new enough to safely allow LLDP stopping?

Quoted 12 lines

Regards.
Qi

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +0 -0

              if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
                  hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {

                      memset(&hw->local_dcbx_config, 0,

--
2.14.4

Zhang, Qi Z

2018-08-02 02:16 UTC #3 lore raw

From: Chas Williams [mailto:[email protected]]
Sent: Wednesday, August 1, 2018 11:31 PM
To: Zhang, Qi Z <[email protected]>
Cc: [email protected]; Xing, Beilei <[email protected]>; Chas Williams <[email protected]>
Subject: Re: [PATCH] net/i40e: stop lldp before setting local lldp MIB

On Wed, Aug 1, 2018 at 10:00 AM Zhang, Qi Z <[email protected]<mailto:[email protected]>> wrote:
Hi Williams:

Quoted 36 lines

-----Original Message-----
From: Chas Williams [mailto:[email protected]<mailto:[email protected]>]
Sent: Wednesday, August 1, 2018 12:07 PM
To: [email protected]<mailto:[email protected]>
Cc: Xing, Beilei <[email protected]<mailto:[email protected]>>; Zhang, Qi Z <[email protected]<mailto:[email protected]>>; Charles (Chas) Williams <[email protected]<mailto:[email protected]>>
Subject: [PATCH] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]<mailto:[email protected]>>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.
This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.
Signed-off-by Chas Williams <[email protected]<mailto:[email protected]>>
---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/i40e/i40e_ethdev.c +0 -0
index a340540ef..03bedf5c1 100644

drivers/net/i40e/i40e_ethdev.c i40e_dcb_init_configure(struct rte_eth_dev · line 11241

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +1 -0

@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev
 *dev, bool sw_dcb)
               * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
               * adminq status. Otherwise, it should return success.
               */
+             i40e_aq_stop_lldp(hw, TRUE, NULL);

I40e_aq_stop_lldp is intended to be removed with below two patches.

commit c6431c891d9e9691e3205fe5c5350071cbaeb852
commit fcbd40d4327b36725c4de9f33f57809edc359f4a

Yeah about that. The i40e driver seems to go out of its way to program the flow control outside of LLDP see i40e_update_flow_control() and i40e_flow_ctrl_set(). So it's not clear to me what is going on here with DPDK and LLDP. Do you really want LLDP running in this mode? The other branch in i40e_dcb_init_configure() explicitly starts LLDP, so I assume at some point, LLDP wasn't/isn't running. How did it get this way? With respect to item #70, shouldn't you therefore always start LLDP to workaround the bug?

LLDP is enabled by firmware as default, from my view it is not necessary to start LLDP, if We never stop it, and maybe we should not allow the case sw_dcb = TRUE since we can’t stop LLDP.

Item#70 is fixed in later NVM's (6.01 or later). Perhaps a check to see if the NVM is new enough to safely allow LLDP stopping?

Yes, I agree, would you mind to add a firmware check for lldp_start and lldp_stop in v2? So that fix your concern in the case with a new firmware For old firmware case, let’s just keep exist way , there should be some re-work in future, and more test and review is required.

Thanks
Qi

Regards.
Qi

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +0 -0

              if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
                  hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {

                      memset(&hw->local_dcbx_config, 0,

--
2.14.4

Chas Williams

author

2018-08-02 21:19 UTC #4 lore raw

Quoted 107 lines · Qi Z

*From:* Chas Williams [mailto:[email protected]]
*Sent:* Wednesday, August 1, 2018 11:31 PM
*To:* Zhang, Qi Z <[email protected]>
*Cc:* [email protected]; Xing, Beilei <[email protected]>; Chas Williams < [email protected]>
*Subject:* Re: [PATCH] net/i40e: stop lldp before setting local lldp MIB
On Wed, Aug 1, 2018 at 10:00 AM Zhang, Qi Z <[email protected]> wrote:
Hi Williams:
Quoted 5 lines
-----Original Message-----
From: Chas Williams [mailto:[email protected]]
Sent: Wednesday, August 1, 2018 12:07 PM
To: [email protected]
Cc: Xing, Beilei <[email protected]>; Zhang, Qi Z <
[email protected]>;
Quoted 21 lines
Charles (Chas) Williams <[email protected]>
Subject: [PATCH] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.
This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.
Review
Quoted 6 lines
Signed-off-by Chas Williams <[email protected]>
---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c
drivers/net/i40e/i40e_ethdev.c i40e_dcb_init_configure(struct rte_eth_dev · line 11241
Quoted 9 lines
index a340540ef..03bedf5c1 100644
drivers/net/i40e/i40e_ethdev.c +1 -0
@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev
 *dev, bool sw_dcb)
               * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
               * adminq status. Otherwise, it should return success.
               */
+             i40e_aq_stop_lldp(hw, TRUE, NULL);
I40e_aq_stop_lldp is intended to be removed with below two patches.
commit c6431c891d9e9691e3205fe5c5350071cbaeb852
commit fcbd40d4327b36725c4de9f33f57809edc359f4a
Yeah about that. The i40e driver seems to go out of its way to program the flow
control outside of LLDP see i40e_update_flow_control() and i40e_flow_ctrl_set().
So it's not clear to me what is going on here with DPDK and LLDP. Do you really
want LLDP running in this mode? The other branch
in i40e_dcb_init_configure()
explicitly starts LLDP, so I assume at some point, LLDP wasn't/isn't running. How did
it get this way? With respect to item #70, shouldn't you therefore always start
LLDP to workaround the bug?
LLDP is enabled by firmware as default, from my view it is not necessary to start LLDP, if We never stop it, and maybe we should not allow the case sw_dcb = TRUE since we can’t stop LLDP.
Item#70 is fixed in later NVM's (6.01 or later). Perhaps a check to see if the NVM
is new enough to safely allow LLDP stopping?

Quoted 6 lines

Yes, I agree, would you mind to add a firmware check for lldp_start and lldp_stop in v2? So that fix your concern in the case with a new firmware
For old firmware case, let’s just keep exist way , there should be some re-work in future, and more test and review is required.

Yes, I can probably manage to add a firmware check. I will need a little bit to test it.

One other question. What's up with i40e_update_flow_control()? Should this be called whenever the link state changes? Right now it only seems to be called when the device is started/initialized.

Quoted 22 lines

Thanks

Regards.
Qi

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +0 -0

              if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
                  hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {

                      memset(&hw->local_dcbx_config, 0,

--
2.14.4

Zhang, Qi Z

2018-08-03 02:10 UTC #5 lore raw

From: Chas Williams [mailto:[email protected]]
Sent: Friday, August 3, 2018 5:20 AM
To: Zhang, Qi Z <[email protected]>
Cc: [email protected]; Xing, Beilei <[email protected]>; Chas Williams <[email protected]>
Subject: Re: [PATCH] net/i40e: stop lldp before setting local lldp MIB

On Wed, Aug 1, 2018 at 10:16 PM Zhang, Qi Z <[email protected]<mailto:[email protected]>> wrote:

From: Chas Williams [mailto:[email protected]<mailto:[email protected]>]
Sent: Wednesday, August 1, 2018 11:31 PM
To: Zhang, Qi Z <[email protected]<mailto:[email protected]>>
Cc: [email protected]<mailto:[email protected]>; Xing, Beilei <[email protected]<mailto:[email protected]>>; Chas Williams <[email protected]<mailto:[email protected]>>
Subject: Re: [PATCH] net/i40e: stop lldp before setting local lldp MIB

On Wed, Aug 1, 2018 at 10:00 AM Zhang, Qi Z <[email protected]<mailto:[email protected]>> wrote:
Hi Williams:

Quoted 36 lines

-----Original Message-----
From: Chas Williams [mailto:[email protected]<mailto:[email protected]>]
Sent: Wednesday, August 1, 2018 12:07 PM
To: [email protected]<mailto:[email protected]>
Cc: Xing, Beilei <[email protected]<mailto:[email protected]>>; Zhang, Qi Z <[email protected]<mailto:[email protected]>>; Charles (Chas) Williams <[email protected]<mailto:[email protected]>>
Subject: [PATCH] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]<mailto:[email protected]>>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.
This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.
Signed-off-by Chas Williams <[email protected]<mailto:[email protected]>>
---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/i40e/i40e_ethdev.c +0 -0
index a340540ef..03bedf5c1 100644

drivers/net/i40e/i40e_ethdev.c i40e_dcb_init_configure(struct rte_eth_dev · line 11241

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +1 -0

@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev
 *dev, bool sw_dcb)
               * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
               * adminq status. Otherwise, it should return success.
               */
+             i40e_aq_stop_lldp(hw, TRUE, NULL);

I40e_aq_stop_lldp is intended to be removed with below two patches.

commit c6431c891d9e9691e3205fe5c5350071cbaeb852
commit fcbd40d4327b36725c4de9f33f57809edc359f4a

Yeah about that. The i40e driver seems to go out of its way to program the flow control outside of LLDP see i40e_update_flow_control() and i40e_flow_ctrl_set(). So it's not clear to me what is going on here with DPDK and LLDP. Do you really want LLDP running in this mode? The other branch in i40e_dcb_init_configure() explicitly starts LLDP, so I assume at some point, LLDP wasn't/isn't running. How did it get this way? With respect to item #70, shouldn't you therefore always start LLDP to workaround the bug?

LLDP is enabled by firmware as default, from my view it is not necessary to start LLDP, if We never stop it, and maybe we should not allow the case sw_dcb = TRUE since we can’t stop LLDP.

Item#70 is fixed in later NVM's (6.01 or later). Perhaps a check to see if the NVM is new enough to safely allow LLDP stopping?

Yes, I can probably manage to add a firmware check. I will need a little bit to test it.

[Qi] OK, just a reminder, this change will be deferred to v18.11.

One other question. What's up with i40e_update_flow_control()? Should this be called whenever the link state changes? Right now it only seems to be called when the device is started/initialized.

[Qi] I’m not very sure if the flow control mode will be changed during LSC, Did you see some real issue that related with it?

Thanks
Qi

Regards.
Qi

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +0 -0

              if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
                  hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {

                      memset(&hw->local_dcbx_config, 0,

--
2.14.4

Chas Williams

author

2018-08-06 19:03 UTC #6 lore raw

Quoted 141 lines · Qi Z

*From:* Chas Williams [mailto:[email protected]]
*Sent:* Friday, August 3, 2018 5:20 AM
*To:* Zhang, Qi Z <[email protected]>
*Cc:* [email protected]; Xing, Beilei <[email protected]>; Chas Williams < [email protected]>
*Subject:* Re: [PATCH] net/i40e: stop lldp before setting local lldp MIB
On Wed, Aug 1, 2018 at 10:16 PM Zhang, Qi Z <[email protected]> wrote:
*From:* Chas Williams [mailto:[email protected]]
*Sent:* Wednesday, August 1, 2018 11:31 PM
*To:* Zhang, Qi Z <[email protected]>
*Cc:* [email protected]; Xing, Beilei <[email protected]>; Chas Williams < [email protected]>
*Subject:* Re: [PATCH] net/i40e: stop lldp before setting local lldp MIB
On Wed, Aug 1, 2018 at 10:00 AM Zhang, Qi Z <[email protected]> wrote:
Hi Williams:
Quoted 5 lines
-----Original Message-----
From: Chas Williams [mailto:[email protected]]
Sent: Wednesday, August 1, 2018 12:07 PM
To: [email protected]
Cc: Xing, Beilei <[email protected]>; Zhang, Qi Z <
[email protected]>;
Quoted 21 lines
Charles (Chas) Williams <[email protected]>
Subject: [PATCH] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.
This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, always stop the lldp daemon when we are in software mode before we attempt to call i40e_set_dcb_config.
Review
Quoted 6 lines
Signed-off-by Chas Williams <[email protected]>
---
 drivers/net/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)
(quoted diff) +0 -0
b/drivers/net/i40e/i40e_ethdev.c
drivers/net/i40e/i40e_ethdev.c i40e_dcb_init_configure(struct rte_eth_dev · line 11241
Quoted 9 lines
index a340540ef..03bedf5c1 100644
drivers/net/i40e/i40e_ethdev.c +1 -0
@@ -11237,6 +11237,7 @@ i40e_dcb_init_configure(struct rte_eth_dev
 *dev, bool sw_dcb)
               * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM
               * adminq status. Otherwise, it should return success.
               */
+             i40e_aq_stop_lldp(hw, TRUE, NULL);
I40e_aq_stop_lldp is intended to be removed with below two patches.
commit c6431c891d9e9691e3205fe5c5350071cbaeb852
commit fcbd40d4327b36725c4de9f33f57809edc359f4a
Yeah about that. The i40e driver seems to go out of its way to program the flow
control outside of LLDP see i40e_update_flow_control() and i40e_flow_ctrl_set().
So it's not clear to me what is going on here with DPDK and LLDP. Do you really
want LLDP running in this mode? The other branch
in i40e_dcb_init_configure()
explicitly starts LLDP, so I assume at some point, LLDP wasn't/isn't running. How did
it get this way? With respect to item #70, shouldn't you therefore always start
LLDP to workaround the bug?
LLDP is enabled by firmware as default, from my view it is not necessary to start LLDP, if We never stop it, and maybe we should not allow the case sw_dcb = TRUE since we can’t stop LLDP.
Item#70 is fixed in later NVM's (6.01 or later). Perhaps a check to see if the NVM
is new enough to safely allow LLDP stopping?
Yes, I agree, would you mind to add a firmware check for lldp_start and lldp_stop in v2? So that fix your concern in the case with a new firmware
For old firmware case, let’s just keep exist way , there should be some re-work in future, and more test and review is required.
Yes, I can probably manage to add a firmware check. I will need a little bit to test it.
[Qi] OK, just a reminder, this change will be deferred to v18.11.

Quoted 9 lines

One other question. What's up with i40e_update_flow_control()? Should this be called whenever the link state changes? Right now it only seems to be called when the device is started/initialized.
[Qi] I’m not very sure if the flow control mode will be changed during LSC, Did you see some real issue that related with it?

There's no reason to believe that the other end wouldn't change the status of flow control. It just seems like this might be something you would want to do more often than device init/reset?

Quoted 26 lines

Thanks

Regards.
Qi

Quoted 5 lines

drivers/net/i40e/i40e_ethdev.c +0 -0

              if ((ret == I40E_SUCCESS) || (ret != I40E_SUCCESS &&
                  hw->aq.asq_last_status == I40E_AQ_RC_EPERM)) {

                      memset(&hw->local_dcbx_config, 0,

--
2.14.4

Chas Williams

author

2018-08-06 20:06 UTC #7 lore raw

PATCH v2 1/1 net/i40e: stop lldp before setting local lldp MIB

In reply to Chas Williams

From: "Charles (Chas) Williams" <[email protected]>

Quoted 1 line

From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction

Update:

This results in the harmless, but annoying, diagnostic:

default dcb config fails. err = -53, aq_err = 1.

So, if possible (older firmwares cannot safely stop LLDP), stop the lldp daemon when we are in software mod before we attempt to call i40e_set_dcb_config.

Signed-off-by Chas Williams <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/net/i40e/i40e_ethdev.c +10 -0

index a340540ef..de3761933 100644
@@ -11232,6 +11232,16 @@ i40e_dcb_init_configure(struct rte_eth_dev *dev, bool sw_dcb)
 	 * LLDP MIB change event.
 	 */
 	if (sw_dcb == TRUE) {
+		/* When using NVM 6.01 or later, the RX data path does
+		 * not hang if the FW LLDP is stopped.
+		 */
+		if (((hw->nvm.version >> 12) & 0xf) >= 6 &&
+		    ((hw->nvm.version >> 4) & 0xff) >= 1) {
+			ret = i40e_aq_stop_lldp(hw, TRUE, NULL);
+			if (ret != I40E_SUCCESS)
+				PMD_INIT_LOG(DEBUG, "Failed to stop lldp");
+		}
+
 		ret = i40e_init_dcb(hw);
 		/* If lldp agent is stopped, the return value from
 		 * i40e_init_dcb we expect is failure with I40E_AQ_RC_EPERM

Signature · 1 line

2.14.4

Zhang, Qi Z

Acked-by on v2 1/1 Applied

2018-09-13 12:30 UTC #8 lore raw

Quoted 21 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Chas Williams
Sent: Tuesday, August 7, 2018 4:06 AM
To: [email protected]
Cc: Xing, Beilei <[email protected]>; Zhang, Qi Z <[email protected]>; Charles (Chas) Williams <[email protected]>
Subject: [dpdk-dev] [PATCH v2] net/i40e: stop lldp before setting local lldp MIB
From: "Charles (Chas) Williams" <[email protected]>
From the Intel Ethernet Controller X710/XXV710/XL710 Specifiction Update:
Starting from NVM 5.02, if the Set Local LLDP MIB command is received while the DCBx specific agent is stopped, the command returns an EPERM error. If the command is received while the LLDP agent is stopped, it sets the local MIB without exchanging LLDP with peer, and returns SUCCESS.

Quoted 8 lines

This results in the harmless, but annoying, diagnostic:
default dcb config fails. err = -53, aq_err = 1.
So, if possible (older firmwares cannot safely stop LLDP), stop the lldp daemon when we are in software mod before we attempt to call i40e_set_dcb_config.
Signed-off-by Chas Williams <[email protected]>

Acked-by Qi Zhang <[email protected]>

Applied to dpdk-next-net-intel

Thanks
Qi