From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl0-f67.google.com (mail-pl0-f67.google.com [209.85.160.67]) by dpdk.org (Postfix) with ESMTP id 092243250 for ; Sat, 6 Jan 2018 15:25:00 +0100 (CET) Received: by mail-pl0-f67.google.com with SMTP id 1so4891306plv.0 for ; Sat, 06 Jan 2018 06:25:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=WoHLhhDDHHx3s9uXdf4RguiU0fSmlrUPDpnP/SuolPs=; b=hzOje8RflcOQFE2EYU0OedNAma5KypgFcLZ01THviXACU8hmqPyN99HNnGv1u8Mssf PySC27cjRb1RFAFs0Pn/7b7PDYDpTQlhU5srXgbK79WVOUErMsuAyp5hBMShNyeaGv0z 9YvhNt9r/GtmoByBGmDZExZNYLwSnWsDQhSCQGLDt6JUDxTKu17JUusK7KTpTkesldiV N3X1La7wuyighMBL5lrKlmKsz/EY18JnDLUCM7djnQZgMaNXWZAklirHkPu2O1newIv+ pNKrfLrbMa32HIvmtf0Wr6UyJRU3f4vJFOJb8Bq+gvnpRhakix4b3wHAHgsKC6hmVGMN WkaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=WoHLhhDDHHx3s9uXdf4RguiU0fSmlrUPDpnP/SuolPs=; b=CXiq9eQkDEYkzpI6+lAqndp3pPkPREqj0QG+sUd79grjHtQQ6v5UUecbsrf3t2ynwv qapwsP3DxO9GAZvNyTPfchjdJWjU2TtIRkP1rjypmrqPD1TD4C0Zq4ZwjQvNyc5zYBpg 0QZ01JoWEc3Fcrjys1K3XDlxWJZFc4y5grw1zoOwO/XUZotKKmTyAx2Zt0RkDWPuJDpM tfMeLpW4LYlYzMlumRK+s29nDsXo08lSLEtTSQIlLxjpiDv6XCYuyevseX3u3J48V4SB OGypTSY2xZqVj6j9roO0E1R73mLMAZb+DQRPtu3NdxIGA78zRyp3jKIVVK5TFITBwvO/ SsgQ== X-Gm-Message-State: AKGB3mL23+wQsn9/hAbs7YNy/5HIBuPJ1YzVnM687S9yNbYDlD9rpLIQ bjxHK+vqbxDk1o3RxGbam0r43w9h548= X-Google-Smtp-Source: ACJfBotgBxSZ0EyZju1FgpsHHVyM8QpbnkGgAyFbOw8iyJgAOFPKnhXOfUg0CV8jSqdXwLtdaKXO0g== X-Received: by 10.84.233.203 with SMTP id m11mr2073090pln.200.1515248699878; Sat, 06 Jan 2018 06:24:59 -0800 (PST) Received: from xeon-e3 (204-195-18-133.wavecable.com. [204.195.18.133]) by smtp.gmail.com with ESMTPSA id j13sm16203821pff.131.2018.01.06.06.24.59 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 06 Jan 2018 06:24:59 -0800 (PST) Date: Sat, 6 Jan 2018 06:24:56 -0800 From: Stephen Hemminger To: Andrew Rybchenko Cc: Message-ID: <20180106062456.1d1eba50@xeon-e3> In-Reply-To: References: <20180106010656.9167-1-stephen@networkplumber.org> <20180106010656.9167-3-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 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: Sat, 06 Jan 2018 14:25:01 -0000 On Sat, 6 Jan 2018 13:49:50 +0300 Andrew Rybchenko wrote: > On 01/06/2018 04:06 AM, 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 | 35 +++++++++++++++++++++++++++++++++++ > > lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++ > > 2 files changed, 63 insertions(+) > > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > > index 27b52131ef01..4897e1dcda14 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1495,6 +1495,41 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) > > } > > } > > > > +int > > +_rte_eth_linkstatus_set(struct rte_eth_dev *dev, > > + const struct rte_eth_link *new_link) > > +{ > > + 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) ? -1 : 0; > > It looks like it contradicts to the function description saying that 0 is > returned if link status is the same. Looks like a mismatch in original code. Will fix that. > > +} > > + > > +void > > +_rte_eth_linkstatus_get(const struct rte_eth_dev *dev, > > + struct rte_eth_link *link) > > +{ > > + const uint64_t *src = (const uint64_t *)&(dev->data->dev_link); > > + volatile uint64_t *dst = (uint64_t *)link; > > + > > + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t)); > > + > > + /* > > + * Note: this should never fail since both destination and expected > > + * values are the same and are a pointer from caller. > > + */ > > + rte_atomic64_cmpset(dst, *dst, *src); > > As I understand nobody says that *link (i.e. *dst) is initialized, so it > could be > reading uninitialized value. Not fatal, but not good as well. > May be it is better to use something like rte_atomic64_read(). > link is a pointer passed in by the caller. If it is uninitialized, that is still ok since it will have some value. The different question is that since is almost backwards use of cmpset, not sure if processor really guarantees atomic read of source pointer. But this code is cloned from original in Intel driver so it is bug for bug compatiable!