From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dai, Wei" Subject: Re: [PATCH] net/ixgbe: ensure link status is updated Date: Tue, 18 Apr 2017 14:33:11 +0000 Message-ID: <49759EB36A64CF4892C1AFEC9231E8D650A662DC@PGSMSX101.gar.corp.intel.com> References: <58ECC85D.4010403@6wind.com> <20170412082147.19182-1-laurent.hardy@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Zhang, Helin" , "Ananyev, Konstantin" , "Lu, Wenzhuo" To: Laurent Hardy , "dev@dpdk.org" , "Yigit, Ferruh" , "olivier.matz@6wind.com" Return-path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 72B902BAE for ; Tue, 18 Apr 2017 16:33:14 +0200 (CEST) In-Reply-To: <20170412082147.19182-1-laurent.hardy@6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 i= t again. +/* setup the highest link when no autoneg */ + if (!autoneg) + speed =3D 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 ; Dai, Wei > ; olivier.matz@6wind.com > Cc: Zhang, Helin ; Ananyev, Konstantin > > Subject: [PATCH] net/ixgbe: ensure link status is updated >=20 > 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 i= s > plugged-in. > Indeed if cable was not plugged when device has been configured and start= ed > then link status will not be updated properly with new speed as no link s= etup > will be triggered. >=20 > To avoid this issue, IXGBE_FLAG_NEED_LINK_CONFIG is set to try a link set= up > each time link_update() is triggered and current link status is down. Whe= n > cable is plugged-in, link setup will be performed via ixgbe_setup_link(). >=20 > Signed-off-by: Laurent Hardy > --- >=20 > v1 -> v2: > - rebase on top of head (change flag to 1<<4) > - fix regression with copper links: only update link for fiber links >=20 > 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(+) >=20 > 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 =3D > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); > struct rte_eth_link link, old; > ixgbe_link_speed link_speed =3D IXGBE_LINK_SPEED_UNKNOWN; > + struct ixgbe_interrupt *intr =3D > + IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private); > int link_up; > int diag; > + u32 speed =3D 0; > + bool autoneg =3D false; >=20 > link.link_status =3D ETH_LINK_DOWN; > link.link_speed =3D 0; > @@ -3527,6 +3531,18 @@ ixgbe_dev_link_update(struct rte_eth_dev *dev, > int wait_to_complete) >=20 > hw->mac.get_link_status =3D true; >=20 > + if ((intr->flags & IXGBE_FLAG_NEED_LINK_CONFIG) && > + hw->mac.ops.get_media_type(hw) =3D=3D ixgbe_media_type_fiber) { > + speed =3D hw->phy.autoneg_advertised; > + if (!speed) { > + ixgbe_get_link_capabilities(hw, &speed, &autoneg); > + /* setup the highest link when no autoneg */ > + if (!autoneg) > + speed =3D 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 =3D=3D 0 || dev->data->dev_conf.intr_conf.lsc !=3D= 0) > diag =3D 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) >=20 > if (link_up =3D=3D 0) { > rte_ixgbe_dev_atomic_write_link_status(dev, &link); > + intr->flags |=3D IXGBE_FLAG_NEED_LINK_CONFIG; > if (link.link_status =3D=3D old.link_status) > return -1; > return 0; > } > + intr->flags &=3D ~IXGBE_FLAG_NEED_LINK_CONFIG; > link.link_status =3D ETH_LINK_UP; > link.link_duplex =3D ETH_LINK_FULL_DUPLEX; >=20 > 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) >=20 > /* > * Defines that were not part of ixgbe_type.h as they are not used by th= e > -- > 2.1.4