[dpdk-stable] [PATCH] net/e1000: fix link status update

[dpdk-stable] [PATCH] net/e1000: fix link status update

[Cui LunyuanX]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/e1000: fix link status update

[Ye Xiaolong]

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
>

[dpdk-stable] [PATCH v2] net/e1000: fix link status update

[Lunyuan Cui]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/e1000: fix link status update

[Ye Xiaolong]

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
>

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/e1000: fix link status update

[Cui, LunyuanX]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/e1000: fix link status update

[Ye Xiaolong]

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
>

Re: [dpdk-stable] [dpdk-dev] [PATCH v3] net/e1000: fix link status update

[Ye Xiaolong]

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
>

[dpdk-stable] [PATCH v3] net/e1000: fix link status update

[Lunyuan Cui]

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

Re: [dpdk-stable] [PATCH v3] net/e1000: fix link status update

[Lu, Wenzhuo]

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

[dpdk-stable] [PATCH v4] net/e1000: fix link status update

[Lunyuan Cui]

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

Re: [dpdk-stable] [PATCH] net/e1000: fix link status update

[Lu, Wenzhuo]

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>

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] net/e1000: fix link status update

[Ye Xiaolong]

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.