From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id A534525E5 for ; Sun, 16 Jul 2017 15:48:59 +0200 (CEST) Received: from pure.maildistiller.com (unknown [10.110.50.29]) by dispatch1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTP id 2BDF68004A; Sun, 16 Jul 2017 13:48:59 +0000 (UTC) X-Virus-Scanned: Proofpoint Essentials engine Received: from mx1-us1.ppe-hosted.com (unknown [10.110.49.251]) by pure.maildistiller.com (Proofpoint Essentials ESMTP Server) with ESMTPS id A78128004C; Sun, 16 Jul 2017 13:48:58 +0000 (UTC) Received: from webmail.solarflare.com (webmail.solarflare.com [12.187.104.26]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 70B4A2006C; Sun, 16 Jul 2017 13:48:58 +0000 (UTC) Received: from [192.168.239.128] (85.187.13.33) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Sun, 16 Jul 2017 06:48:54 -0700 To: Stephen Hemminger , References: <20170714183027.16021-1-stephen@networkplumber.org> <20170714183027.16021-10-stephen@networkplumber.org> From: Andrew Rybchenko Message-ID: <60caca67-cfb9-c474-7fa9-faa7752fdaf1@solarflare.com> Date: Sun, 16 Jul 2017 16:48:50 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170714183027.16021-10-stephen@networkplumber.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.33] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ocex03.SolarFlarecom.com (10.20.40.36) X-MDID: 1500212939-zvhJInms3Pw9 Subject: Re: [dpdk-dev] [RFC 09/14] sfc: use new rte_eth_link helpers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 16 Jul 2017 13:49:00 -0000 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 > --- > 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 = {