From: Roger B Melton <rmelton@cisco.com>
To: Laurent Hardy <laurent.hardy@6wind.com>, dev@dpdk.org, wei.dai@intel.com
Cc: ferruh.yigit@intel.com, helin.zhang@intel.com,
konstantin.ananyev@intel.com, olivier.matz@6wind.com
Subject: Re: [PATCH v4] net/ixgbe: ensure link status is updated
Date: Wed, 17 May 2017 09:31:09 -0400 [thread overview]
Message-ID: <1160d98e-93a7-c14c-82a9-0944a2df6e5c@cisco.com> (raw)
In-Reply-To: <1493305422-10611-1-git-send-email-laurent.hardy@6wind.com>
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
next prev parent reply other threads:[~2017-05-17 13:31 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-17 17:29 [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 ` [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 ` [PATCH] " Laurent Hardy
2017-04-18 14:33 ` Dai, Wei
2017-04-27 15:03 ` [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 [this message]
2017-05-22 18:54 ` 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1160d98e-93a7-c14c-82a9-0944a2df6e5c@cisco.com \
--to=rmelton@cisco.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=helin.zhang@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=laurent.hardy@6wind.com \
--cc=olivier.matz@6wind.com \
--cc=wei.dai@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.