From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Stephen Hemminger <stephen@networkplumber.org>, <dev@dpdk.org>
Subject: Re: [dpdk-dev] [RFC 09/14] sfc: use new rte_eth_link helpers
Date: Sun, 16 Jul 2017 16:48:50 +0300 [thread overview]
Message-ID: <60caca67-cfb9-c474-7fa9-faa7752fdaf1@solarflare.com> (raw)
In-Reply-To: <20170714183027.16021-10-stephen@networkplumber.org>
On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> Use the new API (_rte_eth_link_update) to handle link status update.
> ALso fixes a bug where this driver was not returning -1 when link status changed.
It is really good that you raise it, since:
- as far as I can see the return value of the link_update is never used
- return value is not described and it is unclear what is meant by "link
status changed" since initial point is unspecified
We have interpreted link status change as change in data->dev_link
made in the current execution flow, but not in parallel execution flow.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
> drivers/net/sfc/sfc_ev.c | 23 ++++-------------------
> 2 files changed, 11 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index 6b06f08f7c1a..f0a40ad06f23 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -231,22 +231,12 @@ static int
> sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> {
> struct sfc_adapter *sa = dev->data->dev_private;
> - struct rte_eth_link *dev_link = &dev->data->dev_link;
> - struct rte_eth_link old_link;
> struct rte_eth_link current_link;
>
> sfc_log_init(sa, "entry");
>
> -retry:
> - EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
> - *(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
> -
> if (sa->state != SFC_ADAPTER_STARTED) {
> sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, ¤t_link);
> - if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> - *(uint64_t *)&old_link,
> - *(uint64_t *)¤t_link))
> - goto retry;
> } else if (wait_to_complete) {
> efx_link_mode_t link_mode;
>
> @@ -254,21 +244,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> link_mode = EFX_LINK_UNKNOWN;
> sfc_port_link_mode_to_info(link_mode, ¤t_link);
>
> - if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> - *(uint64_t *)&old_link,
> - *(uint64_t *)¤t_link))
> - goto retry;
> } else {
> sfc_ev_mgmt_qpoll(sa);
> - *(int64_t *)¤t_link =
> - rte_atomic64_read((rte_atomic64_t *)dev_link);
> + _rte_eth_link_read(dev, ¤t_link);
> }
>
> - if (old_link.link_status != current_link.link_status)
> - sfc_info(sa, "Link status is %s",
> - current_link.link_status ? "UP" : "DOWN");
> + if (_rte_eth_link_update(dev, ¤t_link) == 0)
> + return 0;
> +
> + sfc_info(sa, "Link status is %s",
> + current_link.link_status ? "UP" : "DOWN");
>
> - return old_link.link_status == current_link.link_status ? 0 : -1;
> + return -1;
> }
>
> static void
> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
> index a16dc27b380e..fc88baef0ef3 100644
> --- a/drivers/net/sfc/sfc_ev.c
> +++ b/drivers/net/sfc/sfc_ev.c
> @@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
> {
> struct sfc_evq *evq = arg;
> struct sfc_adapter *sa = evq->sa;
> - struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
> struct rte_eth_link new_link;
> - uint64_t new_link_u64;
> - uint64_t old_link_u64;
> -
> - EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
>
> sfc_port_link_mode_to_info(link_mode, &new_link);
> + if (_rte_eth_link_update(sa->eth_dev, &new_link) == 0)
> + return B_FALSE;
It means that change of link speed without link down (not sure that it is
possible, but still) will happen without link status interrupt. May be link
status is about UP/DOWN only, but I'm not sure that it is a good limitation.
At least it is not what we want in initial implementation.
> - new_link_u64 = *(uint64_t *)&new_link;
> - do {
> - old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
> - if (old_link_u64 == new_link_u64)
> - break;
> -
> - if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> - old_link_u64, new_link_u64)) {
> - evq->sa->port.lsc_seq++;
> - break;
> - }
> - } while (B_TRUE);
> -
> - return B_FALSE;
> + evq->sa->port.lsc_seq++;
> + return B_TRUE;
> }
>
> static const efx_ev_callbacks_t sfc_ev_callbacks = {
next prev parent reply other threads:[~2017-07-16 13:48 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-14 18:30 [dpdk-dev] [RFC 00/14] link status API improvement and bugfixes Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions Stephen Hemminger
2017-07-16 13:26 ` Andrew Rybchenko
2017-07-17 15:58 ` Stephen Hemminger
2017-07-17 16:12 ` Andrew Rybchenko
2017-07-17 16:21 ` Stephen Hemminger
2017-07-17 16:31 ` Andrew Rybchenko
2017-10-11 8:32 ` Yang, Qiming
2017-10-13 15:12 ` Stephen Hemminger
2018-01-05 14:24 ` Thomas Monjalon
2018-01-05 20:15 ` Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 02/14] virtio: use eth_link_read/write (and bug fix) Stephen Hemminger
2017-07-16 12:33 ` Andrew Rybchenko
2017-07-17 16:01 ` Stephen Hemminger
2017-07-17 16:14 ` [dpdk-dev] ***Spam*** " Andrew Rybchenko
2017-07-17 16:28 ` [dpdk-dev] " Stephen Hemminger
2018-01-05 15:04 ` Thomas Monjalon
2017-07-14 18:30 ` [dpdk-dev] [RFC 03/14] bnxt: use rte_link_update Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 04/14] vmxnet3: use rte_eth_link_update Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 05/14] dpaa2: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 06/14] nfp: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 07/14] e1000: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 08/14] ixgbe: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 09/14] sfc: use new rte_eth_link helpers Stephen Hemminger
2017-07-16 13:48 ` Andrew Rybchenko [this message]
2017-07-17 16:02 ` Stephen Hemminger
2017-07-17 16:19 ` Andrew Rybchenko
2017-07-14 18:30 ` [dpdk-dev] [RFC 10/14] i40e: use rte_eth_link_update (and bug fix) Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 11/14] liquidio: use _rte_eth_link_update Stephen Hemminger
2017-07-18 10:17 ` Shijith Thotton
2017-07-14 18:30 ` [dpdk-dev] [RFC 12/14] thunderx: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 13/14] szedata: " Stephen Hemminger
2017-07-16 12:46 ` Andrew Rybchenko
2017-07-14 18:30 ` [dpdk-dev] [RFC 14/14] enic: " Stephen Hemminger
2017-07-16 13:55 ` [dpdk-dev] [RFC 00/14] link status API improvement and bugfixes Andrew Rybchenko
2018-01-05 14:29 ` Thomas Monjalon
2018-01-05 20:18 ` Stephen Hemminger
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=60caca67-cfb9-c474-7fa9-faa7752fdaf1@solarflare.com \
--to=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=stephen@networkplumber.org \
/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 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).