From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 4C06B1B1A4 for ; Wed, 17 Jan 2018 08:49:15 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine 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 08D6610006B; Wed, 17 Jan 2018 07:49:13 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ocex03.SolarFlarecom.com (10.20.40.36) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Tue, 16 Jan 2018 23:49:10 -0800 To: Stephen Hemminger , References: <20180116183755.24542-1-stephen@networkplumber.org> <20180116183755.24542-3-stephen@networkplumber.org> From: Andrew Rybchenko Message-ID: Date: Wed, 17 Jan 2018 10:49:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180116183755.24542-3-stephen@networkplumber.org> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ocex03.SolarFlarecom.com (10.20.40.36) X-MDID: 1516175354-2CWRwTnwl2Ia Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v5 02/15] ethdev: add linkstatus get/set helper functions 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: Wed, 17 Jan 2018 07:49:15 -0000 On 01/16/2018 09:37 PM, Stephen Hemminger wrote: > Many drivers are all doing copy/paste of the same code to atomically > update the link status. Reduce duplication, and allow for future > changes by having common function for this. > > Signed-off-by: Stephen Hemminger > --- > lib/librte_ether/rte_ethdev.c | 22 +++------------ > lib/librte_ether/rte_ethdev.h | 62 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+), 18 deletions(-) > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index a524af740f4a..d6433af35a19 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -1447,20 +1447,6 @@ rte_eth_allmulticast_get(uint16_t port_id) > return dev->data->all_multicast; > } > > -static inline int > -rte_eth_dev_atomic_read_link_status(struct rte_eth_dev *dev, > - struct rte_eth_link *link) > -{ > - struct rte_eth_link *dst = link; > - struct rte_eth_link *src = &(dev->data->dev_link); > - > - if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst, > - *(uint64_t *)src) == 0) > - return -1; > - > - return 0; > -} > - > void > rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link) > { > @@ -1469,8 +1455,8 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link) > RTE_ETH_VALID_PORTID_OR_RET(port_id); > dev = &rte_eth_devices[port_id]; > > - if (dev->data->dev_conf.intr_conf.lsc != 0) > - rte_eth_dev_atomic_read_link_status(dev, eth_link); > + if (dev->data->dev_conf.intr_conf.lsc) > + rte_eth_linkstatus_get(dev, eth_link); > else { > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update); > (*dev->dev_ops->link_update)(dev, 1); > @@ -1486,8 +1472,8 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) > RTE_ETH_VALID_PORTID_OR_RET(port_id); > dev = &rte_eth_devices[port_id]; > > - if (dev->data->dev_conf.intr_conf.lsc != 0) > - rte_eth_dev_atomic_read_link_status(dev, eth_link); > + if (dev->data->dev_conf.intr_conf.lsc) > + rte_eth_linkstatus_get(dev, eth_link); > else { > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update); > (*dev->dev_ops->link_update)(dev, 0); > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > index b10e2a92da71..ec8255656967 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -2218,6 +2218,68 @@ int rte_eth_dev_set_link_down(uint16_t port_id); > */ > void rte_eth_dev_close(uint16_t port_id); > > + > +/** > + * @internal > + * Atomically set the link status for the specific device. > + * It is for use by DPDK device driver use only. > + * User applications should not call it > + * > + * @param dev > + * Pointer to struct rte_eth_dev. > + * @param link > + * New link status value. > + * @return > + * 1 if link status has changed; > + * 0 if link status is unchanged. Here "link status" is used as up/down status of the link. > + */ > +static inline int > +rte_eth_linkstatus_set(struct rte_eth_dev *dev, > + const struct rte_eth_link *new_link) Here (and function and the second parameter description) "link status" is used as all information about link. rte_ethdev API calls it just "link" (link_update, link_get_wait/nowait). It is not a big problem, but it is better to avoid it. Thanks. > +{ > + volatile uint64_t *dev_link > + = (volatile uint64_t *)&(dev->data->dev_link); > + union { > + uint64_t val64; > + struct rte_eth_link link; > + } orig; > + > + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t)); > + > + orig.val64 = rte_atomic64_exchange(dev_link, > + *(const uint64_t *)new_link); > + > + return orig.link.link_status != new_link->link_status; > +} > + > +/** > + * @internal > + * Atomically get the link speed and status. > + * > + * @param dev > + * Pointer to struct rte_eth_dev. > + * @param link > + * link status value. > + */ > +static inline void > +rte_eth_linkstatus_get(const struct rte_eth_dev *dev, > + struct rte_eth_link *link) > +{ > + volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link); > + uint64_t *dst = (uint64_t *)link; > + > + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t)); > + > + /* can't use rte_atomic64_read because it returns signed int */ > +#ifdef __LP64__ > + *dst = *src; > +#else > + do { > + *dst = *src; > + } while (!rte_atomic64_cmpset(src, *dst, *dst)); > +#endif > +} > + > /** > * Reset a Ethernet device and keep its port id. > *