net/ixgbe: fix ixgbevf link status

PATCH Superseded Yanglong Wu 3 participants 6 messages Aug 10, 2018 – Nov 14, 2018 Network drivers

Share & tools

Yanglong Wu

author

2018-08-10 08:20 UTC #0 lore raw

PATCH 1/1 net/ixgbe: fix ixgbevf link status

For ixgbevf kernel driver, link status changes from down to up will trigger vf kernel driver send IXGBE_VF_RESET message to pf kernel driver, after this, vf kernel driver will disable and enable it self. By these series operations, the vf kernel driver report link up. Besides, all these operations handles in kernel thread. For DPDK user space driver, it only gets link status changes from down to up, but miss IXGBE_VF_RESET message sending and reset itself. If we will add fully implementation of link status change for DPDK user space driver, we need take much more modification. We have aligned that for link status changes from down to up we only notify link is up, users need to reset vf port.

Fixes dc66e5fd01b9 ("net/ixgbe: improve link state check on VF")

Signed-off-by Rosen Xu <[email protected]>

Signed-off-by Yanglong Wu <[email protected]>

---
 drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

drivers/net/ixgbe/ixgbe_ethdev.c +2 ~~-12~~

index 26b192737..4d5f4441b 100644
@@ -3865,13 +3865,12 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
 
 static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
-		   int *link_up, int wait_to_complete)
+		   int *link_up, __attribute__((unused)) int wait_to_complete)
 {
 	/**
 	 * for a quick link status checking, wait_to_compelet == 0,
 	 * skip PF link status checking
 	 */
-	bool no_pflink_check = wait_to_complete == 0;
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	struct ixgbe_mac_info *mac = &hw->mac;
 	uint32_t links_reg, in_msg;

@@ -3931,15 +3930,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 	default:
 		*speed = IXGBE_LINK_SPEED_UNKNOWN;
 	}
-
-	if (no_pflink_check) {
-		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
-			mac->get_link_status = true;
-		else
-			mac->get_link_status = false;
-
-		goto out;
-	}
 	/* if the read failed it could just be a mailbox collision, best wait
 	 * until we are called again and don't report an error
 	 */

@@ -3949,7 +3939,7 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 	if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
 		/* msg is not CTS and is NACK we must have lost CTS status */
 		if (in_msg & IXGBE_VT_MSGTYPE_NACK)
-			ret_val = -1;
+			mac->get_link_status = false;
 		goto out;
 	}

Signature · 1 line

2.11.0

Zhang, Qi Z

2018-09-06 05:33 UTC #1 lore raw

Hi Yanglong:

Quoted 8 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Friday, August 10, 2018 4:10 PM
To: [email protected]
Cc: Xu, Rosen <[email protected]>; Wang, Dong1 <[email protected]>; Zhang, Qi Z <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH] net/ixgbe: fix ixgbevf link status

Quoted 11 lines

For ixgbevf kernel driver, link status changes from down to up will trigger vf kernel driver send IXGBE_VF_RESET message to pf kernel driver, after this, vf kernel driver will disable and enable it self. By these series operations, the vf kernel driver report link up. Besides, all these operations handles in kernel thread.
For DPDK user space driver, it only gets link status changes from down to up, but miss IXGBE_VF_RESET message sending and reset itself.
If we will add fully implementation of link status change for DPDK user space driver, we need take much more modification. We have aligned that for link status changes from down to up we only notify link is up, users need to reset vf port.

OK, the commit log described the gap between kernel driver and DPDK driver, and the workaround which require proper change in application.
But what is the problem the patch is going to fix?, what's the relationship between the fix and commit log, I can't figure out.
Basically the patch remove the no_pflink_check branch, so it will always check with PF for the link status. I guess this help to guarantee that application can always get correct link status, but why we need that commit log here? would you explain more?

Quoted 15 lines

Fixes dc66e5fd01b9 ("net/ixgbe: improve link state check on VF")
Signed-off-by Rosen Xu <[email protected]>
Signed-off-by Yanglong Wu <[email protected]>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
drivers/net/ixgbe/ixgbe_ethdev.c +0 -0
index 26b192737..4d5f4441b 100644

drivers/net/ixgbe/ixgbe_ethdev.c ixgbevf_dev_info_get(struct rte_eth_dev *dev, · line 3868

Quoted 5 lines

drivers/net/ixgbe/ixgbe_ethdev.c +1 -1

@@ -3865,13 +3865,12 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
 
 static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
-		   int *link_up, int wait_to_complete)
+		   int *link_up, __attribute__((unused)) int wait_to_complete)

I think we can just remove "wait_to_complete", right?

Quoted 38 lines

drivers/net/ixgbe/ixgbe_ethdev.c +0 ~~-10~~

 {
 	/**
 	 * for a quick link status checking, wait_to_compelet == 0,
 	 * skip PF link status checking
 	 */
-	bool no_pflink_check = wait_to_complete == 0;
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	struct ixgbe_mac_info *mac = &hw->mac;
 	uint32_t links_reg, in_msg;

@@ -3931,15 +3930,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw,
 ixgbe_link_speed *speed,
 	default:
 		*speed = IXGBE_LINK_SPEED_UNKNOWN;
 	}
-
-	if (no_pflink_check) {
-		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
-			mac->get_link_status = true;
-		else
-			mac->get_link_status = false;
-
-		goto out;
-	}
 	/* if the read failed it could just be a mailbox collision, best wait
 	 * until we are called again and don't report an error

drivers/net/ixgbe/ixgbe_ethdev.c +1 -2

*/

@@ -3949,7 +3939,7 @@ ixgbevf_check_link(struct ixgbe_hw *hw,
 ixgbe_link_speed *speed,
 	if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
 		/* msg is not CTS and is NACK we must have lost CTS status */
 		if (in_msg & IXGBE_VT_MSGTYPE_NACK)
-			ret_val = -1;
+			mac->get_link_status = false;
 		goto out;
 	}
 
--

2.11.0

Wu, Yanglong

author

2018-10-26 06:10 UTC #2 lore raw

Hi, qi:

This patch we are going to fix the problem H3C: 82599 Niantic VF can't get correct link status when use cable :https://jira01.devtools.intel.com/browse/DPDK-5543?filter=-3

The commit log describe about the gap between kernel and dpdk and why we design this workaround to get right link status.

Regard
Yanglong wu

-----Original Message-----
From: Zhang, Qi Z
Sent: Thursday, September 6, 2018 1:34 PM
To: Wu, Yanglong <[email protected]>; [email protected]
Cc: Xu, Rosen <[email protected]>; Wang, Dong1 <[email protected]>
Subject: RE: [PATCH] net/ixgbe: fix ixgbevf link status

Hi Yanglong:

Quoted 9 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Friday, August 10, 2018 4:10 PM
To: [email protected]
Cc: Xu, Rosen <[email protected]>; Wang, Dong1 <[email protected]>; Zhang, Qi Z <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH] net/ixgbe: fix ixgbevf link status

Quoted 11 lines

For ixgbevf kernel driver, link status changes from down to up will trigger vf kernel driver send IXGBE_VF_RESET message to pf kernel driver, after this, vf kernel driver will disable and enable it self. By these series operations, the vf kernel driver report link up. Besides, all these operations handles in kernel thread.
For DPDK user space driver, it only gets link status changes from down to up, but miss IXGBE_VF_RESET message sending and reset itself. If we will add fully implementation of link status change for DPDK user space driver, we need take much more modification. We have aligned that for link status changes from down to up we only notify link is up, users need to reset vf port.

Quoted 15 lines

Fixes dc66e5fd01b9 ("net/ixgbe: improve link state check on VF")
Signed-off-by Rosen Xu <[email protected]>
Signed-off-by Yanglong Wu <[email protected]>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
drivers/net/ixgbe/ixgbe_ethdev.c +0 -0
index 26b192737..4d5f4441b 100644

drivers/net/ixgbe/ixgbe_ethdev.c ixgbevf_dev_info_get(struct rte_eth_dev *dev, · line 3868

Quoted 5 lines

drivers/net/ixgbe/ixgbe_ethdev.c +1 -1

@@ -3865,13 +3865,12 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
 
 static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
-		   int *link_up, int wait_to_complete)
+		   int *link_up, __attribute__((unused)) int wait_to_complete)

I think we can just remove "wait_to_complete", right?

Quoted 38 lines

drivers/net/ixgbe/ixgbe_ethdev.c +0 ~~-10~~

 {
 	/**
 	 * for a quick link status checking, wait_to_compelet == 0,
 	 * skip PF link status checking
 	 */
-	bool no_pflink_check = wait_to_complete == 0;
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	struct ixgbe_mac_info *mac = &hw->mac;
 	uint32_t links_reg, in_msg;

@@ -3931,15 +3930,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw, 
 ixgbe_link_speed *speed,
 	default:
 		*speed = IXGBE_LINK_SPEED_UNKNOWN;
 	}
-
-	if (no_pflink_check) {
-		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
-			mac->get_link_status = true;
-		else
-			mac->get_link_status = false;
-
-		goto out;
-	}
 	/* if the read failed it could just be a mailbox collision, best wait
 	 * until we are called again and don't report an error

drivers/net/ixgbe/ixgbe_ethdev.c +1 -2

*/

@@ -3949,7 +3939,7 @@ ixgbevf_check_link(struct ixgbe_hw *hw, 
 ixgbe_link_speed *speed,
 	if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
 		/* msg is not CTS and is NACK we must have lost CTS status */
 		if (in_msg & IXGBE_VT_MSGTYPE_NACK)
-			ret_val = -1;
+			mac->get_link_status = false;
 		goto out;
 	}
 
--

2.11.0

Yanglong Wu

author

2018-11-13 06:44 UTC #3 lore raw

PATCH v2 1/1 net/ixgbe: fix ixgbevf link status

In reply to Yanglong Wu

Fixes dc66e5fd01b9 ("net/ixgbe: improve link state check on VF")

Signed-off-by Rosen Xu <[email protected]>

Signed-off-by Yanglong Wu <[email protected]>

---
v2
change as comments

---
 drivers/net/ixgbe/ixgbe_ethdev.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

drivers/net/ixgbe/ixgbe_ethdev.c +1 ~~-14~~

index 8148577f5..91ba6201d 100644
@@ -3880,11 +3880,6 @@ static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 		   int *link_up, int wait_to_complete)
 {
-	/**
-	 * for a quick link status checking, wait_to_compelet == 0,
-	 * skip PF link status checking
-	 */
-	bool no_pflink_check = wait_to_complete == 0;
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	struct ixgbe_mac_info *mac = &hw->mac;
 	uint32_t links_reg, in_msg;

@@ -3945,14 +3940,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 		*speed = IXGBE_LINK_SPEED_UNKNOWN;
 	}
 
-	if (no_pflink_check) {
-		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
-			mac->get_link_status = true;
-		else
-			mac->get_link_status = false;
-
-		goto out;
-	}
 	/* if the read failed it could just be a mailbox collision, best wait
 	 * until we are called again and don't report an error
 	 */

@@ -3962,7 +3949,7 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 	if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
 		/* msg is not CTS and is NACK we must have lost CTS status */
 		if (in_msg & IXGBE_VT_MSGTYPE_NACK)
-			ret_val = -1;
+			mac->get_link_status = false;
 		goto out;
 	}

Signature · 1 line

2.11.0

Zhang, Qi Z

Acked-by on v2 1/1 Applied

2018-11-13 19:28 UTC #4 lore raw

Quoted 23 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Monday, November 12, 2018 10:35 PM
To: [email protected]
Cc: Zhang, Qi Z <[email protected]>; Xu, Rosen <[email protected]>; Wang, Dong1 <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH v2] net/ixgbe: fix ixgbevf link status
For ixgbevf kernel driver, link status changes from down to up will trigger vf kernel driver send IXGBE_VF_RESET message to pf kernel driver, after this, vf kernel driver will disable and enable it self. By these series operations, the vf kernel driver report link up. Besides, all these operations handles in kernel thread.
For DPDK user space driver, it only gets link status changes from down to up, but miss IXGBE_VF_RESET message sending and reset itself.
If we will add fully implementation of link status change for DPDK user space driver, we need take much more modification. We have aligned that for link status changes from down to up we only notify link is up, users need to reset vf port.

Quoted 4 lines

Fixes dc66e5fd01b9 ("net/ixgbe: improve link state check on VF")
Signed-off-by Rosen Xu <[email protected]>
Signed-off-by Yanglong Wu <[email protected]>

Acked-by Qi Zhang <[email protected]>

Applied to dpdk-next-net-intel.

Thanks
Qi

Quoted 51 lines

---
v2
change as comments

---
 drivers/net/ixgbe/ixgbe_ethdev.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

drivers/net/ixgbe/ixgbe_ethdev.c +0 ~~-13~~

index 8148577f5..91ba6201d 100644
@@ -3880,11 +3880,6 @@ static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 		   int *link_up, int wait_to_complete)  {
-	/**
-	 * for a quick link status checking, wait_to_compelet == 0,
-	 * skip PF link status checking
-	 */
-	bool no_pflink_check = wait_to_complete == 0;
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	struct ixgbe_mac_info *mac = &hw->mac;
 	uint32_t links_reg, in_msg;

@@ -3945,14 +3940,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw,
 ixgbe_link_speed *speed,
 		*speed = IXGBE_LINK_SPEED_UNKNOWN;
 	}
 
-	if (no_pflink_check) {
-		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
-			mac->get_link_status = true;
-		else
-			mac->get_link_status = false;
-
-		goto out;
-	}
 	/* if the read failed it could just be a mailbox collision, best wait
 	 * until we are called again and don't report an error

drivers/net/ixgbe/ixgbe_ethdev.c +1 -2

*/

@@ -3962,7 +3949,7 @@ ixgbevf_check_link(struct ixgbe_hw *hw,
 ixgbe_link_speed *speed,
 	if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
 		/* msg is not CTS and is NACK we must have lost CTS status */
 		if (in_msg & IXGBE_VT_MSGTYPE_NACK)
-			ret_val = -1;
+			mac->get_link_status = false;
 		goto out;
 	}
 
--

2.11.0

Wang, Dong1

Acked-by on v2 1/1

2018-11-14 05:14 UTC #5 lore raw

In reply to Yanglong Wu

-----Original Message-----
From: Wu, Yanglong
Sent: Tuesday, November 13, 2018 14:35
To: [email protected]
Cc: Zhang, Qi Z <[email protected]>; Xu, Rosen <[email protected]>; Wang, Dong1 <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH v2] net/ixgbe: fix ixgbevf link status

For ixgbevf kernel driver, link status changes from down to up will trigger vf kernel driver send IXGBE_VF_RESET message to pf kernel driver, after this, vf kernel driver will disable and enable it self. By these series operations, the vf kernel driver report link up. Besides, all these operations handles in kernel thread. For DPDK user space driver, it only gets link status changes from down to up, but miss IXGBE_VF_RESET message sending and reset itself.
If we will add fully implementation of link status change for DPDK user space driver, we need take much more modification. We have aligned that for link status changes from down to up we only notify link is up, users need to reset vf port.

Fixes dc66e5fd01b9 ("net/ixgbe: improve link state check on VF")

Signed-off-by Rosen Xu <[email protected]>

Signed-off-by Yanglong Wu <[email protected]>

---
v2
change as comments

---
 drivers/net/ixgbe/ixgbe_ethdev.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

drivers/net/ixgbe/ixgbe_ethdev.c +1 ~~-14~~

index 8148577f5..91ba6201d 100644
@@ -3880,11 +3880,6 @@ static int
 ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 		   int *link_up, int wait_to_complete)  {
-	/**
-	 * for a quick link status checking, wait_to_compelet == 0,
-	 * skip PF link status checking
-	 */
-	bool no_pflink_check = wait_to_complete == 0;
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	struct ixgbe_mac_info *mac = &hw->mac;
 	uint32_t links_reg, in_msg;

@@ -3945,14 +3940,6 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 		*speed = IXGBE_LINK_SPEED_UNKNOWN;
 	}
 
-	if (no_pflink_check) {
-		if (*speed == IXGBE_LINK_SPEED_UNKNOWN)
-			mac->get_link_status = true;
-		else
-			mac->get_link_status = false;
-
-		goto out;
-	}
 	/* if the read failed it could just be a mailbox collision, best wait
 	 * until we are called again and don't report an error
 	 */

@@ -3962,7 +3949,7 @@ ixgbevf_check_link(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 	if (!(in_msg & IXGBE_VT_MSGTYPE_CTS)) {
 		/* msg is not CTS and is NACK we must have lost CTS status */
 		if (in_msg & IXGBE_VT_MSGTYPE_NACK)
-			ret_val = -1;
+			mac->get_link_status = false;
 		goto out;
 	}

--
2.11.0

Acked-by Wang Dong <[email protected]>