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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.

> +}
> +
> +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().

> +}
> +
>   int
>   rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
>   {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index b10e2a92da71..b47402b797c3 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2218,6 +2218,34 @@ 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 state has changed, 0 if the same.
> + */
> +int _rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> +			    const struct rte_eth_link *link);
> +
> +/**
> + * @internal
> + * Atomically get the link speed and status.
> + * @param dev
> + *  Pointer to struct rte_eth_dev.
> + * @param link
> + *  link status value.
> + */
> +void _rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> +			     struct rte_eth_link *link);
> +
>   /**
>    * Reset a Ethernet device and keep its port id.
>    *