From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [PATCH v3 02/15] ethdev: add linkstatus get/set
 helper functions
Date: Tue, 9 Jan 2018 08:29:02 -0800
Message-ID: <20180109082902.475cb461@xeon-e3>
References: <20180108174514.14688-1-stephen@networkplumber.org>
 <20180108174514.14688-3-stephen@networkplumber.org>
 <c63b2a0f-956e-3f43-7997-9178f438781a@solarflare.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: <dev@dpdk.org>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-pf0-f196.google.com (mail-pf0-f196.google.com
 [209.85.192.196]) by dpdk.org (Postfix) with ESMTP id 2973F1B1B8
 for <dev@dpdk.org>; Tue,  9 Jan 2018 17:29:10 +0100 (CET)
Received: by mail-pf0-f196.google.com with SMTP id n6so8896880pfa.4
 for <dev@dpdk.org>; Tue, 09 Jan 2018 08:29:10 -0800 (PST)
In-Reply-To: <c63b2a0f-956e-3f43-7997-9178f438781a@solarflare.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On Tue, 9 Jan 2018 13:30:46 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 01/08/2018 08:45 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 <stephen@networkplumber.org>
> > ---
> >   lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> >   lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> >   2 files changed, 64 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > index a524af740f4a..6674500bbc4a 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1495,6 +1495,42 @@ 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 still contradicts to: -1 if link state has changed, 0 if the same.
> BTW, it looks like the return value of the link_update callback is not
> specified (described) and not used. It explains why different drivers
> behave differently and nobody notices it.

It looks like link_status callback could be void.
The only places that use return value from set are code that wants
to log something if status changed.