patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] net/e1000: fix link status update
@ 2019-11-13 17:32 Cui LunyuanX
  2019-11-14  6:41 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Cui LunyuanX @ 2019-11-13 17:32 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Yang Qiming, Cui LunyuanX, stable

Unassigned variable should not be used as judgment, and there
is no need to update link status according to old link status.
This patch fix the issue.

Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
Cc: stable@dpdk.org

Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
---
 drivers/net/e1000/em_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 9a88b50b2..a3d39a935 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1157,7 +1157,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 	memset(&link, 0, sizeof(link));
 
 	/* Now we check if a transition has happened */
-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
+	if (link_check) {
 		uint16_t duplex, speed;
 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
 		link.link_duplex = (duplex == FULL_DUPLEX) ?
-- 
2.17.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/e1000: fix link status update
  2019-11-13 17:32 [dpdk-stable] [PATCH] net/e1000: fix link status update Cui LunyuanX
@ 2019-11-14  6:41 ` Ye Xiaolong
  2019-11-15 14:48 ` [dpdk-stable] [PATCH v2] " Lunyuan Cui
  2019-11-26  2:04 ` [dpdk-stable] [PATCH] " Lu, Wenzhuo
  2 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-11-14  6:41 UTC (permalink / raw)
  To: Cui LunyuanX; +Cc: dev, Wenzhuo Lu, Yang Qiming, stable

Hi,

On 11/13, Cui LunyuanX wrote:
>Unassigned variable should not be used as judgment, and there

The issue here is link structure variable has been memset first, which makes it
meaningless to compare the value of link.link_status in the conditions.

>is no need to update link status according to old link status.
>This patch fix the issue.
>
>Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
>Cc: stable@dpdk.org
>
>Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
>---
> drivers/net/e1000/em_ethdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
>index 9a88b50b2..a3d39a935 100644
>--- a/drivers/net/e1000/em_ethdev.c
>+++ b/drivers/net/e1000/em_ethdev.c
>@@ -1157,7 +1157,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 	memset(&link, 0, sizeof(link));
> 
> 	/* Now we check if a transition has happened */
>-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
>+	if (link_check) {
> 		uint16_t duplex, speed;
> 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
> 		link.link_duplex = (duplex == FULL_DUPLEX) ?

Seems you also need to fix the `else if` judgment.

Thanks,
Xiaolong

>-- 
>2.17.1
>

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

* [dpdk-stable] [PATCH v2] net/e1000: fix link status update
  2019-11-13 17:32 [dpdk-stable] [PATCH] net/e1000: fix link status update Cui LunyuanX
  2019-11-14  6:41 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-11-15 14:48 ` Lunyuan Cui
  2019-11-18  3:06   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-11-18 14:58   ` [dpdk-stable] [PATCH v3] " Lunyuan Cui
  2019-11-26  2:04 ` [dpdk-stable] [PATCH] " Lu, Wenzhuo
  2 siblings, 2 replies; 12+ messages in thread
From: Lunyuan Cui @ 2019-11-15 14:48 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Lunyuan Cui, stable

Unassigned variable should not be used as judgment, and there
is no need to update link status according to old link status.
This patch fix the issue.

Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
Cc: stable@dpdk.org

Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
---
 drivers/net/e1000/em_ethdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 9a88b50b2..7959ee4e9 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1157,7 +1157,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 	memset(&link, 0, sizeof(link));
 
 	/* Now we check if a transition has happened */
-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
+	if (link_check) {
 		uint16_t duplex, speed;
 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
 		link.link_duplex = (duplex == FULL_DUPLEX) ?
@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_UP;
 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
 				ETH_LINK_SPEED_FIXED);
-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
+	} else {
 		link.link_speed = ETH_SPEED_NUM_NONE;
 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
 		link.link_status = ETH_LINK_DOWN;
-- 
2.17.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/e1000: fix link status update
  2019-11-15 14:48 ` [dpdk-stable] [PATCH v2] " Lunyuan Cui
@ 2019-11-18  3:06   ` Ye Xiaolong
  2019-11-18  3:21     ` Cui, LunyuanX
  2019-11-18 14:58   ` [dpdk-stable] [PATCH v3] " Lunyuan Cui
  1 sibling, 1 reply; 12+ messages in thread
From: Ye Xiaolong @ 2019-11-18  3:06 UTC (permalink / raw)
  To: Lunyuan Cui; +Cc: dev, Wenzhuo Lu, stable

On 11/15, Lunyuan Cui wrote:
>Unassigned variable should not be used as judgment, and there
>is no need to update link status according to old link status.
>This patch fix the issue.
>
>Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
>Cc: stable@dpdk.org
>
>Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
>---
> drivers/net/e1000/em_ethdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
>index 9a88b50b2..7959ee4e9 100644
>--- a/drivers/net/e1000/em_ethdev.c
>+++ b/drivers/net/e1000/em_ethdev.c
>@@ -1157,7 +1157,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 	memset(&link, 0, sizeof(link));
> 
> 	/* Now we check if a transition has happened */
>-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
>+	if (link_check) {
> 		uint16_t duplex, speed;
> 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
> 		link.link_duplex = (duplex == FULL_DUPLEX) ?
>@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 		link.link_status = ETH_LINK_UP;
> 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> 				ETH_LINK_SPEED_FIXED);
>-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
>+	} else {
> 		link.link_speed = ETH_SPEED_NUM_NONE;
> 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
> 		link.link_status = ETH_LINK_DOWN;

I am a little confused about the variable link_check, is it used to indicate
whether there is link status change or link status up?

Thanks,
Xiaolong

>-- 
>2.17.1
>

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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/e1000: fix link status update
  2019-11-18  3:06   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-11-18  3:21     ` Cui, LunyuanX
  2019-11-18  3:28       ` Ye Xiaolong
  0 siblings, 1 reply; 12+ messages in thread
From: Cui, LunyuanX @ 2019-11-18  3:21 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: dev, Lu, Wenzhuo, stable

Hi, Xiaolong

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Monday, November 18, 2019 11:07 AM
> To: Cui, LunyuanX <lunyuanx.cui@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/e1000: fix link status update
> 
> On 11/15, Lunyuan Cui wrote:
> >Unassigned variable should not be used as judgment, and there is no
> >need to update link status according to old link status.
> >This patch fix the issue.
> >
> >Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
> >---
> > drivers/net/e1000/em_ethdev.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/net/e1000/em_ethdev.c
> >b/drivers/net/e1000/em_ethdev.c index 9a88b50b2..7959ee4e9 100644
> >--- a/drivers/net/e1000/em_ethdev.c
> >+++ b/drivers/net/e1000/em_ethdev.c
> >@@ -1157,7 +1157,7 @@ eth_em_link_update(struct rte_eth_dev *dev,
> int wait_to_complete)
> > 	memset(&link, 0, sizeof(link));
> >
> > 	/* Now we check if a transition has happened */
> >-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
> >+	if (link_check) {
> > 		uint16_t duplex, speed;
> > 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
> > 		link.link_duplex = (duplex == FULL_DUPLEX) ?
> >@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev,
> int wait_to_complete)
> > 		link.link_status = ETH_LINK_UP;
> > 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> > 				ETH_LINK_SPEED_FIXED);
> >-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
> >+	} else {
> > 		link.link_speed = ETH_SPEED_NUM_NONE;
> > 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
> > 		link.link_status = ETH_LINK_DOWN;
> 
> I am a little confused about the variable link_check, is it used to indicate
> whether there is link status change or link status up?

The variable link_check is used to indicate link status up.
When link_check is true, link status is up.
When link_check is false, link status is down.

Thanks,
Lunyuan


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/e1000: fix link status update
  2019-11-18  3:21     ` Cui, LunyuanX
@ 2019-11-18  3:28       ` Ye Xiaolong
  0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-11-18  3:28 UTC (permalink / raw)
  To: Cui, LunyuanX; +Cc: dev, Lu, Wenzhuo, stable

On 11/18, Cui, LunyuanX wrote:
>Hi, Xiaolong
>
>> -----Original Message-----
>> From: Ye, Xiaolong
>> Sent: Monday, November 18, 2019 11:07 AM
>> To: Cui, LunyuanX <lunyuanx.cui@intel.com>
>> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] net/e1000: fix link status update
>> 
>> On 11/15, Lunyuan Cui wrote:
>> >Unassigned variable should not be used as judgment, and there is no
>> >need to update link status according to old link status.
>> >This patch fix the issue.
>> >
>> >Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
>> >Cc: stable@dpdk.org
>> >
>> >Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
>> >---
>> > drivers/net/e1000/em_ethdev.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/net/e1000/em_ethdev.c
>> >b/drivers/net/e1000/em_ethdev.c index 9a88b50b2..7959ee4e9 100644
>> >--- a/drivers/net/e1000/em_ethdev.c
>> >+++ b/drivers/net/e1000/em_ethdev.c
>> >@@ -1157,7 +1157,7 @@ eth_em_link_update(struct rte_eth_dev *dev,
>> int wait_to_complete)
>> > 	memset(&link, 0, sizeof(link));
>> >
>> > 	/* Now we check if a transition has happened */
>> >-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
>> >+	if (link_check) {
>> > 		uint16_t duplex, speed;
>> > 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
>> > 		link.link_duplex = (duplex == FULL_DUPLEX) ?
>> >@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev,
>> int wait_to_complete)
>> > 		link.link_status = ETH_LINK_UP;
>> > 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
>> > 				ETH_LINK_SPEED_FIXED);
>> >-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
>> >+	} else {
>> > 		link.link_speed = ETH_SPEED_NUM_NONE;
>> > 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
>> > 		link.link_status = ETH_LINK_DOWN;
>> 
>> I am a little confused about the variable link_check, is it used to indicate
>> whether there is link status change or link status up?
>
>The variable link_check is used to indicate link status up.
>When link_check is true, link status is up.
>When link_check is false, link status is down.

Then the link_check seems a bad name, could you help submit a patch to rename it?

@whenzhuo, could you help review/ack this change?

Thanks,
Xiaolong
>
>Thanks,
>Lunyuan
>

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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v3] net/e1000: fix link status update
  2019-11-18 14:58   ` [dpdk-stable] [PATCH v3] " Lunyuan Cui
@ 2019-11-18  7:23     ` Ye Xiaolong
  2019-11-19  6:43     ` [dpdk-stable] " Lu, Wenzhuo
  2019-11-20  9:22     ` [dpdk-stable] [PATCH v4] " Lunyuan Cui
  2 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-11-18  7:23 UTC (permalink / raw)
  To: Lunyuan Cui; +Cc: dev, Wenzhuo Lu, stable

Hi, wenzhuo

Could you help ack this patch if you are ok with this change?

Thanks,
Xiaolong

On 11/18, Lunyuan Cui wrote:
>Unassigned variable should not be used as judgment, and there
>is no need to update link status according to old link status.
>This patch fix the issue.
>
>Change the variable from link_check to link_up.
>
>Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
>Cc: stable@dpdk.org
>
>Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
>---
>v3:
>* Change the variable from link_check to link_up.
>
>v2
>* Delete incorrect judgment
>---
> drivers/net/e1000/em_ethdev.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
>index 9a88b50b2..080cbe2df 100644
>--- a/drivers/net/e1000/em_ethdev.c
>+++ b/drivers/net/e1000/em_ethdev.c
>@@ -1121,9 +1121,9 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 	struct e1000_hw *hw =
> 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> 	struct rte_eth_link link;
>-	int link_check, count;
>+	int link_up, count;
> 
>-	link_check = 0;
>+	link_up = 0;
> 	hw->mac.get_link_status = 1;
> 
> 	/* possible wait-to-complete in up to 9 seconds */
>@@ -1133,31 +1133,31 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 		case e1000_media_type_copper:
> 			/* Do the work to read phy */
> 			e1000_check_for_link(hw);
>-			link_check = !hw->mac.get_link_status;
>+			link_up = !hw->mac.get_link_status;
> 			break;
> 
> 		case e1000_media_type_fiber:
> 			e1000_check_for_link(hw);
>-			link_check = (E1000_READ_REG(hw, E1000_STATUS) &
>+			link_up = (E1000_READ_REG(hw, E1000_STATUS) &
> 					E1000_STATUS_LU);
> 			break;
> 
> 		case e1000_media_type_internal_serdes:
> 			e1000_check_for_link(hw);
>-			link_check = hw->mac.serdes_has_link;
>+			link_up = hw->mac.serdes_has_link;
> 			break;
> 
> 		default:
> 			break;
> 		}
>-		if (link_check || wait_to_complete == 0)
>+		if (link_up || wait_to_complete == 0)
> 			break;
> 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
> 	}
> 	memset(&link, 0, sizeof(link));
> 
> 	/* Now we check if a transition has happened */
>-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
>+	if (link_up) {
> 		uint16_t duplex, speed;
> 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
> 		link.link_duplex = (duplex == FULL_DUPLEX) ?
>@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 		link.link_status = ETH_LINK_UP;
> 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> 				ETH_LINK_SPEED_FIXED);
>-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
>+	} else {
> 		link.link_speed = ETH_SPEED_NUM_NONE;
> 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
> 		link.link_status = ETH_LINK_DOWN;
>-- 
>2.17.1
>

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

* [dpdk-stable] [PATCH v3] net/e1000: fix link status update
  2019-11-15 14:48 ` [dpdk-stable] [PATCH v2] " Lunyuan Cui
  2019-11-18  3:06   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-11-18 14:58   ` Lunyuan Cui
  2019-11-18  7:23     ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
                       ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Lunyuan Cui @ 2019-11-18 14:58 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Lunyuan Cui, stable

Unassigned variable should not be used as judgment, and there
is no need to update link status according to old link status.
This patch fix the issue.

Change the variable from link_check to link_up.

Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
Cc: stable@dpdk.org

Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
---
v3:
* Change the variable from link_check to link_up.

v2
* Delete incorrect judgment
---
 drivers/net/e1000/em_ethdev.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 9a88b50b2..080cbe2df 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1121,9 +1121,9 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct rte_eth_link link;
-	int link_check, count;
+	int link_up, count;
 
-	link_check = 0;
+	link_up = 0;
 	hw->mac.get_link_status = 1;
 
 	/* possible wait-to-complete in up to 9 seconds */
@@ -1133,31 +1133,31 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		case e1000_media_type_copper:
 			/* Do the work to read phy */
 			e1000_check_for_link(hw);
-			link_check = !hw->mac.get_link_status;
+			link_up = !hw->mac.get_link_status;
 			break;
 
 		case e1000_media_type_fiber:
 			e1000_check_for_link(hw);
-			link_check = (E1000_READ_REG(hw, E1000_STATUS) &
+			link_up = (E1000_READ_REG(hw, E1000_STATUS) &
 					E1000_STATUS_LU);
 			break;
 
 		case e1000_media_type_internal_serdes:
 			e1000_check_for_link(hw);
-			link_check = hw->mac.serdes_has_link;
+			link_up = hw->mac.serdes_has_link;
 			break;
 
 		default:
 			break;
 		}
-		if (link_check || wait_to_complete == 0)
+		if (link_up || wait_to_complete == 0)
 			break;
 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
 
 	/* Now we check if a transition has happened */
-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
+	if (link_up) {
 		uint16_t duplex, speed;
 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
 		link.link_duplex = (duplex == FULL_DUPLEX) ?
@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_UP;
 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
 				ETH_LINK_SPEED_FIXED);
-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
+	} else {
 		link.link_speed = ETH_SPEED_NUM_NONE;
 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
 		link.link_status = ETH_LINK_DOWN;
-- 
2.17.1


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

* Re: [dpdk-stable] [PATCH v3] net/e1000: fix link status update
  2019-11-18 14:58   ` [dpdk-stable] [PATCH v3] " Lunyuan Cui
  2019-11-18  7:23     ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-11-19  6:43     ` Lu, Wenzhuo
  2019-11-20  9:22     ` [dpdk-stable] [PATCH v4] " Lunyuan Cui
  2 siblings, 0 replies; 12+ messages in thread
From: Lu, Wenzhuo @ 2019-11-19  6:43 UTC (permalink / raw)
  To: Cui, LunyuanX, dev; +Cc: stable

Hi Lunyuan,


> -----Original Message-----
> From: Cui, LunyuanX
> Sent: Monday, November 18, 2019 10:58 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Cui, LunyuanX
> <lunyuanx.cui@intel.com>; stable@dpdk.org
> Subject: [PATCH v3] net/e1000: fix link status update
> 
> Unassigned variable should not be used as judgment, and there is no need
Don't understand "Unassigned variable should not be used as judgment ". Which one is this " Unassigned variable"?
> to update link status according to old link status.
Don't understand why " no need to update link status according to old link status ".
Please add more info about what problem this patch wants to fix. Thanks.

> This patch fix the issue.
> 
> Change the variable from link_check to link_up.
> 
> Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
> ---
> v3:
> * Change the variable from link_check to link_up.
> 
> v2
> * Delete incorrect judgment
> ---
>  drivers/net/e1000/em_ethdev.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/e1000/em_ethdev.c
> b/drivers/net/e1000/em_ethdev.c index 9a88b50b2..080cbe2df 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c


> 
>  	/* Now we check if a transition has happened */
Looks like this comment need to be removed if the below check changes. But still, don't know why we should change the check.
> -	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
> +	if (link_up) {


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

* [dpdk-stable] [PATCH v4] net/e1000: fix link status update
  2019-11-18 14:58   ` [dpdk-stable] [PATCH v3] " Lunyuan Cui
  2019-11-18  7:23     ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-11-19  6:43     ` [dpdk-stable] " Lu, Wenzhuo
@ 2019-11-20  9:22     ` Lunyuan Cui
  2019-11-26  2:24       ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2 siblings, 1 reply; 12+ messages in thread
From: Lunyuan Cui @ 2019-11-20  9:22 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Lunyuan Cui, stable

There is no need to judge the original link state,
only update the data according to the current link state.
It can maintain better robustness.

In addition, this patch change the variable
from link_check to link_up.

Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
Cc: stable@dpdk.org

Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
---
v4:
* modifier commit log

v3:
* Change the variable from link_check to link_up.

v2
* Delete incorrect judgment
---
 drivers/net/e1000/em_ethdev.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 9a88b50b2..080cbe2df 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1121,9 +1121,9 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct rte_eth_link link;
-	int link_check, count;
+	int link_up, count;
 
-	link_check = 0;
+	link_up = 0;
 	hw->mac.get_link_status = 1;
 
 	/* possible wait-to-complete in up to 9 seconds */
@@ -1133,31 +1133,31 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		case e1000_media_type_copper:
 			/* Do the work to read phy */
 			e1000_check_for_link(hw);
-			link_check = !hw->mac.get_link_status;
+			link_up = !hw->mac.get_link_status;
 			break;
 
 		case e1000_media_type_fiber:
 			e1000_check_for_link(hw);
-			link_check = (E1000_READ_REG(hw, E1000_STATUS) &
+			link_up = (E1000_READ_REG(hw, E1000_STATUS) &
 					E1000_STATUS_LU);
 			break;
 
 		case e1000_media_type_internal_serdes:
 			e1000_check_for_link(hw);
-			link_check = hw->mac.serdes_has_link;
+			link_up = hw->mac.serdes_has_link;
 			break;
 
 		default:
 			break;
 		}
-		if (link_check || wait_to_complete == 0)
+		if (link_up || wait_to_complete == 0)
 			break;
 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
 
 	/* Now we check if a transition has happened */
-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
+	if (link_up) {
 		uint16_t duplex, speed;
 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
 		link.link_duplex = (duplex == FULL_DUPLEX) ?
@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_UP;
 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
 				ETH_LINK_SPEED_FIXED);
-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
+	} else {
 		link.link_speed = ETH_SPEED_NUM_NONE;
 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
 		link.link_status = ETH_LINK_DOWN;
-- 
2.17.1


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

* Re: [dpdk-stable] [PATCH] net/e1000: fix link status update
  2019-11-13 17:32 [dpdk-stable] [PATCH] net/e1000: fix link status update Cui LunyuanX
  2019-11-14  6:41 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-11-15 14:48 ` [dpdk-stable] [PATCH v2] " Lunyuan Cui
@ 2019-11-26  2:04 ` Lu, Wenzhuo
  2 siblings, 0 replies; 12+ messages in thread
From: Lu, Wenzhuo @ 2019-11-26  2:04 UTC (permalink / raw)
  To: Cui, LunyuanX, dev; +Cc: Yang, Qiming, stable

Hi,

> -----Original Message-----
> From: Cui, LunyuanX
> Sent: Thursday, November 14, 2019 1:33 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Cui, LunyuanX <lunyuanx.cui@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] net/e1000: fix link status update
> 
> Unassigned variable should not be used as judgment, and there is no need
> to update link status according to old link status.
> This patch fix the issue.
> 
> Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

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

* Re: [dpdk-stable] [dpdk-dev] [PATCH v4] net/e1000: fix link status update
  2019-11-20  9:22     ` [dpdk-stable] [PATCH v4] " Lunyuan Cui
@ 2019-11-26  2:24       ` Ye Xiaolong
  0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-11-26  2:24 UTC (permalink / raw)
  To: Lunyuan Cui; +Cc: dev, Wenzhuo Lu, stable

On 11/20, Lunyuan Cui wrote:
>There is no need to judge the original link state,
>only update the data according to the current link state.
>It can maintain better robustness.
>
>In addition, this patch change the variable
>from link_check to link_up.
>
>Fixes: 80ba61115e77 ("net/e1000: use link status helper functions")
>Cc: stable@dpdk.org
>
>Signed-off-by: Lunyuan Cui <lunyuanx.cui@intel.com>
>---
>v4:
>* modifier commit log
>
>v3:
>* Change the variable from link_check to link_up.
>
>v2
>* Delete incorrect judgment
>---
> drivers/net/e1000/em_ethdev.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
>index 9a88b50b2..080cbe2df 100644
>--- a/drivers/net/e1000/em_ethdev.c
>+++ b/drivers/net/e1000/em_ethdev.c
>@@ -1121,9 +1121,9 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 	struct e1000_hw *hw =
> 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> 	struct rte_eth_link link;
>-	int link_check, count;
>+	int link_up, count;
> 
>-	link_check = 0;
>+	link_up = 0;
> 	hw->mac.get_link_status = 1;
> 
> 	/* possible wait-to-complete in up to 9 seconds */
>@@ -1133,31 +1133,31 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 		case e1000_media_type_copper:
> 			/* Do the work to read phy */
> 			e1000_check_for_link(hw);
>-			link_check = !hw->mac.get_link_status;
>+			link_up = !hw->mac.get_link_status;
> 			break;
> 
> 		case e1000_media_type_fiber:
> 			e1000_check_for_link(hw);
>-			link_check = (E1000_READ_REG(hw, E1000_STATUS) &
>+			link_up = (E1000_READ_REG(hw, E1000_STATUS) &
> 					E1000_STATUS_LU);
> 			break;
> 
> 		case e1000_media_type_internal_serdes:
> 			e1000_check_for_link(hw);
>-			link_check = hw->mac.serdes_has_link;
>+			link_up = hw->mac.serdes_has_link;
> 			break;
> 
> 		default:
> 			break;
> 		}
>-		if (link_check || wait_to_complete == 0)
>+		if (link_up || wait_to_complete == 0)
> 			break;
> 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
> 	}
> 	memset(&link, 0, sizeof(link));
> 
> 	/* Now we check if a transition has happened */
>-	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
>+	if (link_up) {
> 		uint16_t duplex, speed;
> 		hw->mac.ops.get_link_up_info(hw, &speed, &duplex);
> 		link.link_duplex = (duplex == FULL_DUPLEX) ?
>@@ -1167,7 +1167,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> 		link.link_status = ETH_LINK_UP;
> 		link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> 				ETH_LINK_SPEED_FIXED);
>-	} else if (!link_check && (link.link_status == ETH_LINK_UP)) {
>+	} else {
> 		link.link_speed = ETH_SPEED_NUM_NONE;
> 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
> 		link.link_status = ETH_LINK_DOWN;
>-- 
>2.17.1
>

Acked-by: Xiaolong Ye <xiaolong.ye@intel.com>

Applied to dpdk-next-net-intel, Thanks.

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

end of thread, other threads:[~2019-11-26  2:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13 17:32 [dpdk-stable] [PATCH] net/e1000: fix link status update Cui LunyuanX
2019-11-14  6:41 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-11-15 14:48 ` [dpdk-stable] [PATCH v2] " Lunyuan Cui
2019-11-18  3:06   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-11-18  3:21     ` Cui, LunyuanX
2019-11-18  3:28       ` Ye Xiaolong
2019-11-18 14:58   ` [dpdk-stable] [PATCH v3] " Lunyuan Cui
2019-11-18  7:23     ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-11-19  6:43     ` [dpdk-stable] " Lu, Wenzhuo
2019-11-20  9:22     ` [dpdk-stable] [PATCH v4] " Lunyuan Cui
2019-11-26  2:24       ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-11-26  2:04 ` [dpdk-stable] [PATCH] " Lu, Wenzhuo

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