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

[Stephen Hemminger <stephen@networkplumber.org>]

On Sat, 6 Jan 2018 13:49:50 +0300
Andrew Rybchenko <arybchenko@solarflare.com> 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 <stephen@networkplumber.org>
> > ---
> >   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!