diff mbox series

net/ixgbe: fix link status after port reset

Message ID 20200413013839.71374-1-shougangx.wang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: xiaolong ye
Headers
Series net/ixgbe: fix link status after port reset |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed

Commit Message

Shougang Wang April 13, 2020, 1:38 a.m. UTC 
  It's a normal behavior to change the link status to up after
resetting the port. So it is unnecessary to set link down before
starting port, and changing the link state(link up/down) frequently
will cause link speed unstable.

Fixes: c3f2fbff78cf ("net/ixgbe: fix link status")
Cc: stable@dpdk.org

Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Qiming Yang April 13, 2020, 2:18 a.m. UTC | #1 
> -----Original Message-----
> From: Wang, ShougangX <shougangx.wang@intel.com>
> Sent: Monday, April 13, 2020 09:39
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, ShougangX <shougangx.wang@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] net/ixgbe: fix link status after port reset
> 
> It's a normal behavior to change the link status to up after resetting the port.
> So it is unnecessary to set link down before starting port, and changing the
> link state(link up/down) frequently will cause link speed unstable.
> 
> Fixes: c3f2fbff78cf ("net/ixgbe: fix link status")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 23b3f5b0c..206358b85 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1196,7 +1196,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev,
> void *init_params __rte_unused)
>  	diag = ixgbe_bypass_init_hw(hw);
>  #else
>  	diag = ixgbe_init_hw(hw);
> -	hw->mac.autotry_restart = false;
>  #endif /* RTE_LIBRTE_IXGBE_BYPASS */
> 
>  	/*
> @@ -1307,8 +1306,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev,
> void *init_params __rte_unused)
>  	/* enable support intr */
>  	ixgbe_enable_intr(eth_dev);
> 
> -	ixgbe_dev_set_link_down(eth_dev);
> -
>  	/* initialize filter info */
>  	memset(filter_info, 0,
>  	       sizeof(struct ixgbe_filter_info));
> --
> 2.17.1

Acked-by: Qiming Yang <qiming.yang@intel.com>
Zhang, XuemingX April 14, 2020, 6:35 a.m. UTC | #2 
Tested-by: Zhang, XuemingX <xuemingx.zhang@intel.com>


>-----Original Message-----
>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shougang Wang
>Sent: Monday, April 13, 2020 9:39 AM
>To: dev@dpdk.org
>Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
><qiming.yang@intel.com>; Wang, ShougangX <shougangx.wang@intel.com>;
>stable@dpdk.org
>Subject: [dpdk-dev] [PATCH] net/ixgbe: fix link status after port reset
>
>It's a normal behavior to change the link status to up after resetting the
>port. So it is unnecessary to set link down before starting port, and
>changing the link state(link up/down) frequently will cause link speed
>unstable.
>
>Fixes: c3f2fbff78cf ("net/ixgbe: fix link status")
>Cc: stable@dpdk.org
>
>Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>b/drivers/net/ixgbe/ixgbe_ethdev.c
>index 23b3f5b0c..206358b85 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -1196,7 +1196,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void
>*init_params __rte_unused)
> 	diag = ixgbe_bypass_init_hw(hw);
> #else
> 	diag = ixgbe_init_hw(hw);
>-	hw->mac.autotry_restart = false;
> #endif /* RTE_LIBRTE_IXGBE_BYPASS */
>
> 	/*
>@@ -1307,8 +1306,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void
>*init_params __rte_unused)
> 	/* enable support intr */
> 	ixgbe_enable_intr(eth_dev);
>
>-	ixgbe_dev_set_link_down(eth_dev);
>-
> 	/* initialize filter info */
> 	memset(filter_info, 0,
> 	       sizeof(struct ixgbe_filter_info));
>--
>2.17.1
Xiaolong Ye April 14, 2020, 7:53 a.m. UTC | #3 
Hi, Shougang

On 04/13, Shougang Wang wrote:
>It's a normal behavior to change the link status to up after
>resetting the port. So it is unnecessary to set link down before
>starting port, and changing the link state(link up/down) frequently
>will cause link speed unstable.
>
>Fixes: c3f2fbff78cf ("net/ixgbe: fix link status")
>Cc: stable@dpdk.org
>
>Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>index 23b3f5b0c..206358b85 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -1196,7 +1196,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
> 	diag = ixgbe_bypass_init_hw(hw);
> #else
> 	diag = ixgbe_init_hw(hw);
>-	hw->mac.autotry_restart = false;

Why do we need this change? Seems it is not mentioned in the commit log.

> #endif /* RTE_LIBRTE_IXGBE_BYPASS */
> 
> 	/*
>@@ -1307,8 +1306,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
> 	/* enable support intr */
> 	ixgbe_enable_intr(eth_dev);
> 
>-	ixgbe_dev_set_link_down(eth_dev);
>-
> 	/* initialize filter info */
> 	memset(filter_info, 0,
> 	       sizeof(struct ixgbe_filter_info));
>-- 
>2.17.1
>
Shougang Wang April 21, 2020, 2:49 a.m. UTC | #4 
Hi, Xiaolong

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Tuesday, April 14, 2020 3:54 PM
> To: Wang, ShougangX <shougangx.wang@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix link status after port reset
> 
> Hi, Shougang
> 
> On 04/13, Shougang Wang wrote:
> >It's a normal behavior to change the link status to up after resetting
> >the port. So it is unnecessary to set link down before starting port,
> >and changing the link state(link up/down) frequently will cause link
> >speed unstable.
> >
> >Fixes: c3f2fbff78cf ("net/ixgbe: fix link status")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
> >---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> >diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >b/drivers/net/ixgbe/ixgbe_ethdev.c
> >index 23b3f5b0c..206358b85 100644
> >--- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >@@ -1196,7 +1196,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev,
> void *init_params __rte_unused)
> > 	diag = ixgbe_bypass_init_hw(hw);
> > #else
> > 	diag = ixgbe_init_hw(hw);
> >-	hw->mac.autotry_restart = false;
> 
> Why do we need this change? Seems it is not mentioned in the commit log.

In c3f2fbff78cf, port was set to down by following 2 steps.
1. Setting autotry_restart flag to false to prevent NIC from linking up by itself.
2. Calling ixgbe_dev_set_link_down().

Force to set link status to down is unnecessary operation before starting port, so I revert these 2 changes in this patch.

Thanks
Shougang
Xiaolong Ye April 22, 2020, 7:18 a.m. UTC | #5 
On 04/13, Shougang Wang wrote:
>It's a normal behavior to change the link status to up after
>resetting the port. So it is unnecessary to set link down before
>starting port, and changing the link state(link up/down) frequently
>will cause link speed unstable.
>
>Fixes: c3f2fbff78cf ("net/ixgbe: fix link status")
>Cc: stable@dpdk.org
>
>Signed-off-by: Shougang Wang <shougangx.wang@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>index 23b3f5b0c..206358b85 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -1196,7 +1196,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
> 	diag = ixgbe_bypass_init_hw(hw);
> #else
> 	diag = ixgbe_init_hw(hw);
>-	hw->mac.autotry_restart = false;
> #endif /* RTE_LIBRTE_IXGBE_BYPASS */
> 
> 	/*
>@@ -1307,8 +1306,6 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
> 	/* enable support intr */
> 	ixgbe_enable_intr(eth_dev);
> 
>-	ixgbe_dev_set_link_down(eth_dev);
>-
> 	/* initialize filter info */
> 	memset(filter_info, 0,
> 	       sizeof(struct ixgbe_filter_info));
>-- 
>2.17.1
>

Applied to dpdk-next-net-intel, Thanks.
diff mbox series

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 23b3f5b0c..206358b85 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1196,7 +1196,6 @@  eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 	diag = ixgbe_bypass_init_hw(hw);
 #else
 	diag = ixgbe_init_hw(hw);
-	hw->mac.autotry_restart = false;
 #endif /* RTE_LIBRTE_IXGBE_BYPASS */
 
 	/*
@@ -1307,8 +1306,6 @@  eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 	/* enable support intr */
 	ixgbe_enable_intr(eth_dev);
 
-	ixgbe_dev_set_link_down(eth_dev);
-
 	/* initialize filter info */
 	memset(filter_info, 0,
 	       sizeof(struct ixgbe_filter_info));