[v2] net/e1000: fix link status update
Checks
Commit Message
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(-)
Comments
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
>
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
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
>
@@ -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;