Re: [dpdk-dev] [PATCH v3 02/15] ethdev: add linkstatus get/set helper functions

[Stephen Hemminger <stephen@networkplumber.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.