* [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated
@ 2016-11-17 17:29 Laurent Hardy
2016-11-28 17:27 ` Laurent Hardy
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Laurent Hardy @ 2016-11-17 17:29 UTC (permalink / raw)
To: helin.zhang, konstantin.ananyev; +Cc: dev
In case of link speed set to 1Gb at peer side (with autoneg or with
defined speed) and cable not plugged-in when device is configured
and started, then link status is not updated properly with new speed
as no link setup is triggered.
To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link
setup each time link_update() is triggered and current link status is
down. When cable is plugged-in, link setup will be performed via
ixgbe_setup_link().
Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++++++++++++
drivers/net/ixgbe/ixgbe_ethdev.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 52ebbe4..513d1d5 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
/* set flag to update link status after init */
intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
+ intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
/*
* Initialize to TRUE. If any of Rx queues doesn't meet the bulk
@@ -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
struct rte_eth_link link, old;
ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
+ struct ixgbe_interrupt *intr =
+ IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
int link_up;
int diag;
+ u32 speed = 0;
+ bool autoneg = false;
link.link_status = ETH_LINK_DOWN;
link.link_speed = 0;
@@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
hw->mac.get_link_status = true;
+ if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) {
+ speed = hw->phy.autoneg_advertised;
+ if (!speed) {
+ ixgbe_get_link_capabilities(hw, &speed, &autoneg);
+ /* setup the highest link when no autoneg */
+ if (!autoneg) {
+ if (speed & IXGBE_LINK_SPEED_10GB_FULL)
+ speed = IXGBE_LINK_SPEED_10GB_FULL;
+ }
+ }
+ ixgbe_setup_link(hw, speed, true);
+ }
+
/* check if it needs to wait to complete, if lsc interrupt is enabled */
if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0)
diag = ixgbe_check_link(hw, &link_speed, &link_up, 0);
@@ -3145,10 +3163,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
if (link_up == 0) {
rte_ixgbe_dev_atomic_write_link_status(dev, &link);
+ intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
if (link.link_status == old.link_status)
return -1;
return 0;
}
+ intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
link.link_status = ETH_LINK_UP;
link.link_duplex = ETH_LINK_FULL_DUPLEX;
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index e060c3d..9d335ba 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -43,6 +43,7 @@
#define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0)
#define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1)
#define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2)
+#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3)
/*
* Defines that were not part of ixgbe_type.h as they are not used by the
--
1.7.10.4
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2016-11-17 17:29 [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated Laurent Hardy @ 2016-11-28 17:27 ` Laurent Hardy 2016-12-08 17:10 ` Ferruh Yigit 2017-02-08 15:51 ` Dai, Wei 2017-02-16 16:32 ` [dpdk-dev] [PATCH v2] " Olivier Matz 2 siblings, 1 reply; 25+ messages in thread From: Laurent Hardy @ 2016-11-28 17:27 UTC (permalink / raw) To: helin.zhang, konstantin.ananyev; +Cc: dev Hello, Is there anyone available to review this patch ? regards, Laurent On 11/17/2016 06:29 PM, Laurent Hardy wrote: > In case of link speed set to 1Gb at peer side (with autoneg or with > defined speed) and cable not plugged-in when device is configured > and started, then link status is not updated properly with new speed > as no link setup is triggered. > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link > setup each time link_update() is triggered and current link status is > down. When cable is plugged-in, link setup will be performed via > ixgbe_setup_link(). > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++++++++++++ > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c > index 52ebbe4..513d1d5 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > /* set flag to update link status after init */ > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > /* > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > @@ -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr = > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed = 0; > + bool autoneg = false; > > link.link_status = ETH_LINK_DOWN; > link.link_speed = 0; > @@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > hw->mac.get_link_status = true; > > + if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) { > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + /* setup the highest link when no autoneg */ > + if (!autoneg) { > + if (speed & IXGBE_LINK_SPEED_10GB_FULL) > + speed = IXGBE_LINK_SPEED_10GB_FULL; > + } > + } > + ixgbe_setup_link(hw, speed, true); > + } > + > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); > @@ -3145,10 +3163,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > if (link_up == 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status == old.link_status) > return -1; > return 0; > } > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status = ETH_LINK_UP; > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h > index e060c3d..9d335ba 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -43,6 +43,7 @@ > #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0) > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3) > > /* > * Defines that were not part of ixgbe_type.h as they are not used by the ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2016-11-28 17:27 ` Laurent Hardy @ 2016-12-08 17:10 ` Ferruh Yigit 2017-02-01 14:14 ` Thomas Monjalon 0 siblings, 1 reply; 25+ messages in thread From: Ferruh Yigit @ 2016-12-08 17:10 UTC (permalink / raw) To: Laurent Hardy, helin.zhang, konstantin.ananyev Cc: dev, Wenzhuo Lu, Zhang Qi, Xiao Wang On 11/28/2016 5:27 PM, Laurent Hardy wrote: > Hello, > Is there anyone available to review this patch ? Adding more developers into cc. > > regards, > Laurent > <...> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2016-12-08 17:10 ` Ferruh Yigit @ 2017-02-01 14:14 ` Thomas Monjalon 0 siblings, 0 replies; 25+ messages in thread From: Thomas Monjalon @ 2017-02-01 14:14 UTC (permalink / raw) To: helin.zhang, konstantin.ananyev Cc: dev, Ferruh Yigit, Laurent Hardy, Wenzhuo Lu 2016-12-08 17:10, Ferruh Yigit: > On 11/28/2016 5:27 PM, Laurent Hardy wrote: > > Hello, > > Is there anyone available to review this patch ? > > Adding more developers into cc. This patch is dying: more than 2 months without any review! A tip to help ixgbe maintainers in their task, please check this regularly: http://dpdk.org/dev/patchwork/project/dpdk/list/?state=&q=ixgbe ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2016-11-17 17:29 [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated Laurent Hardy 2016-11-28 17:27 ` Laurent Hardy @ 2017-02-08 15:51 ` Dai, Wei 2017-02-08 17:03 ` Olivier MATZ 2017-02-16 16:32 ` [dpdk-dev] [PATCH v2] " Olivier Matz 2 siblings, 1 reply; 25+ messages in thread From: Dai, Wei @ 2017-02-08 15:51 UTC (permalink / raw) To: Laurent Hardy, Zhang, Helin, Ananyev, Konstantin; +Cc: dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Laurent Hardy > Sent: Friday, November 18, 2016 1:30 AM > To: Zhang, Helin <helin.zhang@intel.com>; Ananyev, Konstantin > <konstantin.ananyev@intel.com> > Cc: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated > > In case of link speed set to 1Gb at peer side (with autoneg or with defined > speed) and cable not plugged-in when device is configured and started, then > link status is not updated properly with new speed as no link setup is triggered. > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link setup > each time link_update() is triggered and current link status is down. When > cable is plugged-in, link setup will be performed via ixgbe_setup_link(). > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++++++++++++ > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > b/drivers/net/ixgbe/ixgbe_ethdev.c > index 52ebbe4..513d1d5 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > /* set flag to update link status after init */ > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > /* > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk @@ > -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int > wait_to_complete) > struct ixgbe_hw *hw = > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr = > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed = 0; > + bool autoneg = false; > > link.link_status = ETH_LINK_DOWN; > link.link_speed = 0; > @@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) > > hw->mac.get_link_status = true; > > + if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) { > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + /* setup the highest link when no autoneg */ > + if (!autoneg) { > + if (speed & IXGBE_LINK_SPEED_10GB_FULL) > + speed = IXGBE_LINK_SPEED_10GB_FULL; > + } > + } > + ixgbe_setup_link(hw, speed, true); > + } > + > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3145,10 > +3163,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int > wait_to_complete) > > if (link_up == 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status == old.link_status) > return -1; > return 0; > } > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status = ETH_LINK_UP; > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > b/drivers/net/ixgbe/ixgbe_ethdev.h > index e060c3d..9d335ba 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -43,6 +43,7 @@ > #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0) > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3) Now there is following macro in DPDK 17.02-rc2. #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) You can redefine it as #define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > /* > * Defines that were not part of ixgbe_type.h as they are not used by the > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2017-02-08 15:51 ` Dai, Wei @ 2017-02-08 17:03 ` Olivier MATZ 2017-02-13 3:17 ` Dai, Wei 0 siblings, 1 reply; 25+ messages in thread From: Olivier MATZ @ 2017-02-08 17:03 UTC (permalink / raw) To: Dai, Wei; +Cc: Laurent Hardy, Zhang, Helin, Ananyev, Konstantin, dev Hi Wei, On Wed, 8 Feb 2017 15:51:42 +0000, "Dai, Wei" <wei.dai@intel.com> wrote: > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Laurent Hardy > > Sent: Friday, November 18, 2016 1:30 AM > > To: Zhang, Helin <helin.zhang@intel.com>; Ananyev, Konstantin > > <konstantin.ananyev@intel.com> > > Cc: dev@dpdk.org > > Subject: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated > > > > In case of link speed set to 1Gb at peer side (with autoneg or with > > defined speed) and cable not plugged-in when device is configured > > and started, then link status is not updated properly with new > > speed as no link setup is triggered. > > > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a > > link setup each time link_update() is triggered and current link > > status is down. When cable is plugged-in, link setup will be > > performed via ixgbe_setup_link(). > > > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > > --- > > drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++++++++++++ > > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > index 52ebbe4..513d1d5 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > @@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > > > /* set flag to update link status after init */ > > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > > > /* > > * Initialize to TRUE. If any of Rx queues doesn't meet > > the bulk @@ -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct > > rte_eth_dev *dev, int wait_to_complete) > > struct ixgbe_hw *hw = > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > struct rte_eth_link link, old; > > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > > + struct ixgbe_interrupt *intr = > > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > > int link_up; > > int diag; > > + u32 speed = 0; > > + bool autoneg = false; > > > > link.link_status = ETH_LINK_DOWN; > > link.link_speed = 0; > > @@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev > > *dev, int wait_to_complete) > > > > hw->mac.get_link_status = true; > > > > + if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) { > > + speed = hw->phy.autoneg_advertised; > > + if (!speed) { > > + ixgbe_get_link_capabilities(hw, &speed, > > &autoneg); > > + /* setup the highest link when no autoneg > > */ > > + if (!autoneg) { > > + if (speed & > > IXGBE_LINK_SPEED_10GB_FULL) > > + speed = > > IXGBE_LINK_SPEED_10GB_FULL; > > + } > > + } > > + ixgbe_setup_link(hw, speed, true); > > + } > > + > > /* check if it needs to wait to complete, if lsc interrupt > > is enabled */ if (wait_to_complete == 0 || > > dev->data->dev_conf.intr_conf.lsc != 0) diag = ixgbe_check_link(hw, > > &link_speed, &link_up, 0); @@ -3145,10 +3163,12 @@ > > ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > > > if (link_up == 0) { > > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > if (link.link_status == old.link_status) > > return -1; > > return 0; > > } > > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > > link.link_status = ETH_LINK_UP; > > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > > b/drivers/net/ixgbe/ixgbe_ethdev.h > > index e060c3d..9d335ba 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > > @@ -43,6 +43,7 @@ > > #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0) > > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3) > > Now there is following macro in DPDK 17.02-rc2. > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > You can redefine it as #define IXGBE_FLAG_NEED_LINK_CONFIG > (uint32_t)(1 << 4) Thanks, I'll send a v2. Do you agree with the rest of the patch? Regards, Olivier ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2017-02-08 17:03 ` Olivier MATZ @ 2017-02-13 3:17 ` Dai, Wei 0 siblings, 0 replies; 25+ messages in thread From: Dai, Wei @ 2017-02-13 3:17 UTC (permalink / raw) To: Olivier MATZ; +Cc: Laurent Hardy, Zhang, Helin, Ananyev, Konstantin, dev > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Thursday, February 9, 2017 1:03 AM > To: Dai, Wei <wei.dai@intel.com> > Cc: Laurent Hardy <laurent.hardy@6wind.com>; Zhang, Helin > <helin.zhang@intel.com>; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated > > Hi Wei, > > On Wed, 8 Feb 2017 15:51:42 +0000, "Dai, Wei" <wei.dai@intel.com> wrote: > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Laurent Hardy > > > Sent: Friday, November 18, 2016 1:30 AM > > > To: Zhang, Helin <helin.zhang@intel.com>; Ananyev, Konstantin > > > <konstantin.ananyev@intel.com> > > > Cc: dev@dpdk.org > > > Subject: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated > > > > > > In case of link speed set to 1Gb at peer side (with autoneg or with > > > defined speed) and cable not plugged-in when device is configured > > > and started, then link status is not updated properly with new speed > > > as no link setup is triggered. > > > > > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a > > > link setup each time link_update() is triggered and current link > > > status is down. When cable is plugged-in, link setup will be > > > performed via ixgbe_setup_link(). > > > > > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > > > --- > > > drivers/net/ixgbe/ixgbe_ethdev.c | 20 ++++++++++++++++++++ > > > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > > > 2 files changed, 21 insertions(+) > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > > index 52ebbe4..513d1d5 100644 > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > > @@ -2095,6 +2095,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > > > > > /* set flag to update link status after init */ > > > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > > > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > > > > > /* > > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > > > @@ -3117,8 +3118,12 @@ ixgbe_dev_link_update(struct rte_eth_dev > > > *dev, int wait_to_complete) > > > struct ixgbe_hw *hw = > > > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > > struct rte_eth_link link, old; > > > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > > > + struct ixgbe_interrupt *intr = > > > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > > > int link_up; > > > int diag; > > > + u32 speed = 0; > > > + bool autoneg = false; > > > > > > link.link_status = ETH_LINK_DOWN; > > > link.link_speed = 0; > > > @@ -3128,6 +3133,19 @@ ixgbe_dev_link_update(struct rte_eth_dev > > > *dev, int wait_to_complete) > > > > > > hw->mac.get_link_status = true; > > > > > > + if (intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) { > > > + speed = hw->phy.autoneg_advertised; > > > + if (!speed) { > > > + ixgbe_get_link_capabilities(hw, &speed, > > > &autoneg); > > > + /* setup the highest link when no autoneg > > > */ > > > + if (!autoneg) { > > > + if (speed & > > > IXGBE_LINK_SPEED_10GB_FULL) > > > + speed = > > > IXGBE_LINK_SPEED_10GB_FULL; > > > + } > > > + } > > > + ixgbe_setup_link(hw, speed, true); > > > + } > > > + > > > /* check if it needs to wait to complete, if lsc interrupt is > > > enabled */ if (wait_to_complete == 0 || > > > dev->data->dev_conf.intr_conf.lsc != 0) diag = ixgbe_check_link(hw, > > > &link_speed, &link_up, 0); @@ -3145,10 +3163,12 @@ > > > ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > > > > > if (link_up == 0) { > > > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > > > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > > if (link.link_status == old.link_status) > > > return -1; > > > return 0; > > > } > > > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > > > link.link_status = ETH_LINK_UP; > > > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > > > b/drivers/net/ixgbe/ixgbe_ethdev.h > > > index e060c3d..9d335ba 100644 > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > > > @@ -43,6 +43,7 @@ > > > #define IXGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0) > > > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > > > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > > > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 3) > > > > Now there is following macro in DPDK 17.02-rc2. > > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > > You can redefine it as #define IXGBE_FLAG_NEED_LINK_CONFIG > > (uint32_t)(1 << 4) > > Thanks, I'll send a v2. > > Do you agree with the rest of the patch? So far I have no disagree opinions about the reset of this patch. > > > Regards, > Olivier ^ permalink raw reply [flat|nested] 25+ messages in thread
* [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated 2016-11-17 17:29 [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated Laurent Hardy 2016-11-28 17:27 ` Laurent Hardy 2017-02-08 15:51 ` Dai, Wei @ 2017-02-16 16:32 ` Olivier Matz 2017-03-30 9:19 ` Olivier Matz 2 siblings, 1 reply; 25+ messages in thread From: Olivier Matz @ 2017-02-16 16:32 UTC (permalink / raw) To: dev, helin.zhang, konstantin.ananyev, wei.dai; +Cc: laurent.hardy From: Laurent Hardy <laurent.hardy@6wind.com> In case of fiber and link speed set to 1Gb at peer side (with autoneg or with defined speed), link status could be not properly updated at time cable is plugged-in. Indeed if cable was not plugged when device has been configured and started then link status will not be updated properly with new speed as no link setup will be triggered. To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link setup each time link_update() is triggered and current link status is down. When cable is plugged-in, link setup will be performed via ixgbe_setup_link(). Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> --- v1 -> v2: - rebase on top of head (change flag to 1<<4) - fix regression with copper links: only update link for fiber links drivers/net/ixgbe/ixgbe_ethdev.c | 21 +++++++++++++++++++++ drivers/net/ixgbe/ixgbe_ethdev.h | 1 + 2 files changed, 22 insertions(+) diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c index 0b89598..1114106 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.c +++ b/drivers/net/ixgbe/ixgbe_ethdev.c @@ -2357,6 +2357,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) /* set flag to update link status after init */ intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; /* * Initialize to TRUE. If any of Rx queues doesn't meet the bulk @@ -3515,8 +3516,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); struct rte_eth_link link, old; ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; + struct ixgbe_interrupt *intr = + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); int link_up; int diag; + u32 speed = 0; + bool autoneg = false; link.link_status = ETH_LINK_DOWN; link.link_speed = 0; @@ -3526,6 +3531,20 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) hw->mac.get_link_status = true; + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { + speed = hw->phy.autoneg_advertised; + if (!speed) { + ixgbe_get_link_capabilities(hw, &speed, &autoneg); + /* setup the highest link when no autoneg */ + if (!autoneg) { + if (speed & IXGBE_LINK_SPEED_10GB_FULL) + speed = IXGBE_LINK_SPEED_10GB_FULL; + } + } + ixgbe_setup_link(hw, speed, true); + } + /* check if it needs to wait to complete, if lsc interrupt is enabled */ if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3543,10 +3562,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) if (link_up == 0) { rte_ixgbe_dev_atomic_write_link_status(dev, &link); + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; if (link.link_status == old.link_status) return -1; return 0; } + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; link.link_status = ETH_LINK_UP; link.link_duplex = ETH_LINK_FULL_DUPLEX; diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h index aabbb00..bed4379 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.h +++ b/drivers/net/ixgbe/ixgbe_ethdev.h @@ -45,6 +45,7 @@ #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) /* * Defines that were not part of ixgbe_type.h as they are not used by the -- 2.8.1 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated 2017-02-16 16:32 ` [dpdk-dev] [PATCH v2] " Olivier Matz @ 2017-03-30 9:19 ` Olivier Matz 2017-03-30 16:32 ` Dai, Wei 0 siblings, 1 reply; 25+ messages in thread From: Olivier Matz @ 2017-03-30 9:19 UTC (permalink / raw) To: dev, helin.zhang, konstantin.ananyev, wei.dai; +Cc: laurent.hardy Hi, Can this patch be applied? Thanks, Olivier On Thu, 16 Feb 2017 17:32:01 +0100, Olivier Matz <olivier.matz@6wind.com> wrote: > From: Laurent Hardy <laurent.hardy@6wind.com> > > In case of fiber and link speed set to 1Gb at peer side (with autoneg > or with defined speed), link status could be not properly updated at > time cable is plugged-in. > Indeed if cable was not plugged when device has been configured and > started then link status will not be updated properly with new speed > as no link setup will be triggered. > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link > setup each time link_update() is triggered and current link status is > down. When cable is plugged-in, link setup will be performed via > ixgbe_setup_link(). > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > --- > > v1 -> v2: > - rebase on top of head (change flag to 1<<4) > - fix regression with copper links: only update link for fiber links > > drivers/net/ixgbe/ixgbe_ethdev.c | 21 +++++++++++++++++++++ > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > 2 files changed, 22 insertions(+) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c > index 0b89598..1114106 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -2357,6 +2357,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > /* set flag to update link status after init */ > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > /* > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > @@ -3515,8 +3516,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr = > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed = 0; > + bool autoneg = false; > > link.link_status = ETH_LINK_DOWN; > link.link_speed = 0; > @@ -3526,6 +3531,20 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > hw->mac.get_link_status = true; > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) { > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + /* setup the highest link when no autoneg */ > + if (!autoneg) { > + if (speed & IXGBE_LINK_SPEED_10GB_FULL) > + speed = IXGBE_LINK_SPEED_10GB_FULL; > + } > + } > + ixgbe_setup_link(hw, speed, true); > + } > + > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); > @@ -3543,10 +3562,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > if (link_up == 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status == old.link_status) > return -1; > return 0; > } > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status = ETH_LINK_UP; > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h > index aabbb00..bed4379 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -45,6 +45,7 @@ > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > /* > * Defines that were not part of ixgbe_type.h as they are not used by the ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated 2017-03-30 9:19 ` Olivier Matz @ 2017-03-30 16:32 ` Dai, Wei 2017-04-03 13:23 ` Laurent Hardy 0 siblings, 1 reply; 25+ messages in thread From: Dai, Wei @ 2017-03-30 16:32 UTC (permalink / raw) To: Olivier Matz, dev, Zhang, Helin, Ananyev, Konstantin; +Cc: laurent.hardy Hi, Olivier Has anyone already tested this patch ? Can you present some useful info on how to test it ? Can I use ethtool with some argument to downgrade or upgrade the rate of peer port ? I have just run testpmd with 82599, the hw->phy. autoneg_advertised is 0 after rte_eal_init() and rte_eth_dev_start(). So I think the codes in if (!speed) { ... } is likely to be run. I agree with most of your codes. But why to limit speed of NIC to 10Gbps if autoneg is false and 10Gbps is supported ? In this case, how about setting multiple speed ? Thanks -Wei > -----Original Message----- > From: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Thursday, March 30, 2017 5:20 PM > To: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev, > Konstantin <konstantin.ananyev@intel.com>; Dai, Wei <wei.dai@intel.com> > Cc: laurent.hardy@6wind.com > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated > > Hi, > > Can this patch be applied? > > Thanks, > Olivier > > > On Thu, 16 Feb 2017 17:32:01 +0100, Olivier Matz <olivier.matz@6wind.com> > wrote: > > From: Laurent Hardy <laurent.hardy@6wind.com> > > > > In case of fiber and link speed set to 1Gb at peer side (with autoneg > > or with defined speed), link status could be not properly updated at > > time cable is plugged-in. > > Indeed if cable was not plugged when device has been configured and > > started then link status will not be updated properly with new speed > > as no link setup will be triggered. > > > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link > > setup each time link_update() is triggered and current link status is > > down. When cable is plugged-in, link setup will be performed via > > ixgbe_setup_link(). > > > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > > --- > > > > v1 -> v2: > > - rebase on top of head (change flag to 1<<4) > > - fix regression with copper links: only update link for fiber links > > > > drivers/net/ixgbe/ixgbe_ethdev.c | 21 +++++++++++++++++++++ > > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > index 0b89598..1114106 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > @@ -2357,6 +2357,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > > > /* set flag to update link status after init */ > > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > > > /* > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk @@ > > -3515,8 +3516,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int > wait_to_complete) > > struct ixgbe_hw *hw = > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > struct rte_eth_link link, old; > > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > > + struct ixgbe_interrupt *intr = > > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > > int link_up; > > int diag; > > + u32 speed = 0; > > + bool autoneg = false; > > > > link.link_status = ETH_LINK_DOWN; > > link.link_speed = 0; > > @@ -3526,6 +3531,20 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > > int wait_to_complete) > > > > hw->mac.get_link_status = true; > > > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > > + speed = hw->phy.autoneg_advertised; > > + if (!speed) { > > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > > + /* setup the highest link when no autoneg */ > > + if (!autoneg) { > > + if (speed & IXGBE_LINK_SPEED_10GB_FULL) > > + speed = IXGBE_LINK_SPEED_10GB_FULL; > > + } > > + } > > + ixgbe_setup_link(hw, speed, true); > > + } > > + > > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3543,10 > > +3562,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int > > wait_to_complete) > > > > if (link_up == 0) { > > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > if (link.link_status == old.link_status) > > return -1; > > return 0; > > } > > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > > link.link_status = ETH_LINK_UP; > > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > > b/drivers/net/ixgbe/ixgbe_ethdev.h > > index aabbb00..bed4379 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > > @@ -45,6 +45,7 @@ > > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > > > /* > > * Defines that were not part of ixgbe_type.h as they are not used by > > the ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated 2017-03-30 16:32 ` Dai, Wei @ 2017-04-03 13:23 ` Laurent Hardy 2017-04-10 13:36 ` Ferruh Yigit 0 siblings, 1 reply; 25+ messages in thread From: Laurent Hardy @ 2017-04-03 13:23 UTC (permalink / raw) To: Dai, Wei, Olivier Matz, dev, Zhang, Helin, Ananyev, Konstantin Hi Wei, Thanks for your reply. If autoneg is false, then we set a default speed to the highest value before performing the link setup. You are right, there is no relevant reason to keep this check on speed mask. should be: + if (!autoneg) + speed = IXGBE_LINK_SPEED_10GB_FULL; Patch has been tested using testpmd with following setups: Autoneg setup: ========= - On dut, both ports of 82599 are connected to a switch with 1Gb ports - auto-negotiate option is enabled on switch Defined speed setup: ============ - set link speed to 1Gb on both ports of the switch connected to the dut Test plan: ====== - set down ports on switch before starting testpmd on dut - start testpmd, then set up ports on switch - show port on dut through testpmd without patch: --------------------- testpmd> show port info 0 ********************* Infos for port 0 ********************* MAC address: 00:1B:21:C7:3B:84 Driver name: net_ixgbe Connect to socket: 0 memory allocation on the socket: 0 Link status: down Link speed: 0 Mbps Link duplex: half-duplex Promiscuous mode: enabled Allmulticast mode: disabled Maximum number of MAC addresses: 127 Maximum number of MAC addresses of hash filtering: 4096 VLAN offload: strip on filter on qinq(extend) off Hash key size in bytes: 40 Redirection table size: 128 Supported flow types: ipv4 ipv4-tcp ipv4-udp ipv6 ipv6-tcp ipv6-udp unknown unknown unknown Max possible RX queues: 128 Max possible number of RXDs per queue: 4096 Min possible number of RXDs per queue: 32 RXDs number alignment: 8 Max possible TX queues: 64 Max possible number of TXDs per queue: 4096 Min possible number of TXDs per queue: 32 TXDs number alignment: 8 With patch: --------------- testpmd> show port info 0 ********************* Infos for port 0 ********************* MAC address: 00:1B:21:C7:3B:84 Driver name: net_ixgbe Connect to socket: 0 memory allocation on the socket: 0 Link status: up Link speed: 1000 Mbps Link duplex: full-duplex Promiscuous mode: enabled Allmulticast mode: disabled Maximum number of MAC addresses: 127 Maximum number of MAC addresses of hash filtering: 4096 VLAN offload: strip on filter on qinq(extend) off Hash key size in bytes: 40 Redirection table size: 128 Supported flow types: ipv4 ipv4-tcp ipv4-udp ipv6 ipv6-tcp ipv6-udp unknown unknown unknown Max possible RX queues: 128 Max possible number of RXDs per queue: 4096 Min possible number of RXDs per queue: 32 RXDs number alignment: 8 Max possible TX queues: 64 Max possible number of TXDs per queue: 4096 Min possible number of TXDs per queue: 32 TXDs number alignment: 8 Thanks & regards, Laurent On 03/30/2017 06:32 PM, Dai, Wei wrote: > Hi, Olivier > > Has anyone already tested this patch ? > Can you present some useful info on how to test it ? > Can I use ethtool with some argument to downgrade or upgrade the rate of peer port ? > > I have just run testpmd with 82599, the hw->phy. autoneg_advertised is 0 after rte_eal_init() and rte_eth_dev_start(). > So I think the codes in if (!speed) { ... } is likely to be run. > I agree with most of your codes. > But why to limit speed of NIC to 10Gbps if autoneg is false and 10Gbps is supported ? > In this case, how about setting multiple speed ? > > Thanks > -Wei > > >> -----Original Message----- >> From: Olivier Matz [mailto:olivier.matz@6wind.com] >> Sent: Thursday, March 30, 2017 5:20 PM >> To: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev, >> Konstantin <konstantin.ananyev@intel.com>; Dai, Wei <wei.dai@intel.com> >> Cc: laurent.hardy@6wind.com >> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated >> >> Hi, >> >> Can this patch be applied? >> >> Thanks, >> Olivier >> >> >> On Thu, 16 Feb 2017 17:32:01 +0100, Olivier Matz <olivier.matz@6wind.com> >> wrote: >>> From: Laurent Hardy <laurent.hardy@6wind.com> >>> >>> In case of fiber and link speed set to 1Gb at peer side (with autoneg >>> or with defined speed), link status could be not properly updated at >>> time cable is plugged-in. >>> Indeed if cable was not plugged when device has been configured and >>> started then link status will not be updated properly with new speed >>> as no link setup will be triggered. >>> >>> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link >>> setup each time link_update() is triggered and current link status is >>> down. When cable is plugged-in, link setup will be performed via >>> ixgbe_setup_link(). >>> >>> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> >>> --- >>> >>> v1 -> v2: >>> - rebase on top of head (change flag to 1<<4) >>> - fix regression with copper links: only update link for fiber links >>> >>> drivers/net/ixgbe/ixgbe_ethdev.c | 21 +++++++++++++++++++++ >>> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + >>> 2 files changed, 22 insertions(+) >>> >>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >>> b/drivers/net/ixgbe/ixgbe_ethdev.c >>> index 0b89598..1114106 100644 >>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >>> @@ -2357,6 +2357,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) >>> >>> /* set flag to update link status after init */ >>> intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; >>> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>> >>> /* >>> * Initialize to TRUE. If any of Rx queues doesn't meet the bulk @@ >>> -3515,8 +3516,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int >> wait_to_complete) >>> struct ixgbe_hw *hw = >> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >>> struct rte_eth_link link, old; >>> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; >>> + struct ixgbe_interrupt *intr = >>> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >>> int link_up; >>> int diag; >>> + u32 speed = 0; >>> + bool autoneg = false; >>> >>> link.link_status = ETH_LINK_DOWN; >>> link.link_speed = 0; >>> @@ -3526,6 +3531,20 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, >>> int wait_to_complete) >>> >>> hw->mac.get_link_status = true; >>> >>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { >>> + speed = hw->phy.autoneg_advertised; >>> + if (!speed) { >>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>> + /* setup the highest link when no autoneg */ >>> + if (!autoneg) { >>> + if (speed & IXGBE_LINK_SPEED_10GB_FULL) >>> + speed = IXGBE_LINK_SPEED_10GB_FULL; >>> + } >>> + } >>> + ixgbe_setup_link(hw, speed, true); >>> + } >>> + >>> /* check if it needs to wait to complete, if lsc interrupt is enabled */ >>> if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) >>> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3543,10 >>> +3562,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int >>> wait_to_complete) >>> >>> if (link_up == 0) { >>> rte_ixgbe_dev_atomic_write_link_status(dev, &link); >>> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>> if (link.link_status == old.link_status) >>> return -1; >>> return 0; >>> } >>> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; >>> link.link_status = ETH_LINK_UP; >>> link.link_duplex = ETH_LINK_FULL_DUPLEX; >>> >>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h >>> b/drivers/net/ixgbe/ixgbe_ethdev.h >>> index aabbb00..bed4379 100644 >>> --- a/drivers/net/ixgbe/ixgbe_ethdev.h >>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h >>> @@ -45,6 +45,7 @@ >>> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) >>> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) >>> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) >>> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) >>> >>> /* >>> * Defines that were not part of ixgbe_type.h as they are not used by >>> the ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated 2017-04-03 13:23 ` Laurent Hardy @ 2017-04-10 13:36 ` Ferruh Yigit 2017-04-11 12:13 ` Laurent Hardy 0 siblings, 1 reply; 25+ messages in thread From: Ferruh Yigit @ 2017-04-10 13:36 UTC (permalink / raw) To: Laurent Hardy, Dai, Wei, Olivier Matz, dev, Zhang, Helin, Ananyev, Konstantin On 4/3/2017 2:23 PM, Laurent Hardy wrote: > Hi Wei, > Thanks for your reply. > > If autoneg is false, then we set a default speed to the highest value > before performing the link setup. > You are right, there is no relevant reason to keep this check on speed > mask> should be: > + if (!autoneg) > + speed = IXGBE_LINK_SPEED_10GB_FULL; Hi Laurent, Should we expect a new version of the patch with above update? Thanks, ferruh > > Patch has been tested using testpmd with following setups: > > Autoneg setup: > ========= > - On dut, both ports of 82599 are connected to a switch with 1Gb ports > - auto-negotiate option is enabled on switch > > Defined speed setup: > ============ > - set link speed to 1Gb on both ports of the switch connected to the dut > <...> > > > Thanks & regards, > Laurent > > On 03/30/2017 06:32 PM, Dai, Wei wrote: >> Hi, Olivier >> >> Has anyone already tested this patch ? >> Can you present some useful info on how to test it ? >> Can I use ethtool with some argument to downgrade or upgrade the rate of peer port ? >> >> I have just run testpmd with 82599, the hw->phy. autoneg_advertised is 0 after rte_eal_init() and rte_eth_dev_start(). >> So I think the codes in if (!speed) { ... } is likely to be run. >> I agree with most of your codes. >> But why to limit speed of NIC to 10Gbps if autoneg is false and 10Gbps is supported ? >> In this case, how about setting multiple speed ? >> >> Thanks >> -Wei >> >> <...> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/ixgbe: ensure link status is updated 2017-04-10 13:36 ` Ferruh Yigit @ 2017-04-11 12:13 ` Laurent Hardy 2017-04-12 8:21 ` [dpdk-dev] [PATCH] " Laurent Hardy 0 siblings, 1 reply; 25+ messages in thread From: Laurent Hardy @ 2017-04-11 12:13 UTC (permalink / raw) To: Ferruh Yigit, Dai, Wei, Olivier Matz, dev, Zhang, Helin, Ananyev, Konstantin On 04/10/2017 03:36 PM, Ferruh Yigit wrote: > On 4/3/2017 2:23 PM, Laurent Hardy wrote: >> Hi Wei, >> Thanks for your reply. >> >> If autoneg is false, then we set a default speed to the highest value >> before performing the link setup. >> You are right, there is no relevant reason to keep this check on speed >> mask> should be: >> + if (!autoneg) >> + speed = IXGBE_LINK_SPEED_10GB_FULL; > Hi Laurent, > > Should we expect a new version of the patch with above update? Hi Ferruh, yes, I will send a new version today. regards, Laurent > Thanks, > ferruh > >> Patch has been tested using testpmd with following setups: >> >> Autoneg setup: >> ========= >> - On dut, both ports of 82599 are connected to a switch with 1Gb ports >> - auto-negotiate option is enabled on switch >> >> Defined speed setup: >> ============ >> - set link speed to 1Gb on both ports of the switch connected to the dut >> > <...> > >> >> Thanks & regards, >> Laurent >> >> On 03/30/2017 06:32 PM, Dai, Wei wrote: >>> Hi, Olivier >>> >>> Has anyone already tested this patch ? >>> Can you present some useful info on how to test it ? >>> Can I use ethtool with some argument to downgrade or upgrade the rate of peer port ? >>> >>> I have just run testpmd with 82599, the hw->phy. autoneg_advertised is 0 after rte_eal_init() and rte_eth_dev_start(). >>> So I think the codes in if (!speed) { ... } is likely to be run. >>> I agree with most of your codes. >>> But why to limit speed of NIC to 10Gbps if autoneg is false and 10Gbps is supported ? >>> In this case, how about setting multiple speed ? >>> >>> Thanks >>> -Wei >>> >>> > <...> > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2017-04-11 12:13 ` Laurent Hardy @ 2017-04-12 8:21 ` Laurent Hardy 2017-04-18 14:33 ` Dai, Wei 0 siblings, 1 reply; 25+ messages in thread From: Laurent Hardy @ 2017-04-12 8:21 UTC (permalink / raw) To: dev, ferruh.yigit, wei.dai, olivier.matz; +Cc: helin.zhang, konstantin.ananyev In case of fiber and link speed set to 1Gb at peer side (with autoneg or with defined speed), link status could be not properly updated at time cable is plugged-in. Indeed if cable was not plugged when device has been configured and started then link status will not be updated properly with new speed as no link setup will be triggered. To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link setup each time link_update() is triggered and current link status is down. When cable is plugged-in, link setup will be performed via ixgbe_setup_link(). Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> --- v1 -> v2: - rebase on top of head (change flag to 1<<4) - fix regression with copper links: only update link for fiber links v2 -> v3: - remove unnescessary check on speed mask if autoneg is false --- drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++++++++ drivers/net/ixgbe/ixgbe_ethdev.h | 1 + 2 files changed, 19 insertions(+) diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c index 1462324..bd03e60 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.c +++ b/drivers/net/ixgbe/ixgbe_ethdev.c @@ -3516,8 +3516,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); struct rte_eth_link link, old; ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; + struct ixgbe_interrupt *intr = + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); int link_up; int diag; + u32 speed = 0; + bool autoneg = false; link.link_status = ETH_LINK_DOWN; link.link_speed = 0; @@ -3527,6 +3531,18 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) hw->mac.get_link_status = true; + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { + speed = hw->phy.autoneg_advertised; + if (!speed) { + ixgbe_get_link_capabilities(hw, &speed, &autoneg); + /* setup the highest link when no autoneg */ + if (!autoneg) + speed = IXGBE_LINK_SPEED_10GB_FULL; + } + ixgbe_setup_link(hw, speed, true); + } + /* check if it needs to wait to complete, if lsc interrupt is enabled */ if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3544,10 +3560,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) if (link_up == 0) { rte_ixgbe_dev_atomic_write_link_status(dev, &link); + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; if (link.link_status == old.link_status) return -1; return 0; } + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; link.link_status = ETH_LINK_UP; link.link_duplex = ETH_LINK_FULL_DUPLEX; diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h index a32ba4d..a6e8c8a 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.h +++ b/drivers/net/ixgbe/ixgbe_ethdev.h @@ -45,6 +45,7 @@ #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) /* * Defines that were not part of ixgbe_type.h as they are not used by the -- 2.1.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated 2017-04-12 8:21 ` [dpdk-dev] [PATCH] " Laurent Hardy @ 2017-04-18 14:33 ` Dai, Wei 2017-04-27 15:03 ` [dpdk-dev] [PATCH v4] " Laurent Hardy 0 siblings, 1 reply; 25+ messages in thread From: Dai, Wei @ 2017-04-18 14:33 UTC (permalink / raw) To: Laurent Hardy, dev, Yigit, Ferruh, olivier.matz Cc: Zhang, Helin, Ananyev, Konstantin, Lu, Wenzhuo First of all, I agree usage of IXGBE_FLAG_NEED_LINK_CONFIG to trigger link setup when link is up again. Both ixgbe_get_link_capabilities( ) and ixgbe_setup_link( ) calls different sub-function for different ixgbe MAC type including 82598, 82599, X540 and X550 and so on. I think ixgbe_setup_link( ) can process all speeds returned from ixgbe_get_link_capabilities( ) no matter what autoneg is. It is no need to check autoneg and set speed to 10G if autoneg is false. And sometimes if autoneg is false, the speed can't be 10G, maybe should be 5G or 2.5G or others in case of multi-speed fiber. So I suggest that you can remove following 3 lines in your patch and test it again. +/* setup the highest link when no autoneg */ + if (!autoneg) + speed = IXGBE_LINK_SPEED_10GB_FULL; > -----Original Message----- > From: Laurent Hardy [mailto:laurent.hardy@6wind.com] > Sent: Wednesday, April 12, 2017 4:22 PM > To: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Dai, Wei > <wei.dai@intel.com>; olivier.matz@6wind.com > Cc: Zhang, Helin <helin.zhang@intel.com>; Ananyev, Konstantin > <konstantin.ananyev@intel.com> > Subject: [PATCH] net/ixgbe: ensure link status is updated > > In case of fiber and link speed set to 1Gb at peer side (with autoneg or with > defined speed), link status could be not properly updated at time cable is > plugged-in. > Indeed if cable was not plugged when device has been configured and started > then link status will not be updated properly with new speed as no link setup > will be triggered. > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link setup > each time link_update() is triggered and current link status is down. When > cable is plugged-in, link setup will be performed via ixgbe_setup_link(). > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > --- > > v1 -> v2: > - rebase on top of head (change flag to 1<<4) > - fix regression with copper links: only update link for fiber links > > v2 -> v3: > - remove unnescessary check on speed mask if autoneg is false > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++++++++ > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > 2 files changed, 19 insertions(+) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > b/drivers/net/ixgbe/ixgbe_ethdev.c > index 1462324..bd03e60 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -3516,8 +3516,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) > struct ixgbe_hw *hw = > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr = > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed = 0; > + bool autoneg = false; > > link.link_status = ETH_LINK_DOWN; > link.link_speed = 0; > @@ -3527,6 +3531,18 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) > > hw->mac.get_link_status = true; > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) { > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + /* setup the highest link when no autoneg */ > + if (!autoneg) > + speed = IXGBE_LINK_SPEED_10GB_FULL; > + } > + ixgbe_setup_link(hw, speed, true); > + } > + > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3544,10 > +3560,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int > wait_to_complete) > > if (link_up == 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status == old.link_status) > return -1; > return 0; > } > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status = ETH_LINK_UP; > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > b/drivers/net/ixgbe/ixgbe_ethdev.h > index a32ba4d..a6e8c8a 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -45,6 +45,7 @@ > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > /* > * Defines that were not part of ixgbe_type.h as they are not used by the > -- > 2.1.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated 2017-04-18 14:33 ` Dai, Wei @ 2017-04-27 15:03 ` Laurent Hardy 2017-04-28 0:43 ` Dai, Wei 2017-05-17 13:31 ` Roger B Melton 0 siblings, 2 replies; 25+ messages in thread From: Laurent Hardy @ 2017-04-27 15:03 UTC (permalink / raw) To: dev, wei.dai; +Cc: ferruh.yigit, helin.zhang, konstantin.ananyev, olivier.matz In case of fiber and link speed set to 1Gb at peer side (with autoneg or with defined speed), link status could be not properly updated at time cable is plugged-in. Indeed if cable was not plugged when device has been configured and started then link status will not be updated properly with new speed as no link setup will be triggered. To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link setup each time link_update() is triggered and current link status is down. When cable is plugged-in, link setup will be performed via ixgbe_setup_link(). Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> --- Hi Wei, please find enclosed patch v4, tested using testpmd. v1 -> v2: - rebase on top of head (change flag to 1<<4) - fix regression with copper links: only update link for fiber links v2 -> v3: - remove unnescessary check on speed mask if autoneg is false v3 -> v4: - remove default speed set to 10Gb if autoneg is false, rely on ixgbe_get_link_capabilities( ) instead. --- drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ drivers/net/ixgbe/ixgbe_ethdev.h | 1 + 2 files changed, 15 insertions(+) diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c index 7b856bb..8a0c0a7 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.c +++ b/drivers/net/ixgbe/ixgbe_ethdev.c @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); struct rte_eth_link link, old; ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; + struct ixgbe_interrupt *intr = + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); int link_up; int diag; + u32 speed = 0; + bool autoneg = false; link.link_status = ETH_LINK_DOWN; link.link_speed = 0; @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) hw->mac.get_link_status = true; + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { + speed = hw->phy.autoneg_advertised; + if (!speed) + ixgbe_get_link_capabilities(hw, &speed, &autoneg); + ixgbe_setup_link(hw, speed, true); + } + /* check if it needs to wait to complete, if lsc interrupt is enabled */ if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) if (link_up == 0) { rte_ixgbe_dev_atomic_write_link_status(dev, &link); + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; if (link.link_status == old.link_status) return -1; return 0; } + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; link.link_status = ETH_LINK_UP; link.link_duplex = ETH_LINK_FULL_DUPLEX; diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h index 5176b02..b576a6f 100644 --- a/drivers/net/ixgbe/ixgbe_ethdev.h +++ b/drivers/net/ixgbe/ixgbe_ethdev.h @@ -45,6 +45,7 @@ #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) /* * Defines that were not part of ixgbe_type.h as they are not used by the -- 2.1.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated 2017-04-27 15:03 ` [dpdk-dev] [PATCH v4] " Laurent Hardy @ 2017-04-28 0:43 ` Dai, Wei 2017-04-28 5:23 ` Ferruh Yigit 2017-05-17 13:31 ` Roger B Melton 1 sibling, 1 reply; 25+ messages in thread From: Dai, Wei @ 2017-04-28 0:43 UTC (permalink / raw) To: Laurent Hardy, dev Cc: Yigit, Ferruh, Zhang, Helin, Ananyev, Konstantin, olivier.matz > -----Original Message----- > From: Laurent Hardy [mailto:laurent.hardy@6wind.com] > Sent: Thursday, April 27, 2017 11:04 PM > To: dev@dpdk.org; Dai, Wei <wei.dai@intel.com> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin > <helin.zhang@intel.com>; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; olivier.matz@6wind.com > Subject: [PATCH v4] net/ixgbe: ensure link status is updated > > In case of fiber and link speed set to 1Gb at peer side (with autoneg > or with defined speed), link status could be not properly updated at > time cable is plugged-in. > Indeed if cable was not plugged when device has been configured and > started then link status will not be updated properly with new speed > as no link setup will be triggered. > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link > setup each time link_update() is triggered and current link status is > down. When cable is plugged-in, link setup will be performed via > ixgbe_setup_link(). > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> Acked-by: Wei Dai <wei.dai@intel.com> > --- > Hi Wei, please find enclosed patch v4, tested using testpmd. > > v1 -> v2: > - rebase on top of head (change flag to 1<<4) > - fix regression with copper links: only update link for fiber links > > v2 -> v3: > - remove unnescessary check on speed mask if autoneg is false > > v3 -> v4: > - remove default speed set to 10Gb if autoneg is false, rely on > ixgbe_get_link_capabilities( ) instead. > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > b/drivers/net/ixgbe/ixgbe_ethdev.c > index 7b856bb..8a0c0a7 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) > struct ixgbe_hw *hw = > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr = > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed = 0; > + bool autoneg = false; > > link.link_status = ETH_LINK_DOWN; > link.link_speed = 0; > @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) > > hw->mac.get_link_status = true; > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + ixgbe_setup_link(hw, speed, true); > + } > + > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); > @@ -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) > > if (link_up == 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status == old.link_status) > return -1; > return 0; > } > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status = ETH_LINK_UP; > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > b/drivers/net/ixgbe/ixgbe_ethdev.h > index 5176b02..b576a6f 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -45,6 +45,7 @@ > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > /* > * Defines that were not part of ixgbe_type.h as they are not used by the > -- > 2.1.4 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated 2017-04-28 0:43 ` Dai, Wei @ 2017-04-28 5:23 ` Ferruh Yigit 0 siblings, 0 replies; 25+ messages in thread From: Ferruh Yigit @ 2017-04-28 5:23 UTC (permalink / raw) To: Dai, Wei, Laurent Hardy, dev Cc: Zhang, Helin, Ananyev, Konstantin, olivier.matz On 4/28/2017 1:43 AM, Dai, Wei wrote: >> -----Original Message----- >> From: Laurent Hardy [mailto:laurent.hardy@6wind.com] >> Sent: Thursday, April 27, 2017 11:04 PM >> To: dev@dpdk.org; Dai, Wei <wei.dai@intel.com> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >> <helin.zhang@intel.com>; Ananyev, Konstantin >> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >> Subject: [PATCH v4] net/ixgbe: ensure link status is updated >> >> In case of fiber and link speed set to 1Gb at peer side (with autoneg >> or with defined speed), link status could be not properly updated at >> time cable is plugged-in. >> Indeed if cable was not plugged when device has been configured and >> started then link status will not be updated properly with new speed >> as no link setup will be triggered. >> >> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link >> setup each time link_update() is triggered and current link status is >> down. When cable is plugged-in, link setup will be performed via >> ixgbe_setup_link(). >> >> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > Acked-by: Wei Dai <wei.dai@intel.com> Applied to dpdk-next-net/master, thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated 2017-04-27 15:03 ` [dpdk-dev] [PATCH v4] " Laurent Hardy 2017-04-28 0:43 ` Dai, Wei @ 2017-05-17 13:31 ` Roger B Melton 2017-05-22 18:54 ` [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] " Roger B. Melton 1 sibling, 1 reply; 25+ messages in thread From: Roger B Melton @ 2017-05-17 13:31 UTC (permalink / raw) To: Laurent Hardy, dev, wei.dai Cc: ferruh.yigit, helin.zhang, konstantin.ananyev, olivier.matz Hi Laurent/Wei, As I continue to integrate DPDK 17.05 into my application, I have hit another issue with this patch while testing in a VM with multispeed fiber ixgbe PCI passthrough. My application periodically invokes rte_eth_link_get_nowait() to detect link state changes. If the link is down (no cable or far end disabled), ixgbe_setup_link() will not return for ~1.3seconds due to the link setup algorithm in ixgbe_common.c:ixgbe_multispeed_fiber(): + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { + speed = hw->phy.autoneg_advertised; + if (!speed) + ixgbe_get_link_capabilities(hw, &speed, &autoneg); + ixgbe_setup_link(hw, speed, true); + } + I have two questions: * Shouldn't we avoid the link setup cost if the caller has specified not to wait_to_complete? * If the concern is speed may not be properly configured, shouldn't the link setup be deferred until state changes link up thus minimizing the delays enforced in ixgbe_multispeed_fiber()? Regards, -Roger On 4/27/17 11:03 AM, Laurent Hardy wrote: > In case of fiber and link speed set to 1Gb at peer side (with autoneg > or with defined speed), link status could be not properly updated at > time cable is plugged-in. > Indeed if cable was not plugged when device has been configured and > started then link status will not be updated properly with new speed > as no link setup will be triggered. > > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link > setup each time link_update() is triggered and current link status is > down. When cable is plugged-in, link setup will be performed via > ixgbe_setup_link(). > > Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > --- > Hi Wei, please find enclosed patch v4, tested using testpmd. > > v1 -> v2: > - rebase on top of head (change flag to 1<<4) > - fix regression with copper links: only update link for fiber links > > v2 -> v3: > - remove unnescessary check on speed mask if autoneg is false > > v3 -> v4: > - remove default speed set to 10Gb if autoneg is false, rely on > ixgbe_get_link_capabilities( ) instead. > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ > drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c > index 7b856bb..8a0c0a7 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr = > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed = 0; > + bool autoneg = false; > > link.link_status = ETH_LINK_DOWN; > link.link_speed = 0; > @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > hw->mac.get_link_status = true; > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + ixgbe_setup_link(hw, speed, true); > + } > + > /* check if it needs to wait to complete, if lsc interrupt is enabled */ > if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); > @@ -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > > if (link_up == 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status == old.link_status) > return -1; > return 0; > } > + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status = ETH_LINK_UP; > link.link_duplex = ETH_LINK_FULL_DUPLEX; > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h > index 5176b02..b576a6f 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > @@ -45,6 +45,7 @@ > #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > /* > * Defines that were not part of ixgbe_type.h as they are not used by the ^ permalink raw reply [flat|nested] 25+ messages in thread
* [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] Re: [PATCH v4] net/ixgbe: ensure link status is updated 2017-05-17 13:31 ` Roger B Melton @ 2017-05-22 18:54 ` Roger B. Melton 2017-05-23 7:24 ` Dai, Wei 2017-05-23 7:41 ` Dai, Wei 0 siblings, 2 replies; 25+ messages in thread From: Roger B. Melton @ 2017-05-22 18:54 UTC (permalink / raw) To: Laurent Hardy, dev, wei.dai Cc: ferruh.yigit, helin.zhang, konstantin.ananyev, olivier.matz Hi Laurent/Wei, Any thoughts? Regards, Roger On 5/17/17 9:31 AM, Roger B Melton wrote: > Hi Laurent/Wei, > > As I continue to integrate DPDK 17.05 into my application, I have hit > another issue with this patch while testing in a VM with multispeed > fiber ixgbe PCI passthrough. My application periodically invokes > rte_eth_link_get_nowait() to detect link state changes. If the link > is down (no cable or far end disabled), ixgbe_setup_link() will not > return for ~1.3seconds due to the link setup algorithm in > ixgbe_common.c:ixgbe_multispeed_fiber(): > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > + speed = hw->phy.autoneg_advertised; > + if (!speed) > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + ixgbe_setup_link(hw, speed, true); + } > + > > I have two questions: > > * Shouldn't we avoid the link setup cost if the caller has specified > not to wait_to_complete? > * If the concern is speed may not be properly configured, shouldn't > the link setup be deferred until state changes link up thus > minimizing the delays enforced in ixgbe_multispeed_fiber()? > > > Regards, > -Roger > > > > > On 4/27/17 11:03 AM, Laurent Hardy wrote: >> In case of fiber and link speed set to 1Gb at peer side (with autoneg >> or with defined speed), link status could be not properly updated at >> time cable is plugged-in. >> Indeed if cable was not plugged when device has been configured and >> started then link status will not be updated properly with new speed >> as no link setup will be triggered. >> >> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link >> setup each time link_update() is triggered and current link status is >> down. When cable is plugged-in, link setup will be performed via >> ixgbe_setup_link(). >> >> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> >> --- >> Hi Wei, please find enclosed patch v4, tested using testpmd. >> >> v1 -> v2: >> - rebase on top of head (change flag to 1<<4) >> - fix regression with copper links: only update link for fiber links >> >> v2 -> v3: >> - remove unnescessary check on speed mask if autoneg is false >> >> v3 -> v4: >> - remove default speed set to 10Gb if autoneg is false, rely on >> ixgbe_get_link_capabilities( ) instead. >> --- >> drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ >> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + >> 2 files changed, 15 insertions(+) >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >> b/drivers/net/ixgbe/ixgbe_ethdev.c >> index 7b856bb..8a0c0a7 100644 >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >> @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, >> int wait_to_complete) >> struct ixgbe_hw *hw = >> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >> struct rte_eth_link link, old; >> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; >> + struct ixgbe_interrupt *intr = >> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >> int link_up; >> int diag; >> + u32 speed = 0; >> + bool autoneg = false; >> link.link_status = ETH_LINK_DOWN; >> link.link_speed = 0; >> @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, >> int wait_to_complete) >> hw->mac.get_link_status = true; >> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { >> + speed = hw->phy.autoneg_advertised; >> + if (!speed) >> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >> + ixgbe_setup_link(hw, speed, true); >> + } >> + >> /* check if it needs to wait to complete, if lsc interrupt is >> enabled */ >> if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc >> != 0) >> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); >> @@ -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev >> *dev, int wait_to_complete) >> if (link_up == 0) { >> rte_ixgbe_dev_atomic_write_link_status(dev, &link); >> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >> if (link.link_status == old.link_status) >> return -1; >> return 0; >> } >> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; >> link.link_status = ETH_LINK_UP; >> link.link_duplex = ETH_LINK_FULL_DUPLEX; >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h >> b/drivers/net/ixgbe/ixgbe_ethdev.h >> index 5176b02..b576a6f 100644 >> --- a/drivers/net/ixgbe/ixgbe_ethdev.h >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h >> @@ -45,6 +45,7 @@ >> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) >> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) >> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) >> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) >> /* >> * Defines that were not part of ixgbe_type.h as they are not used >> by the > > . > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] Re: [PATCH v4] net/ixgbe: ensure link status is updated 2017-05-22 18:54 ` [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] " Roger B. Melton @ 2017-05-23 7:24 ` Dai, Wei 2017-05-23 7:41 ` Dai, Wei 1 sibling, 0 replies; 25+ messages in thread From: Dai, Wei @ 2017-05-23 7:24 UTC (permalink / raw) To: Roger B. Melton, Laurent Hardy, dev Cc: Yigit, Ferruh, Zhang, Helin, Ananyev, Konstantin, olivier.matz Hi, Roger Sorry for late response as we are busy with other higher priority task. ixgbe_setup_mac_link_multispeed_fiber( ) in ixgbe_common.c calls ixgbe_setup_mac_link( ) which some functions defined in ixgbe/base . Would you please give us more info to narrow down this issue ? What device id did you use to trigger this issue ? To get more info, would you please change "CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n" to " CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=y" in config/common_base And run your application with EAL parameter --log-level=7 to get trace path of ixgbe functions for us. Thanks & Best Regards -Wei > -----Original Message----- > From: Roger B. Melton [mailto:rmelton@cisco.com] > Sent: Tuesday, May 23, 2017 2:54 AM > To: Laurent Hardy <laurent.hardy@6wind.com>; dev@dpdk.org; Dai, Wei > <wei.dai@intel.com> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin > <helin.zhang@intel.com>; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; olivier.matz@6wind.com > Subject: 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber > [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated > > Hi Laurent/Wei, > > Any thoughts? > > Regards, > Roger > > > On 5/17/17 9:31 AM, Roger B Melton wrote: > > Hi Laurent/Wei, > > > > As I continue to integrate DPDK 17.05 into my application, I have hit > > another issue with this patch while testing in a VM with multispeed > > fiber ixgbe PCI passthrough. My application periodically invokes > > rte_eth_link_get_nowait() to detect link state changes. If the link > > is down (no cable or far end disabled), ixgbe_setup_link() will not > > return for ~1.3seconds due to the link setup algorithm in > > ixgbe_common.c:ixgbe_multispeed_fiber(): > > > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > > + speed = hw->phy.autoneg_advertised; > > + if (!speed) > > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > > + ixgbe_setup_link(hw, speed, true); + } > > + > > > > I have two questions: > > > > * Shouldn't we avoid the link setup cost if the caller has specified > > not to wait_to_complete? > > * If the concern is speed may not be properly configured, shouldn't > > the link setup be deferred until state changes link up thus > > minimizing the delays enforced in ixgbe_multispeed_fiber()? > > > > > > Regards, > > -Roger > > > > > > > > > > On 4/27/17 11:03 AM, Laurent Hardy wrote: > >> In case of fiber and link speed set to 1Gb at peer side (with autoneg > >> or with defined speed), link status could be not properly updated at > >> time cable is plugged-in. > >> Indeed if cable was not plugged when device has been configured and > >> started then link status will not be updated properly with new speed > >> as no link setup will be triggered. > >> > >> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link > >> setup each time link_update() is triggered and current link status is > >> down. When cable is plugged-in, link setup will be performed via > >> ixgbe_setup_link(). > >> > >> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > >> --- > >> Hi Wei, please find enclosed patch v4, tested using testpmd. > >> > >> v1 -> v2: > >> - rebase on top of head (change flag to 1<<4) > >> - fix regression with copper links: only update link for fiber links > >> > >> v2 -> v3: > >> - remove unnescessary check on speed mask if autoneg is false > >> > >> v3 -> v4: > >> - remove default speed set to 10Gb if autoneg is false, rely on > >> ixgbe_get_link_capabilities( ) instead. > >> --- > >> drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ > >> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > >> 2 files changed, 15 insertions(+) > >> > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > >> b/drivers/net/ixgbe/ixgbe_ethdev.c > >> index 7b856bb..8a0c0a7 100644 > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > >> @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev > *dev, > >> int wait_to_complete) > >> struct ixgbe_hw *hw = > >> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > >> struct rte_eth_link link, old; > >> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > >> + struct ixgbe_interrupt *intr = > >> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > >> int link_up; > >> int diag; > >> + u32 speed = 0; > >> + bool autoneg = false; > >> link.link_status = ETH_LINK_DOWN; > >> link.link_speed = 0; > >> @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev > *dev, > >> int wait_to_complete) > >> hw->mac.get_link_status = true; > >> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > >> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > >> + speed = hw->phy.autoneg_advertised; > >> + if (!speed) > >> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > >> + ixgbe_setup_link(hw, speed, true); > >> + } > >> + > >> /* check if it needs to wait to complete, if lsc interrupt is > >> enabled */ > >> if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc > >> != 0) > >> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ > >> -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > >> int wait_to_complete) > >> if (link_up == 0) { > >> rte_ixgbe_dev_atomic_write_link_status(dev, &link); > >> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > >> if (link.link_status == old.link_status) > >> return -1; > >> return 0; > >> } > >> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > >> link.link_status = ETH_LINK_UP; > >> link.link_duplex = ETH_LINK_FULL_DUPLEX; > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > >> b/drivers/net/ixgbe/ixgbe_ethdev.h > >> index 5176b02..b576a6f 100644 > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.h > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > >> @@ -45,6 +45,7 @@ > >> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > >> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > >> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > >> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > >> /* > >> * Defines that were not part of ixgbe_type.h as they are not used > >> by the > > > > . > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] Re: [PATCH v4] net/ixgbe: ensure link status is updated 2017-05-22 18:54 ` [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] " Roger B. Melton 2017-05-23 7:24 ` Dai, Wei @ 2017-05-23 7:41 ` Dai, Wei 2017-05-24 14:40 ` Roger B. Melton 1 sibling, 1 reply; 25+ messages in thread From: Dai, Wei @ 2017-05-23 7:41 UTC (permalink / raw) To: Roger B. Melton, Laurent Hardy, dev Cc: Yigit, Ferruh, Zhang, Helin, Ananyev, Konstantin, olivier.matz Hi, Roger Sorry for typo error. You should set --log-level=8 Regards -Wei > -----Original Message----- > From: Dai, Wei > Sent: Tuesday, May 23, 2017 3:24 PM > To: 'Roger B. Melton' <rmelton@cisco.com>; Laurent Hardy > <laurent.hardy@6wind.com>; dev@dpdk.org > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin > <helin.zhang@intel.com>; Ananyev, Konstantin > <konstantin.ananyev@intel.com>; olivier.matz@6wind.com > Subject: RE: 2nd try: problem with ixgbe_dev_link_update() for multi-speed > fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated > > Hi, Roger > Sorry for late response as we are busy with other higher priority task. > ixgbe_setup_mac_link_multispeed_fiber( ) in ixgbe_common.c calls > ixgbe_setup_mac_link( ) which some functions defined in ixgbe/base . > Would you please give us more info to narrow down this issue ? > > What device id did you use to trigger this issue ? > > To get more info, would you please change > "CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n" to " > CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=y" in config/common_base And > run your application with EAL parameter --log-level=7 to get trace path of > ixgbe functions for us. > > Thanks & Best Regards > -Wei > > > > -----Original Message----- > > From: Roger B. Melton [mailto:rmelton@cisco.com] > > Sent: Tuesday, May 23, 2017 2:54 AM > > To: Laurent Hardy <laurent.hardy@6wind.com>; dev@dpdk.org; Dai, Wei > > <wei.dai@intel.com> > > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin > > <helin.zhang@intel.com>; Ananyev, Konstantin > > <konstantin.ananyev@intel.com>; olivier.matz@6wind.com > > Subject: 2nd try: problem with ixgbe_dev_link_update() for multi-speed > > fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is > > updated > > > > Hi Laurent/Wei, > > > > Any thoughts? > > > > Regards, > > Roger > > > > > > On 5/17/17 9:31 AM, Roger B Melton wrote: > > > Hi Laurent/Wei, > > > > > > As I continue to integrate DPDK 17.05 into my application, I have > > > hit another issue with this patch while testing in a VM with > > > multispeed fiber ixgbe PCI passthrough. My application periodically > > > invokes > > > rte_eth_link_get_nowait() to detect link state changes. If the link > > > is down (no cable or far end disabled), ixgbe_setup_link() will not > > > return for ~1.3seconds due to the link setup algorithm in > > > ixgbe_common.c:ixgbe_multispeed_fiber(): > > > > > > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > > > + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { > > > + speed = hw->phy.autoneg_advertised; > > > + if (!speed) > > > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > > > + ixgbe_setup_link(hw, speed, true); + } > > > + > > > > > > I have two questions: > > > > > > * Shouldn't we avoid the link setup cost if the caller has specified > > > not to wait_to_complete? > > > * If the concern is speed may not be properly configured, shouldn't > > > the link setup be deferred until state changes link up thus > > > minimizing the delays enforced in ixgbe_multispeed_fiber()? > > > > > > > > > Regards, > > > -Roger > > > > > > > > > > > > > > > On 4/27/17 11:03 AM, Laurent Hardy wrote: > > >> In case of fiber and link speed set to 1Gb at peer side (with > > >> autoneg or with defined speed), link status could be not properly > > >> updated at time cable is plugged-in. > > >> Indeed if cable was not plugged when device has been configured and > > >> started then link status will not be updated properly with new > > >> speed as no link setup will be triggered. > > >> > > >> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a > > >> link setup each time link_update() is triggered and current link > > >> status is down. When cable is plugged-in, link setup will be > > >> performed via ixgbe_setup_link(). > > >> > > >> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> > > >> --- > > >> Hi Wei, please find enclosed patch v4, tested using testpmd. > > >> > > >> v1 -> v2: > > >> - rebase on top of head (change flag to 1<<4) > > >> - fix regression with copper links: only update link for fiber > > >> links > > >> > > >> v2 -> v3: > > >> - remove unnescessary check on speed mask if autoneg is false > > >> > > >> v3 -> v4: > > >> - remove default speed set to 10Gb if autoneg is false, rely on > > >> ixgbe_get_link_capabilities( ) instead. > > >> --- > > >> drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ > > >> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + > > >> 2 files changed, 15 insertions(+) > > >> > > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > >> b/drivers/net/ixgbe/ixgbe_ethdev.c > > >> index 7b856bb..8a0c0a7 100644 > > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > >> @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev > > *dev, > > >> int wait_to_complete) > > >> struct ixgbe_hw *hw = > > >> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > >> struct rte_eth_link link, old; > > >> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; > > >> + struct ixgbe_interrupt *intr = > > >> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > > >> int link_up; > > >> int diag; > > >> + u32 speed = 0; > > >> + bool autoneg = false; > > >> link.link_status = ETH_LINK_DOWN; > > >> link.link_speed = 0; > > >> @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev > > *dev, > > >> int wait_to_complete) > > >> hw->mac.get_link_status = true; > > >> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > > >> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) > { > > >> + speed = hw->phy.autoneg_advertised; > > >> + if (!speed) > > >> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > > >> + ixgbe_setup_link(hw, speed, true); > > >> + } > > >> + > > >> /* check if it needs to wait to complete, if lsc interrupt is > > >> enabled */ > > >> if (wait_to_complete == 0 || > > >> dev->data->dev_conf.intr_conf.lsc != 0) > > >> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ > > >> -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > > >> int wait_to_complete) > > >> if (link_up == 0) { > > >> rte_ixgbe_dev_atomic_write_link_status(dev, &link); > > >> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; > > >> if (link.link_status == old.link_status) > > >> return -1; > > >> return 0; > > >> } > > >> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; > > >> link.link_status = ETH_LINK_UP; > > >> link.link_duplex = ETH_LINK_FULL_DUPLEX; > > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > > >> b/drivers/net/ixgbe/ixgbe_ethdev.h > > >> index 5176b02..b576a6f 100644 > > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.h > > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > > >> @@ -45,6 +45,7 @@ > > >> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > > >> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > > >> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) > > >> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > > >> /* > > >> * Defines that were not part of ixgbe_type.h as they are not > > >> used by the > > > > > > . > > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] Re: [PATCH v4] net/ixgbe: ensure link status is updated 2017-05-23 7:41 ` Dai, Wei @ 2017-05-24 14:40 ` Roger B. Melton 2017-05-24 18:38 ` Roger B. Melton 0 siblings, 1 reply; 25+ messages in thread From: Roger B. Melton @ 2017-05-24 14:40 UTC (permalink / raw) To: Dai, Wei, Laurent Hardy, dev Cc: Yigit, Ferruh, Zhang, Helin, Ananyev, Konstantin, olivier.matz Hi Wei, Thanks for getting back to me. I'm also dealing with various priorities so it'll take me a bit to get the data, but I'll get back to you with the details in the next day or so. Regards, Roger On 5/23/17 3:41 AM, Dai, Wei wrote: > Hi, Roger > Sorry for typo error. You should set --log-level=8 > > Regards > -Wei > >> -----Original Message----- >> From: Dai, Wei >> Sent: Tuesday, May 23, 2017 3:24 PM >> To: 'Roger B. Melton' <rmelton@cisco.com>; Laurent Hardy >> <laurent.hardy@6wind.com>; dev@dpdk.org >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >> <helin.zhang@intel.com>; Ananyev, Konstantin >> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >> Subject: RE: 2nd try: problem with ixgbe_dev_link_update() for multi-speed >> fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is updated >> >> Hi, Roger >> Sorry for late response as we are busy with other higher priority task. >> ixgbe_setup_mac_link_multispeed_fiber( ) in ixgbe_common.c calls >> ixgbe_setup_mac_link( ) which some functions defined in ixgbe/base . >> Would you please give us more info to narrow down this issue ? >> >> What device id did you use to trigger this issue ? >> >> To get more info, would you please change >> "CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n" to" >> CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=y" in config/common_base And >> run your application with EAL parameter --log-level=7 to get trace path of >> ixgbe functions for us. >> >> Thanks & Best Regards >> -Wei >> >> >>> -----Original Message----- >>> From: Roger B. Melton [mailto:rmelton@cisco.com] >>> Sent: Tuesday, May 23, 2017 2:54 AM >>> To: Laurent Hardy <laurent.hardy@6wind.com>; dev@dpdk.org; Dai, Wei >>> <wei.dai@intel.com> >>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >>> <helin.zhang@intel.com>; Ananyev, Konstantin >>> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >>> Subject: 2nd try: problem with ixgbe_dev_link_update() for multi-speed >>> fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is >>> updated >>> >>> Hi Laurent/Wei, >>> >>> Any thoughts? >>> >>> Regards, >>> Roger >>> >>> >>> On 5/17/17 9:31 AM, Roger B Melton wrote: >>>> Hi Laurent/Wei, >>>> >>>> As I continue to integrate DPDK 17.05 into my application, I have >>>> hit another issue with this patch while testing in a VM with >>>> multispeed fiber ixgbe PCI passthrough. My application periodically >>>> invokes >>>> rte_eth_link_get_nowait() to detect link state changes. If the link >>>> is down (no cable or far end disabled), ixgbe_setup_link() will not >>>> return for ~1.3seconds due to the link setup algorithm in >>>> ixgbe_common.c:ixgbe_multispeed_fiber(): >>>> >>>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { >>>> + speed = hw->phy.autoneg_advertised; >>>> + if (!speed) >>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>> + ixgbe_setup_link(hw, speed, true); + } >>>> + >>>> >>>> I have two questions: >>>> >>>> * Shouldn't we avoid the link setup cost if the caller has specified >>>> not to wait_to_complete? >>>> * If the concern is speed may not be properly configured, shouldn't >>>> the link setup be deferred until state changes link up thus >>>> minimizing the delays enforced in ixgbe_multispeed_fiber()? >>>> >>>> >>>> Regards, >>>> -Roger >>>> >>>> >>>> >>>> >>>> On 4/27/17 11:03 AM, Laurent Hardy wrote: >>>>> In case of fiber and link speed set to 1Gb at peer side (with >>>>> autoneg or with defined speed), link status could be not properly >>>>> updated at time cable is plugged-in. >>>>> Indeed if cable was not plugged when device has been configured and >>>>> started then link status will not be updated properly with new >>>>> speed as no link setup will be triggered. >>>>> >>>>> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a >>>>> link setup each time link_update() is triggered and current link >>>>> status is down. When cable is plugged-in, link setup will be >>>>> performed via ixgbe_setup_link(). >>>>> >>>>> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> >>>>> --- >>>>> Hi Wei, please find enclosed patch v4, tested using testpmd. >>>>> >>>>> v1 -> v2: >>>>> - rebase on top of head (change flag to 1<<4) >>>>> - fix regression with copper links: only update link for fiber >>>>> links >>>>> >>>>> v2 -> v3: >>>>> - remove unnescessary check on speed mask if autoneg is false >>>>> >>>>> v3 -> v4: >>>>> - remove default speed set to 10Gb if autoneg is false, rely on >>>>> ixgbe_get_link_capabilities( ) instead. >>>>> --- >>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ >>>>> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + >>>>> 2 files changed, 15 insertions(+) >>>>> >>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>> index 7b856bb..8a0c0a7 100644 >>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>> @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev >>> *dev, >>>>> int wait_to_complete) >>>>> struct ixgbe_hw *hw = >>>>> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >>>>> struct rte_eth_link link, old; >>>>> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; >>>>> + struct ixgbe_interrupt *intr = >>>>> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >>>>> int link_up; >>>>> int diag; >>>>> + u32 speed = 0; >>>>> + bool autoneg = false; >>>>> link.link_status = ETH_LINK_DOWN; >>>>> link.link_speed = 0; >>>>> @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev >>> *dev, >>>>> int wait_to_complete) >>>>> hw->mac.get_link_status = true; >>>>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) >> { >>>>> + speed = hw->phy.autoneg_advertised; >>>>> + if (!speed) >>>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>> + ixgbe_setup_link(hw, speed, true); >>>>> + } >>>>> + >>>>> /* check if it needs to wait to complete, if lsc interrupt is >>>>> enabled */ >>>>> if (wait_to_complete == 0 || >>>>> dev->data->dev_conf.intr_conf.lsc != 0) >>>>> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ >>>>> -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, >>>>> int wait_to_complete) >>>>> if (link_up == 0) { >>>>> rte_ixgbe_dev_atomic_write_link_status(dev, &link); >>>>> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>>>> if (link.link_status == old.link_status) >>>>> return -1; >>>>> return 0; >>>>> } >>>>> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; >>>>> link.link_status = ETH_LINK_UP; >>>>> link.link_duplex = ETH_LINK_FULL_DUPLEX; >>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h >>>>> b/drivers/net/ixgbe/ixgbe_ethdev.h >>>>> index 5176b02..b576a6f 100644 >>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.h >>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h >>>>> @@ -45,6 +45,7 @@ >>>>> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) >>>>> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) >>>>> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) >>>>> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) >>>>> /* >>>>> * Defines that were not part of ixgbe_type.h as they are not >>>>> used by the >>>> . >>>> -- ____________________________________________________________________ |Roger B. Melton | | Cisco Systems | |CPP Software :|: :|: 7100 Kit Creek Rd | |+1.919.476.2332 phone :|||: :|||: RTP, NC 27709-4987 | |+1.919.392.1094 fax .:|||||||:..:|||||||:. rmelton@cisco.com | | | | This email may contain confidential and privileged material for the| | sole use of the intended recipient. Any review, use, distribution | | or disclosure by others is strictly prohibited. If you are not the | | intended recipient (or authorized to receive for the recipient), | | please contact the sender by reply email and delete all copies of | | this message. | | | | For corporate legal information go to: | | http://www.cisco.com/web/about/doing_business/legal/cri/index.html | |__________________________ http://www.cisco.com ____________________| ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] Re: [PATCH v4] net/ixgbe: ensure link status is updated 2017-05-24 14:40 ` Roger B. Melton @ 2017-05-24 18:38 ` Roger B. Melton 2017-06-02 16:15 ` Roger B. Melton 0 siblings, 1 reply; 25+ messages in thread From: Roger B. Melton @ 2017-05-24 18:38 UTC (permalink / raw) To: Dai, Wei, Laurent Hardy, dev Cc: Yigit, Ferruh, Zhang, Helin, Ananyev, Konstantin, olivier.matz Hi Wei, I am using ixgbe: 00:02.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection (rev 01) The debug output you requested is below showing the results for both far end link up and far end link down. In the case of far end link up, the elapse time is too small for the granularity of my logging system. In the case when the far end link is down, the elapsed time for rte_eth_link_get_nowait() is ~1.4 seconds. You can really see those 40 and 100msec delays add up when the link is down. Let me know if you require more information. Regards, Roger Far end link up: 2017/05/24 18:15:14.353 [ulord-dp]: [3706]: UUID: 0, ra: 0 (debug): rmeltonDBG Start rte_eth_link_get_nowait() 2017/05/24 18:15:14.353 [ulord-dp]: [3706]: UUID: 0, ra: 0 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:14.353 [ulord-dp]: [3706]: UUID: 0, ra: 0 (debug): rmeltonDBG :End rte_eth_link_get_nowait() Far end link down: 2017/05/24 18:15:38.502 (debug): rmeltonDBG: Start rte_eth_link_get_nowait() 2017/05/24 18:15:38.502 (debug): PMD: ixgbe_get_media_type_82599(): ixgbe_get_media_type_82599 2017/05/24 18:15:38.502 (debug): PMD: ixgbe_setup_mac_link_multispeed_fiber(): ixgbe_setup_mac_link_multispeed_fiber 2017/05/24 18:15:38.502 (debug): PMD: ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 2017/05/24 18:15:38.542 (debug): PMD: ixgbe_setup_mac_link_82599(): ixgbe_setup_mac_link_82599 2017/05/24 18:15:38.542 (debug): PMD: ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 2017/05/24 18:15:38.542 (debug): PMD: ixgbe_flap_tx_laser_multispeed_fiber(): ixgbe_flap_tx_laser_multispeed_fiber 2017/05/24 18:15:38.542 (debug): PMD: ixgbe_check_reset_blocked(): ixgbe_check_reset_blocked 2017/05/24 18:15:38.643 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:38.743 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:38.843 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:38.943 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.043 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_setup_mac_link_82599(): ixgbe_setup_mac_link_82599 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_check_reset_blocked(): ixgbe_check_reset_blocked 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_verify_lesm_fw_enabled_82599(): ixgbe_verify_lesm_fw_enabled_82599 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_read_eeprom_82599(): ixgbe_read_eeprom_82599 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_poll_eerd_eewr_done(): ixgbe_poll_eerd_eewr_done 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_read_eeprom_82599(): ixgbe_read_eeprom_82599 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_poll_eerd_eewr_done(): ixgbe_poll_eerd_eewr_done 2017/05/24 18:15:39.165 (debug): PMD: ixgbe_flap_tx_laser_multispeed_fiber(): ixgbe_flap_tx_laser_multispeed_fiber 2017/05/24 18:15:39.165 (debug): PMD: ixgbe_check_reset_blocked(): ixgbe_check_reset_blocked 2017/05/24 18:15:39.265 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.265 (debug): PMD: ixgbe_setup_mac_link_multispeed_fiber(): ixgbe_setup_mac_link_multispeed_fiber 2017/05/24 18:15:39.265 (debug): PMD: ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_setup_mac_link_82599(): ixgbe_setup_mac_link_82599 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_check_reset_blocked(): ixgbe_check_reset_blocked 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_verify_lesm_fw_enabled_82599(): ixgbe_verify_lesm_fw_enabled_82599 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_read_eeprom_82599(): ixgbe_read_eeprom_82599 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_poll_eerd_eewr_done(): ixgbe_poll_eerd_eewr_done 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_read_eeprom_82599(): ixgbe_read_eeprom_82599 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_poll_eerd_eewr_done(): ixgbe_poll_eerd_eewr_done 2017/05/24 18:15:39.387 (debug): PMD: ixgbe_flap_tx_laser_multispeed_fiber(): ixgbe_flap_tx_laser_multispeed_fiber 2017/05/24 18:15:39.387 (debug): PMD: ixgbe_check_reset_blocked(): ixgbe_check_reset_blocked 2017/05/24 18:15:39.487 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.587 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.687 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.787 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.887 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.887 (debug): PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic 2017/05/24 18:15:39.887 (debug): rmeltonDBG posix_pmd_oper_state_get_by_id:End rte_eth_link_get_nowait() On 5/24/17 10:40 AM, Roger B. Melton wrote: > Hi Wei, > > Thanks for getting back to me. I'm also dealing with various > priorities so it'll take me a bit to get the data, but I'll get back > to you with the details in the next day or so. > > Regards, > Roger > > > On 5/23/17 3:41 AM, Dai, Wei wrote: >> Hi, Roger >> Sorry for typo error. You should set --log-level=8 >> >> Regards >> -Wei >> >>> -----Original Message----- >>> From: Dai, Wei >>> Sent: Tuesday, May 23, 2017 3:24 PM >>> To: 'Roger B. Melton' <rmelton@cisco.com>; Laurent Hardy >>> <laurent.hardy@6wind.com>; dev@dpdk.org >>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >>> <helin.zhang@intel.com>; Ananyev, Konstantin >>> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >>> Subject: RE: 2nd try: problem with ixgbe_dev_link_update() for >>> multi-speed >>> fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status >>> is updated >>> >>> Hi, Roger >>> Sorry for late response as we are busy with other higher priority task. >>> ixgbe_setup_mac_link_multispeed_fiber( ) in ixgbe_common.c calls >>> ixgbe_setup_mac_link( ) which some functions defined in ixgbe/base . >>> Would you please give us more info to narrow down this issue ? >>> >>> What device id did you use to trigger this issue ? >>> >>> To get more info, would you please change >>> "CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n" to" >>> CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=y" in config/common_base And >>> run your application with EAL parameter --log-level=7 to get trace >>> path of >>> ixgbe functions for us. >>> >>> Thanks & Best Regards >>> -Wei >>> >>> >>>> -----Original Message----- >>>> From: Roger B. Melton [mailto:rmelton@cisco.com] >>>> Sent: Tuesday, May 23, 2017 2:54 AM >>>> To: Laurent Hardy <laurent.hardy@6wind.com>; dev@dpdk.org; Dai, Wei >>>> <wei.dai@intel.com> >>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >>>> <helin.zhang@intel.com>; Ananyev, Konstantin >>>> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >>>> Subject: 2nd try: problem with ixgbe_dev_link_update() for multi-speed >>>> fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status is >>>> updated >>>> >>>> Hi Laurent/Wei, >>>> >>>> Any thoughts? >>>> >>>> Regards, >>>> Roger >>>> >>>> >>>> On 5/17/17 9:31 AM, Roger B Melton wrote: >>>>> Hi Laurent/Wei, >>>>> >>>>> As I continue to integrate DPDK 17.05 into my application, I have >>>>> hit another issue with this patch while testing in a VM with >>>>> multispeed fiber ixgbe PCI passthrough. My application periodically >>>>> invokes >>>>> rte_eth_link_get_nowait() to detect link state changes. If the link >>>>> is down (no cable or far end disabled), ixgbe_setup_link() will not >>>>> return for ~1.3seconds due to the link setup algorithm in >>>>> ixgbe_common.c:ixgbe_multispeed_fiber(): >>>>> >>>>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { >>>>> + speed = hw->phy.autoneg_advertised; >>>>> + if (!speed) >>>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>> + ixgbe_setup_link(hw, speed, true); + } >>>>> + >>>>> >>>>> I have two questions: >>>>> >>>>> * Shouldn't we avoid the link setup cost if the caller has >>>>> specified >>>>> not to wait_to_complete? >>>>> * If the concern is speed may not be properly configured, shouldn't >>>>> the link setup be deferred until state changes link up thus >>>>> minimizing the delays enforced in ixgbe_multispeed_fiber()? >>>>> >>>>> >>>>> Regards, >>>>> -Roger >>>>> >>>>> >>>>> >>>>> >>>>> On 4/27/17 11:03 AM, Laurent Hardy wrote: >>>>>> In case of fiber and link speed set to 1Gb at peer side (with >>>>>> autoneg or with defined speed), link status could be not properly >>>>>> updated at time cable is plugged-in. >>>>>> Indeed if cable was not plugged when device has been configured and >>>>>> started then link status will not be updated properly with new >>>>>> speed as no link setup will be triggered. >>>>>> >>>>>> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a >>>>>> link setup each time link_update() is triggered and current link >>>>>> status is down. When cable is plugged-in, link setup will be >>>>>> performed via ixgbe_setup_link(). >>>>>> >>>>>> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> >>>>>> --- >>>>>> Hi Wei, please find enclosed patch v4, tested using testpmd. >>>>>> >>>>>> v1 -> v2: >>>>>> - rebase on top of head (change flag to 1<<4) >>>>>> - fix regression with copper links: only update link for fiber >>>>>> links >>>>>> >>>>>> v2 -> v3: >>>>>> - remove unnescessary check on speed mask if autoneg is false >>>>>> >>>>>> v3 -> v4: >>>>>> - remove default speed set to 10Gb if autoneg is false, rely on >>>>>> ixgbe_get_link_capabilities( ) instead. >>>>>> --- >>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ >>>>>> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + >>>>>> 2 files changed, 15 insertions(+) >>>>>> >>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> index 7b856bb..8a0c0a7 100644 >>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>> @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev >>>> *dev, >>>>>> int wait_to_complete) >>>>>> struct ixgbe_hw *hw = >>>>>> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >>>>>> struct rte_eth_link link, old; >>>>>> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; >>>>>> + struct ixgbe_interrupt *intr = >>>>>> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >>>>>> int link_up; >>>>>> int diag; >>>>>> + u32 speed = 0; >>>>>> + bool autoneg = false; >>>>>> link.link_status = ETH_LINK_DOWN; >>>>>> link.link_speed = 0; >>>>>> @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev >>>> *dev, >>>>>> int wait_to_complete) >>>>>> hw->mac.get_link_status = true; >>>>>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>>>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) >>> { >>>>>> + speed = hw->phy.autoneg_advertised; >>>>>> + if (!speed) >>>>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>>> + ixgbe_setup_link(hw, speed, true); >>>>>> + } >>>>>> + >>>>>> /* check if it needs to wait to complete, if lsc interrupt is >>>>>> enabled */ >>>>>> if (wait_to_complete == 0 || >>>>>> dev->data->dev_conf.intr_conf.lsc != 0) >>>>>> diag = ixgbe_check_link(hw, &link_speed, &link_up, 0); @@ >>>>>> -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, >>>>>> int wait_to_complete) >>>>>> if (link_up == 0) { >>>>>> rte_ixgbe_dev_atomic_write_link_status(dev, &link); >>>>>> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>> if (link.link_status == old.link_status) >>>>>> return -1; >>>>>> return 0; >>>>>> } >>>>>> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>> link.link_status = ETH_LINK_UP; >>>>>> link.link_duplex = ETH_LINK_FULL_DUPLEX; >>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>> index 5176b02..b576a6f 100644 >>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>> @@ -45,6 +45,7 @@ >>>>>> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) >>>>>> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) >>>>>> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) >>>>>> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) >>>>>> /* >>>>>> * Defines that were not part of ixgbe_type.h as they are not >>>>>> used by the >>>>> . >>>>> > > = ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] Re: [PATCH v4] net/ixgbe: ensure link status is updated 2017-05-24 18:38 ` Roger B. Melton @ 2017-06-02 16:15 ` Roger B. Melton 0 siblings, 0 replies; 25+ messages in thread From: Roger B. Melton @ 2017-06-02 16:15 UTC (permalink / raw) To: Dai, Wei, Laurent Hardy, dev Cc: Yigit, Ferruh, Zhang, Helin, Ananyev, Konstantin, olivier.matz Hi Wei, Have you had a chance to look at the debug output yet? Is there any additional data you need? Regards, Roger On 5/24/17 2:38 PM, Roger B. Melton wrote: > Hi Wei, > > I am using ixgbe: > > 00:02.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit > SFI/SFP+ Network Connection (rev 01) > > The debug output you requested is below showing the results for both > far end link up and far end link down. In the case of far end link > up, the elapse time is too small for the granularity of my logging > system. In the case when the far end link is down, the elapsed time > for rte_eth_link_get_nowait() is ~1.4 seconds. You can really see > those 40 and 100msec delays add up when the link is down. > > Let me know if you require more information. > > Regards, > Roger > > > Far end link up: > > 2017/05/24 18:15:14.353 [ulord-dp]: [3706]: UUID: 0, ra: 0 (debug): > rmeltonDBG Start rte_eth_link_get_nowait() > > 2017/05/24 18:15:14.353 [ulord-dp]: [3706]: UUID: 0, ra: 0 (debug): > PMD: ixgbe_check_mac_link_generic(): ixgbe_check_mac_link_generic > > 2017/05/24 18:15:14.353 [ulord-dp]: [3706]: UUID: 0, ra: 0 (debug): > rmeltonDBG :End rte_eth_link_get_nowait() > > > Far end link down: > > > 2017/05/24 18:15:38.502 (debug): rmeltonDBG: Start > rte_eth_link_get_nowait() > > 2017/05/24 18:15:38.502 (debug): PMD: ixgbe_get_media_type_82599(): > ixgbe_get_media_type_82599 > 2017/05/24 18:15:38.502 (debug): PMD: > ixgbe_setup_mac_link_multispeed_fiber(): > ixgbe_setup_mac_link_multispeed_fiber > 2017/05/24 18:15:38.502 (debug): PMD: > ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 > 2017/05/24 18:15:38.542 (debug): PMD: ixgbe_setup_mac_link_82599(): > ixgbe_setup_mac_link_82599 > 2017/05/24 18:15:38.542 (debug): PMD: > ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 > 2017/05/24 18:15:38.542 (debug): PMD: > ixgbe_flap_tx_laser_multispeed_fiber(): > ixgbe_flap_tx_laser_multispeed_fiber > 2017/05/24 18:15:38.542 (debug): PMD: ixgbe_check_reset_blocked(): > ixgbe_check_reset_blocked > 2017/05/24 18:15:38.643 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:38.743 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:38.843 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:38.943 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.043 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_setup_mac_link_82599(): > ixgbe_setup_mac_link_82599 > 2017/05/24 18:15:39.083 (debug): PMD: > ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 > 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_check_reset_blocked(): > ixgbe_check_reset_blocked > 2017/05/24 18:15:39.083 (debug): PMD: > ixgbe_verify_lesm_fw_enabled_82599(): ixgbe_verify_lesm_fw_enabled_82599 > 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_read_eeprom_82599(): > ixgbe_read_eeprom_82599 > 2017/05/24 18:15:39.083 (debug): PMD: > ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic > 2017/05/24 18:15:39.083 (debug): PMD: > ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic > 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_poll_eerd_eewr_done(): > ixgbe_poll_eerd_eewr_done > 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_read_eeprom_82599(): > ixgbe_read_eeprom_82599 > 2017/05/24 18:15:39.083 (debug): PMD: > ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic > 2017/05/24 18:15:39.083 (debug): PMD: > ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic > 2017/05/24 18:15:39.083 (debug): PMD: ixgbe_poll_eerd_eewr_done(): > ixgbe_poll_eerd_eewr_done > 2017/05/24 18:15:39.165 (debug): PMD: > ixgbe_flap_tx_laser_multispeed_fiber(): > ixgbe_flap_tx_laser_multispeed_fiber > 2017/05/24 18:15:39.165 (debug): PMD: ixgbe_check_reset_blocked(): > ixgbe_check_reset_blocked > 2017/05/24 18:15:39.265 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.265 (debug): PMD: > ixgbe_setup_mac_link_multispeed_fiber(): > ixgbe_setup_mac_link_multispeed_fiber > 2017/05/24 18:15:39.265 (debug): PMD: > ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 > 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_setup_mac_link_82599(): > ixgbe_setup_mac_link_82599 > 2017/05/24 18:15:39.305 (debug): PMD: > ixgbe_get_link_capabilities_82599(): ixgbe_get_link_capabilities_82599 > 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_check_reset_blocked(): > ixgbe_check_reset_blocked > 2017/05/24 18:15:39.305 (debug): PMD: > ixgbe_verify_lesm_fw_enabled_82599(): ixgbe_verify_lesm_fw_enabled_82599 > 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_read_eeprom_82599(): > ixgbe_read_eeprom_82599 > 2017/05/24 18:15:39.305 (debug): PMD: > ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic > 2017/05/24 18:15:39.305 (debug): PMD: > ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic > 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_poll_eerd_eewr_done(): > ixgbe_poll_eerd_eewr_done > 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_read_eeprom_82599(): > ixgbe_read_eeprom_82599 > 2017/05/24 18:15:39.305 (debug): PMD: > ixgbe_read_eerd_buffer_generic(): ixgbe_read_eerd_buffer_generic > 2017/05/24 18:15:39.305 (debug): PMD: > ixgbe_init_eeprom_params_generic(): ixgbe_init_eeprom_params_generic > 2017/05/24 18:15:39.305 (debug): PMD: ixgbe_poll_eerd_eewr_done(): > ixgbe_poll_eerd_eewr_done > 2017/05/24 18:15:39.387 (debug): PMD: > ixgbe_flap_tx_laser_multispeed_fiber(): > ixgbe_flap_tx_laser_multispeed_fiber > 2017/05/24 18:15:39.387 (debug): PMD: ixgbe_check_reset_blocked(): > ixgbe_check_reset_blocked > 2017/05/24 18:15:39.487 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.587 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.687 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.787 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.887 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > 2017/05/24 18:15:39.887 (debug): PMD: ixgbe_check_mac_link_generic(): > ixgbe_check_mac_link_generic > > 2017/05/24 18:15:39.887 (debug): rmeltonDBG > posix_pmd_oper_state_get_by_id:End rte_eth_link_get_nowait() > > > On 5/24/17 10:40 AM, Roger B. Melton wrote: >> Hi Wei, >> >> Thanks for getting back to me. I'm also dealing with various >> priorities so it'll take me a bit to get the data, but I'll get back >> to you with the details in the next day or so. >> >> Regards, >> Roger >> >> >> On 5/23/17 3:41 AM, Dai, Wei wrote: >>> Hi, Roger >>> Sorry for typo error. You should set --log-level=8 >>> >>> Regards >>> -Wei >>> >>>> -----Original Message----- >>>> From: Dai, Wei >>>> Sent: Tuesday, May 23, 2017 3:24 PM >>>> To: 'Roger B. Melton' <rmelton@cisco.com>; Laurent Hardy >>>> <laurent.hardy@6wind.com>; dev@dpdk.org >>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >>>> <helin.zhang@intel.com>; Ananyev, Konstantin >>>> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >>>> Subject: RE: 2nd try: problem with ixgbe_dev_link_update() for >>>> multi-speed >>>> fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link status >>>> is updated >>>> >>>> Hi, Roger >>>> Sorry for late response as we are busy with other higher priority >>>> task. >>>> ixgbe_setup_mac_link_multispeed_fiber( ) in ixgbe_common.c calls >>>> ixgbe_setup_mac_link( ) which some functions defined in ixgbe/base . >>>> Would you please give us more info to narrow down this issue ? >>>> >>>> What device id did you use to trigger this issue ? >>>> >>>> To get more info, would you please change >>>> "CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n" to" >>>> CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=y" in config/common_base And >>>> run your application with EAL parameter --log-level=7 to get trace >>>> path of >>>> ixgbe functions for us. >>>> >>>> Thanks & Best Regards >>>> -Wei >>>> >>>> >>>>> -----Original Message----- >>>>> From: Roger B. Melton [mailto:rmelton@cisco.com] >>>>> Sent: Tuesday, May 23, 2017 2:54 AM >>>>> To: Laurent Hardy <laurent.hardy@6wind.com>; dev@dpdk.org; Dai, Wei >>>>> <wei.dai@intel.com> >>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Zhang, Helin >>>>> <helin.zhang@intel.com>; Ananyev, Konstantin >>>>> <konstantin.ananyev@intel.com>; olivier.matz@6wind.com >>>>> Subject: 2nd try: problem with ixgbe_dev_link_update() for >>>>> multi-speed >>>>> fiber [was] Re: [dpdk-dev] [PATCH v4] net/ixgbe: ensure link >>>>> status is >>>>> updated >>>>> >>>>> Hi Laurent/Wei, >>>>> >>>>> Any thoughts? >>>>> >>>>> Regards, >>>>> Roger >>>>> >>>>> >>>>> On 5/17/17 9:31 AM, Roger B Melton wrote: >>>>>> Hi Laurent/Wei, >>>>>> >>>>>> As I continue to integrate DPDK 17.05 into my application, I have >>>>>> hit another issue with this patch while testing in a VM with >>>>>> multispeed fiber ixgbe PCI passthrough. My application periodically >>>>>> invokes >>>>>> rte_eth_link_get_nowait() to detect link state changes. If the link >>>>>> is down (no cable or far end disabled), ixgbe_setup_link() will not >>>>>> return for ~1.3seconds due to the link setup algorithm in >>>>>> ixgbe_common.c:ixgbe_multispeed_fiber(): >>>>>> >>>>>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>>>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) { >>>>>> + speed = hw->phy.autoneg_advertised; >>>>>> + if (!speed) >>>>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>>> + ixgbe_setup_link(hw, speed, true); + } >>>>>> + >>>>>> >>>>>> I have two questions: >>>>>> >>>>>> * Shouldn't we avoid the link setup cost if the caller has >>>>>> specified >>>>>> not to wait_to_complete? >>>>>> * If the concern is speed may not be properly configured, >>>>>> shouldn't >>>>>> the link setup be deferred until state changes link up thus >>>>>> minimizing the delays enforced in ixgbe_multispeed_fiber()? >>>>>> >>>>>> >>>>>> Regards, >>>>>> -Roger >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 4/27/17 11:03 AM, Laurent Hardy wrote: >>>>>>> In case of fiber and link speed set to 1Gb at peer side (with >>>>>>> autoneg or with defined speed), link status could be not properly >>>>>>> updated at time cable is plugged-in. >>>>>>> Indeed if cable was not plugged when device has been configured and >>>>>>> started then link status will not be updated properly with new >>>>>>> speed as no link setup will be triggered. >>>>>>> >>>>>>> To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a >>>>>>> link setup each time link_update() is triggered and current link >>>>>>> status is down. When cable is plugged-in, link setup will be >>>>>>> performed via ixgbe_setup_link(). >>>>>>> >>>>>>> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com> >>>>>>> --- >>>>>>> Hi Wei, please find enclosed patch v4, tested using testpmd. >>>>>>> >>>>>>> v1 -> v2: >>>>>>> - rebase on top of head (change flag to 1<<4) >>>>>>> - fix regression with copper links: only update link for fiber >>>>>>> links >>>>>>> >>>>>>> v2 -> v3: >>>>>>> - remove unnescessary check on speed mask if autoneg is false >>>>>>> >>>>>>> v3 -> v4: >>>>>>> - remove default speed set to 10Gb if autoneg is false, rely on >>>>>>> ixgbe_get_link_capabilities( ) instead. >>>>>>> --- >>>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++++++++++++++ >>>>>>> drivers/net/ixgbe/ixgbe_ethdev.h | 1 + >>>>>>> 2 files changed, 15 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>>> index 7b856bb..8a0c0a7 100644 >>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >>>>>>> @@ -3782,8 +3782,12 @@ ixgbe_dev_link_update(struct rte_eth_dev >>>>> *dev, >>>>>>> int wait_to_complete) >>>>>>> struct ixgbe_hw *hw = >>>>>>> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >>>>>>> struct rte_eth_link link, old; >>>>>>> ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN; >>>>>>> + struct ixgbe_interrupt *intr = >>>>>>> + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); >>>>>>> int link_up; >>>>>>> int diag; >>>>>>> + u32 speed = 0; >>>>>>> + bool autoneg = false; >>>>>>> link.link_status = ETH_LINK_DOWN; >>>>>>> link.link_speed = 0; >>>>>>> @@ -3793,6 +3797,14 @@ ixgbe_dev_link_update(struct rte_eth_dev >>>>> *dev, >>>>>>> int wait_to_complete) >>>>>>> hw->mac.get_link_status = true; >>>>>>> + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && >>>>>>> + hw->mac.ops.get_media_type(hw) == ixgbe_media_type_fiber) >>>> { >>>>>>> + speed = hw->phy.autoneg_advertised; >>>>>>> + if (!speed) >>>>>>> + ixgbe_get_link_capabilities(hw, &speed, &autoneg); >>>>>>> + ixgbe_setup_link(hw, speed, true); >>>>>>> + } >>>>>>> + >>>>>>> /* check if it needs to wait to complete, if lsc >>>>>>> interrupt is >>>>>>> enabled */ >>>>>>> if (wait_to_complete == 0 || >>>>>>> dev->data->dev_conf.intr_conf.lsc != 0) >>>>>>> diag = ixgbe_check_link(hw, &link_speed, &link_up, >>>>>>> 0); @@ >>>>>>> -3810,10 +3822,12 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, >>>>>>> int wait_to_complete) >>>>>>> if (link_up == 0) { >>>>>>> rte_ixgbe_dev_atomic_write_link_status(dev, &link); >>>>>>> + intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>>> if (link.link_status == old.link_status) >>>>>>> return -1; >>>>>>> return 0; >>>>>>> } >>>>>>> + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG; >>>>>>> link.link_status = ETH_LINK_UP; >>>>>>> link.link_duplex = ETH_LINK_FULL_DUPLEX; >>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>>> index 5176b02..b576a6f 100644 >>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h >>>>>>> @@ -45,6 +45,7 @@ >>>>>>> #define IXGBE_FLAG_MAILBOX (uint32_t)(1 << 1) >>>>>>> #define IXGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) >>>>>>> #define IXGBE_FLAG_MACSEC (uint32_t)(1 << 3) >>>>>>> +#define IXGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) >>>>>>> /* >>>>>>> * Defines that were not part of ixgbe_type.h as they are not >>>>>>> used by the >>>>>> . >>>>>> >> >> > = > . > ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-06-02 16:16 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-17 17:29 [dpdk-dev] [PATCH] net/ixgbe: ensure link status is updated Laurent Hardy 2016-11-28 17:27 ` Laurent Hardy 2016-12-08 17:10 ` Ferruh Yigit 2017-02-01 14:14 ` Thomas Monjalon 2017-02-08 15:51 ` Dai, Wei 2017-02-08 17:03 ` Olivier MATZ 2017-02-13 3:17 ` Dai, Wei 2017-02-16 16:32 ` [dpdk-dev] [PATCH v2] " Olivier Matz 2017-03-30 9:19 ` Olivier Matz 2017-03-30 16:32 ` Dai, Wei 2017-04-03 13:23 ` Laurent Hardy 2017-04-10 13:36 ` Ferruh Yigit 2017-04-11 12:13 ` Laurent Hardy 2017-04-12 8:21 ` [dpdk-dev] [PATCH] " Laurent Hardy 2017-04-18 14:33 ` Dai, Wei 2017-04-27 15:03 ` [dpdk-dev] [PATCH v4] " Laurent Hardy 2017-04-28 0:43 ` Dai, Wei 2017-04-28 5:23 ` Ferruh Yigit 2017-05-17 13:31 ` Roger B Melton 2017-05-22 18:54 ` [dpdk-dev] 2nd try: problem with ixgbe_dev_link_update() for multi-speed fiber [was] " Roger B. Melton 2017-05-23 7:24 ` Dai, Wei 2017-05-23 7:41 ` Dai, Wei 2017-05-24 14:40 ` Roger B. Melton 2017-05-24 18:38 ` Roger B. Melton 2017-06-02 16:15 ` Roger B. Melton
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).