From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Hardy Subject: Re: [PATCH v2] net/ixgbe: ensure link status is updated Date: Mon, 03 Apr 2017 15:23:20 +0200 Message-ID: <58E24CC8.1080406@6wind.com> References: <1479403792-11928-1-git-send-email-laurent.hardy@6wind.com> <1487262721-22370-1-git-send-email-olivier.matz@6wind.com> <20170330111959.43ac9078@platinum> <49759EB36A64CF4892C1AFEC9231E8D650A23CB0@PGSMSX101.gar.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: "Dai, Wei" , Olivier Matz , "dev@dpdk.org" , "Zhang, Helin" , "Ananyev, Konstantin" Return-path: Received: from mail-lf0-f52.google.com (mail-lf0-f52.google.com [209.85.215.52]) by dpdk.org (Postfix) with ESMTP id 72E58271 for ; Mon, 3 Apr 2017 15:23:29 +0200 (CEST) Received: by mail-lf0-f52.google.com with SMTP id j90so73544486lfk.2 for ; Mon, 03 Apr 2017 06:23:29 -0700 (PDT) In-Reply-To: <49759EB36A64CF4892C1AFEC9231E8D650A23CB0@PGSMSX101.gar.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 ; Ananyev, >> Konstantin ; Dai, Wei >> 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 >> wrote: >>> From: Laurent Hardy >>> >>> 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 >>> --- >>> >>> 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