[dpdk-dev] [PATCH v3 00/15] common ethdev linkstatus functions

[dpdk-dev] [PATCH v3 00/15] common ethdev linkstatus functions

[Stephen Hemminger]

While reviewing drivers, noticed a lot of unnecessary
duplication of code in drivers for handling the eth_dev link status
information. While consolidating this, it also became obvious that
some drivers behave differently for no good reason.

It also was a good chance to introduce atomic exchange primitives
in EAL because there are other places using cmpset where not
necessary (such as bonding).

Mostly only compile tested only, don't have all of the hardware
available (except ixgbe and virtio) to test.

v3
 - align rte_linkstatus_get with rte_atomic64_read
 - virtio use ETH_SPEED_NUM_10G
 - add net/
v2 
 - function names changed
 - rebased to current master


Stephen Hemminger (15):
  eal: introduce atomic exchange operation
  ethdev: add linkstatus get/set helper functions
  net/virtio: use eth_linkstatus_set
  net/vmxnet3: use rte_eth_linkstatus_set
  net/dpaa2: use rte_eth_linkstatus_set
  net/nfp: use rte_eth_linkstatus functions
  net/e1000: use rte_eth_linkstatus helpers
  net/ixgbe: use rte_eth_linkstatus functions
  net/sfc: use new rte_eth_linkstatus functions
  net/i40e: use rte_eth_linkstatus functions
  net/liquidio: use _rte_eth_linkstatus_set
  net/thunderx: use _rte_eth_linkstatus_set
  net/szedata: use _rte_eth_linkstatus_set
  net/octeontx: use rte_eth_linkstatus_set
  net/enic: use _rte_eth_linkstatus_set

 drivers/net/dpaa2/dpaa2_ethdev.c                   | 65 ++------------
 drivers/net/e1000/em_ethdev.c                      | 70 ++-------------
 drivers/net/e1000/igb_ethdev.c                     | 71 ++--------------
 drivers/net/enic/enic_ethdev.c                     |  5 +-
 drivers/net/enic/enic_main.c                       | 16 ++--
 drivers/net/i40e/i40e_ethdev.c                     | 44 ++--------
 drivers/net/i40e/i40e_ethdev_vf.c                  | 19 +----
 drivers/net/ixgbe/ixgbe_ethdev.c                   | 99 ++++------------------
 drivers/net/liquidio/lio_ethdev.c                  | 54 ++----------
 drivers/net/nfp/nfp_net.c                          | 75 ++--------------
 drivers/net/octeontx/octeontx_ethdev.c             | 16 +---
 drivers/net/sfc/sfc_ethdev.c                       | 27 ++----
 drivers/net/sfc/sfc_ev.c                           | 23 +----
 drivers/net/szedata2/rte_eth_szedata2.c            | 20 ++---
 drivers/net/thunderx/nicvf_ethdev.c                | 17 +---
 drivers/net/virtio/virtio_ethdev.c                 | 64 +++-----------
 drivers/net/vmxnet3/vmxnet3_ethdev.c               | 85 ++++---------------
 .../common/include/arch/x86/rte_atomic.h           | 24 ++++++
 .../common/include/arch/x86/rte_atomic_32.h        | 12 +++
 .../common/include/arch/x86/rte_atomic_64.h        | 12 +++
 lib/librte_eal/common/include/generic/rte_atomic.h | 78 +++++++++++++++++
 lib/librte_ether/rte_ethdev.c                      | 36 ++++++++
 lib/librte_ether/rte_ethdev.h                      | 28 ++++++
 23 files changed, 303 insertions(+), 657 deletions(-)

-- 
2.15.1

[dpdk-dev] [PATCH v3 01/15] eal: introduce atomic exchange operation

[Stephen Hemminger]

To handle atomic update of link status (64 bit), every driver
was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right;
therefore make it a EAL routine.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../common/include/arch/x86/rte_atomic.h           | 24 +++++++
 .../common/include/arch/x86/rte_atomic_32.h        | 12 ++++
 .../common/include/arch/x86/rte_atomic_64.h        | 12 ++++
 lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++++++
 4 files changed, 126 insertions(+)

diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic.h b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
index 8469f97e193a..20d10cc18e4e 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
@@ -59,6 +59,18 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)
 	return res;
 }
 
+static inline uint16_t
+rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val)
+{
+	asm volatile(
+			MPLOCKED
+			"xchgw %0, %1;"
+			: "=r" (val), "=m" (*dst)
+			: "0" (val),  "m" (*dst)
+			: "memory");         /* no-clobber list */
+	return val;
+}
+
 static inline int rte_atomic16_test_and_set(rte_atomic16_t *v)
 {
 	return rte_atomic16_cmpset((volatile uint16_t *)&v->cnt, 0, 1);
@@ -133,6 +145,18 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)
 	return res;
 }
 
+static inline uint32_t
+rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val)
+{
+	asm volatile(
+			MPLOCKED
+			"xchgl %0, %1;"
+			: "=r" (val), "=m" (*dst)
+			: "0" (val),  "m" (*dst)
+			: "memory");         /* no-clobber list */
+	return val;
+}
+
 static inline int rte_atomic32_test_and_set(rte_atomic32_t *v)
 {
 	return rte_atomic32_cmpset((volatile uint32_t *)&v->cnt, 0, 1);
diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h
index fb3abf187998..43fa59355ac5 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h
@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
 	return res;
 }
 
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dest, uint64_t val)
+{
+	uint64_t old;
+
+	do {
+		old = *dest;
+	} while (rte_atomic64_t_cmpset(dest, old, val));
+
+	return old;
+}
+
 static inline void
 rte_atomic64_init(rte_atomic64_t *v)
 {
diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
index 1a53a766bd72..fd2ec9c53796 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
@@ -71,6 +71,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
 	return res;
 }
 
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val)
+{
+	asm volatile(
+			MPLOCKED
+			"xchgq %0, %1;"
+			: "=r" (val), "=m" (*dst)
+			: "0" (val),  "m" (*dst)
+			: "memory");         /* no-clobber list */
+	return val;
+}
+
 static inline void
 rte_atomic64_init(rte_atomic64_t *v)
 {
diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h b/lib/librte_eal/common/include/generic/rte_atomic.h
index 16af5ca57e01..97854df3d134 100644
--- a/lib/librte_eal/common/include/generic/rte_atomic.h
+++ b/lib/librte_eal/common/include/generic/rte_atomic.h
@@ -139,6 +139,32 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)
 }
 #endif
 
+/**
+ * Atomic exchange.
+ *
+ * (atomic) equivalent to:
+ *   ret = *dst
+ *   *dst = val;
+ *   return ret;
+ *
+ * @param dst
+ *   The destination location into which the value will be written.
+ * @param val
+ *   The new value.
+ * @return
+ *   The original value at that location
+ */
+static inline uint16_t
+rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val);
+
+#ifdef RTE_FORCE_INTRINSICS
+static inline uint16_t
+rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val)
+{
+	return __atomic_exchange_2(dst, val, __ATOMIC_SEQ_CST);
+}
+#endif
+
 /**
  * The atomic counter structure.
  */
@@ -392,6 +418,32 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)
 }
 #endif
 
+/**
+ * Atomic exchange.
+ *
+ * (atomic) equivalent to:
+ *   ret = *dst
+ *   *dst = val;
+ *   return ret;
+ *
+ * @param dst
+ *   The destination location into which the value will be written.
+ * @param val
+ *   The new value.
+ * @return
+ *   The original value at that location
+ */
+static inline uint32_t
+rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val);
+
+#ifdef RTE_FORCE_INTRINSICS
+static inline uint32_t
+rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val)
+{
+	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST);
+}
+#endif
+
 /**
  * The atomic counter structure.
  */
@@ -644,6 +696,32 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
 }
 #endif
 
+/**
+ * Atomic exchange.
+ *
+ * (atomic) equivalent to:
+ *   ret = *dst
+ *   *dst = val;
+ *   return ret;
+ *
+ * @param dst
+ *   The destination location into which the value will be written.
+ * @param val
+ *   The new value.
+ * @return
+ *   The original value at that location
+ */
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val);
+
+#ifdef RTE_FORCE_INTRINSICS
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val)
+{
+	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST);
+}
+#endif
+
 /**
  * The atomic counter structure.
  */
-- 
2.15.1

Re: [dpdk-dev] [PATCH v3 01/15] eal: introduce atomic exchange operation

[Ferruh Yigit]

On 1/8/2018 5:45 PM, Stephen Hemminger wrote:
> To handle atomic update of link status (64 bit), every driver
> was doing its own version using cmpset.
> Atomic exchange is a useful primitive in its own right;
> therefore make it a EAL routine.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

> @@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
>  	return res;
>  }
>  
> +static inline uint64_t
> +rte_atomic64_exchange(volatile uint64_t *dest, uint64_t val)
> +{
> +	uint64_t old;
> +
> +	do {
> +		old = *dest;
> +	} while (rte_atomic64_t_cmpset(dest, old, val));

rte_atomic64_cmpset ? (without _t)

> +
> +	return old;
> +}

<...>

Re: [dpdk-dev] [PATCH v3 01/15] eal: introduce atomic exchange operation

[Stephen Hemminger]

On Thu, 11 Jan 2018 17:01:10 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/8/2018 5:45 PM, Stephen Hemminger wrote:
> > To handle atomic update of link status (64 bit), every driver
> > was doing its own version using cmpset.
> > Atomic exchange is a useful primitive in its own right;
> > therefore make it a EAL routine.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> <...>
> 
> > @@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
> >  	return res;
> >  }
> >  
> > +static inline uint64_t
> > +rte_atomic64_exchange(volatile uint64_t *dest, uint64_t val)
> > +{
> > +	uint64_t old;
> > +
> > +	do {
> > +		old = *dest;
> > +	} while (rte_atomic64_t_cmpset(dest, old, val));  
> 
> rte_atomic64_cmpset ? (without _t)
> 
> > +
> > +	return old;
> > +}  
> 
> <...>
> 

Addressed in followup patch, needed to build on 32bit.

Will fold into next version.

Re: [dpdk-dev] [PATCH v3 01/15] eal: introduce atomic exchange operation

[Stephen Hemminger]

On Thu, 11 Jan 2018 17:01:10 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/8/2018 5:45 PM, Stephen Hemminger wrote:
> > To handle atomic update of link status (64 bit), every driver
> > was doing its own version using cmpset.
> > Atomic exchange is a useful primitive in its own right;
> > therefore make it a EAL routine.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> <...>
> 
> > @@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
> >  	return res;
> >  }
> >  
> > +static inline uint64_t
> > +rte_atomic64_exchange(volatile uint64_t *dest, uint64_t val)
> > +{
> > +	uint64_t old;
> > +
> > +	do {
> > +		old = *dest;
> > +	} while (rte_atomic64_t_cmpset(dest, old, val));  
> 
> rte_atomic64_cmpset ? (without _t)

Wasn't that fixed in later version?

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

[Stephen Hemminger]

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;
+}
+
+void
+_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
+			struct rte_eth_link *link)
+{
+	volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link);
+	uint64_t tmp;
+
+	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+	/* can't use rte_atomic64_read because it returns signed int */
+	do {
+		tmp = *src;
+	} while (!rte_atomic64_cmpset(src, tmp, tmp));
+
+	memcpy(link, &tmp, sizeof(tmp));
+}
+
 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.
  *
-- 
2.15.1

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

[Andrew Rybchenko]

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.

> +}
> +
> +void
> +_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> +			struct rte_eth_link *link)
> +{
> +	volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link);
> +	uint64_t tmp;
> +
> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> +	/* can't use rte_atomic64_read because it returns signed int */
> +	do {
> +		tmp = *src;
> +	} while (!rte_atomic64_cmpset(src, tmp, tmp));
> +
> +	memcpy(link, &tmp, sizeof(tmp));
> +}
> +
>   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.
>    *

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

[Stephen Hemminger]

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.

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

[Ferruh Yigit]

On 1/9/2018 4:29 PM, Stephen Hemminger wrote:
> 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>

<...>

>>> +int
>>> +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>>> +		       const struct rte_eth_link *new_link)

I see "_rte" prefix is used to say this is internal only but this is not the
syntax for other internal APIs, only there is one using this syntax, so may be
confusing to have a mix of them.

"@internal" marker in function comment already saying that this is internal.
Also I send a patch to separate public APIs and PMD APIs to different files,
hopefully that will also help clarifying target of the APIs.

And although these two new APIs are internal, they need to be in .map file for
shared library case.

>>> +{
>>> +	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.

+1, function documentation in header file and this conflicts, one needs fixing.

>> 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.

When a set function returns -1/0, it looks like it returns set status. Returning
if link status changes seems odd to me. What about returning original link
struct, so PMD may get this information if they want.

[dpdk-dev] [PATCH v3 03/15] net/virtio: use eth_linkstatus_set

[Stephen Hemminger]

Use the new comon code in ethdev to handle link status update.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_ethdev.c | 64 +++++++-------------------------------
 1 file changed, 11 insertions(+), 53 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 21f2131a9efd..ac46c749aaa4 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -14,7 +14,6 @@
 #include <rte_string_fns.h>
 #include <rte_memzone.h>
 #include <rte_malloc.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
@@ -771,46 +770,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
 	.mac_addr_set            = virtio_mac_addr_set,
 };
 
-static inline int
-virtio_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-			*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-virtio_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-		struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static void
 virtio_update_stats(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -1913,8 +1872,10 @@ static void
 virtio_dev_stop(struct rte_eth_dev *dev)
 {
 	struct virtio_hw *hw = dev->data->dev_private;
-	struct rte_eth_link link;
 	struct rte_intr_conf *intr_conf = &dev->data->dev_conf.intr_conf;
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+	};
 
 	PMD_INIT_LOG(DEBUG, "stop");
 
@@ -1922,21 +1883,19 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 		virtio_intr_disable(dev);
 
 	hw->started = 0;
-	memset(&link, 0, sizeof(link));
-	virtio_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
 virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 {
-	struct rte_eth_link link, old;
-	uint16_t status;
 	struct virtio_hw *hw = dev->data->dev_private;
-	memset(&link, 0, sizeof(link));
-	virtio_dev_atomic_read_link_status(dev, &link);
-	old = link;
-	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	link.link_speed  = SPEED_10G;
+	uint16_t status;
+	struct rte_eth_link link = {
+		.link_speed = ETH_SPEED_NUM_10G,
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_SPEED_FIXED,
+	};
 
 	if (hw->started == 0) {
 		link.link_status = ETH_LINK_DOWN;
@@ -1957,9 +1916,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 	} else {
 		link.link_status = ETH_LINK_UP;
 	}
-	virtio_dev_atomic_write_link_status(dev, &link);
 
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.15.1

Re: [dpdk-dev] [PATCH v3 03/15] net/virtio: use eth_linkstatus_set

[Ferruh Yigit]

On 1/8/2018 5:45 PM, Stephen Hemminger wrote:
> Use the new comon code in ethdev to handle link status update.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

>  static int
>  virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
>  {
> -	struct rte_eth_link link, old;
> -	uint16_t status;
>  	struct virtio_hw *hw = dev->data->dev_private;
> -	memset(&link, 0, sizeof(link));
> -	virtio_dev_atomic_read_link_status(dev, &link);
> -	old = link;
> -	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> -	link.link_speed  = SPEED_10G;
> +	uint16_t status;
> +	struct rte_eth_link link = {
> +		.link_speed = ETH_SPEED_NUM_10G,
> +		.link_duplex = ETH_LINK_FULL_DUPLEX,
> +		.link_autoneg = ETH_LINK_SPEED_FIXED,

With a new cleanup commit [1] ETH_LINK_FIXED used for autoneg value (instead of
ETH_LINK_SPEED_FIXED). And since is ETH_LINK_FIXED is 0, setting autoneg to it
can be omitted.

[1]
93dd7b328a9b ("ethdev: fix link autonegotiation value")

This commit is in next-net.
Unlike Linux "net-next" sub-tree which is for next release development tree,
dpdk next-net is for current release network drivers which this patch seems fits
into.

I already rebased this patchset to next-net, I will share it to continue
discussion from that point.

> +	};
>  
>  	if (hw->started == 0) {
>  		link.link_status = ETH_LINK_DOWN;
> @@ -1957,9 +1916,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
>  	} else {
>  		link.link_status = ETH_LINK_UP;
>  	}
> -	virtio_dev_atomic_write_link_status(dev, &link);
>  
> -	return (old.link_status == link.link_status) ? -1 : 0;
> +	return _rte_eth_linkstatus_set(dev, &link);
>  }
>  
>  static int
> 

[dpdk-dev] [PATCH v3 04/15] net/vmxnet3: use rte_eth_linkstatus_set

[Stephen Hemminger]

Use new _rte_eth_link_update helper.
Also remove no longer necessary include of rte_atomic.h

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 85 +++++++-----------------------------
 1 file changed, 15 insertions(+), 70 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index c2f12cdf2a4d..0149125affe9 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_debug.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_memory.h>
 #include <rte_memzone.h>
@@ -157,59 +156,6 @@ gpa_zone_reserve(struct rte_eth_dev *dev, uint32_t size,
 	return rte_memzone_reserve_aligned(z_name, size, socket_id, 0, align);
 }
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-
-static int
-vmxnet3_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				    struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to write to.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static int
-vmxnet3_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				     struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 /*
  * This function is based on vmxnet3_disable_intr()
  */
@@ -814,9 +760,13 @@ vmxnet3_dev_start(struct rte_eth_dev *dev)
 static void
 vmxnet3_dev_stop(struct rte_eth_dev *dev)
 {
-	struct rte_eth_link link;
 	struct vmxnet3_hw *hw = dev->data->dev_private;
-
+	struct rte_eth_link link = {
+		.link_speed = ETH_SPEED_NUM_10G,
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_SPEED_FIXED,
+		.link_status = ETH_LINK_DOWN,
+	};
 	PMD_INIT_FUNC_TRACE();
 
 	if (hw->adapter_stopped == 1) {
@@ -849,8 +799,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	vmxnet3_dev_clear_queues(dev);
 
 	/* Clear recorded link status */
-	memset(&link, 0, sizeof(link));
-	vmxnet3_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 /*
@@ -1130,25 +1079,21 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev,
 			  __rte_unused int wait_to_complete)
 {
 	struct vmxnet3_hw *hw = dev->data->dev_private;
-	struct rte_eth_link old = { 0 }, link;
+	struct rte_eth_link link = {
+		.link_speed = ETH_SPEED_NUM_10G,
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_SPEED_FIXED,
+		.link_status = ETH_LINK_DOWN,
+	};
 	uint32_t ret;
 
-	memset(&link, 0, sizeof(link));
-	vmxnet3_dev_atomic_read_link_status(dev, &old);
-
 	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_GET_LINK);
 	ret = VMXNET3_READ_BAR1_REG(hw, VMXNET3_REG_CMD);
 
-	if (ret & 0x1) {
+	if (ret & 0x1)
 		link.link_status = ETH_LINK_UP;
-		link.link_duplex = ETH_LINK_FULL_DUPLEX;
-		link.link_speed = ETH_SPEED_NUM_10G;
-		link.link_autoneg = ETH_LINK_SPEED_FIXED;
-	}
-
-	vmxnet3_dev_atomic_write_link_status(dev, &link);
 
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.15.1

Re: [dpdk-dev] [PATCH v3 04/15] net/vmxnet3: use rte_eth_linkstatus_set

[Ferruh Yigit]

On 1/8/2018 5:45 PM, Stephen Hemminger wrote:
> Use new _rte_eth_link_update helper.
> Also remove no longer necessary include of rte_atomic.h
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

> @@ -1130,25 +1079,21 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev,
>  			  __rte_unused int wait_to_complete)
>  {
>  	struct vmxnet3_hw *hw = dev->data->dev_private;
> -	struct rte_eth_link old = { 0 }, link;
> +	struct rte_eth_link link = {
> +		.link_speed = ETH_SPEED_NUM_10G,
> +		.link_duplex = ETH_LINK_FULL_DUPLEX,
> +		.link_autoneg = ETH_LINK_SPEED_FIXED,
> +		.link_status = ETH_LINK_DOWN,

Link status before this patch was "ETH_LINK_UP", this is changing it to
"ETH_LINK_DOWN", intentional?

> +	};
>  	uint32_t ret;
>  
> -	memset(&link, 0, sizeof(link));
> -	vmxnet3_dev_atomic_read_link_status(dev, &old);
> -
>  	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_GET_LINK);
>  	ret = VMXNET3_READ_BAR1_REG(hw, VMXNET3_REG_CMD);
>  
> -	if (ret & 0x1) {
> +	if (ret & 0x1)
>  		link.link_status = ETH_LINK_UP;
> -		link.link_duplex = ETH_LINK_FULL_DUPLEX;
> -		link.link_speed = ETH_SPEED_NUM_10G;
> -		link.link_autoneg = ETH_LINK_SPEED_FIXED;
> -	}
> -
> -	vmxnet3_dev_atomic_write_link_status(dev, &link);
>  
> -	return (old.link_status == link.link_status) ? -1 : 0;
> +	return _rte_eth_linkstatus_set(dev, &link);
>  }
>  
>  static int
> 

[dpdk-dev] [PATCH v3 05/15] net/dpaa2: use rte_eth_linkstatus_set

[Stephen Hemminger]

Use new helper function to update the link status.
As a good side effect this fixes a but because this driver was not
returning correct status (should be -1 in link_status changed).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++-----------------------------------
 1 file changed, 7 insertions(+), 58 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 2f99d95d2216..c0fc59ab148f 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -56,58 +56,6 @@ static int dpaa2_dev_set_link_up(struct rte_eth_dev *dev);
 static int dpaa2_dev_set_link_down(struct rte_eth_dev *dev);
 static int dpaa2_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-dpaa2_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				  struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &dev->data->dev_link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-dpaa2_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				   struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static int
 dpaa2_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
 {
@@ -804,7 +752,7 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_link_write(dev, &link);
 }
 
 static void
@@ -836,7 +784,7 @@ dpaa2_dev_close(struct rte_eth_dev *dev)
 	}
 
 	memset(&link, 0, sizeof(link));
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_link_write(dev, &link);
 }
 
 static void
@@ -1291,8 +1239,8 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 		RTE_LOG(ERR, PMD, "dpni is NULL\n");
 		return 0;
 	}
-	memset(&old, 0, sizeof(old));
-	dpaa2_dev_atomic_read_link_status(dev, &old);
+
+	_rte_eth_link_read(dev, &old);
 
 	ret = dpni_get_link_state(dpni, CMD_PRI_LOW, priv->token, &state);
 	if (ret < 0) {
@@ -1314,13 +1262,14 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 	else
 		link.link_duplex = ETH_LINK_FULL_DUPLEX;
 
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	ret = _rte_eth_linkstatus_set(dev, &link);
 
 	if (link.link_status)
 		PMD_DRV_LOG(INFO, "Port %d Link is Up\n", dev->data->port_id);
 	else
 		PMD_DRV_LOG(INFO, "Port %d Link is Down", dev->data->port_id);
-	return 0;
+
+	return ret;
 }
 
 /**
-- 
2.15.1

[dpdk-dev] [PATCH v3 06/15] net/nfp: use rte_eth_linkstatus functions

[Stephen Hemminger]

Use new _rte_eth_linkstatus_get/set helper function.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/nfp/nfp_net.c | 75 +++++------------------------------------------
 1 file changed, 8 insertions(+), 67 deletions(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 0501156bacb7..79c031ed2745 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -204,57 +204,6 @@ nn_cfg_writeq(struct nfp_net_hw *hw, int off, uint64_t val)
 	nn_writeq(rte_cpu_to_le_64(val), hw->ctrl_bar + off);
 }
 
-/*
- * Atomically reads link status information from global structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-nfp_net_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				    struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &dev->data->dev_link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/*
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-nfp_net_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				     struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static void
 nfp_net_rx_queue_release_mbufs(struct nfp_net_rxq *rxq)
 {
@@ -977,7 +926,7 @@ static int
 nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 {
 	struct nfp_net_hw *hw;
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	uint32_t nn_link_status;
 
 	static const uint32_t ls_to_ethtool[] = {
@@ -995,9 +944,6 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	memset(&old, 0, sizeof(old));
-	nfp_net_dev_atomic_read_link_status(dev, &old);
-
 	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
 
 	memset(&link, 0, sizeof(struct rte_eth_link));
@@ -1015,15 +961,13 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	else
 		link.link_speed = ls_to_ethtool[nn_link_status];
 
-	if (old.link_status != link.link_status) {
-		nfp_net_dev_atomic_write_link_status(dev, &link);
-		if (link.link_status)
-			PMD_DRV_LOG(INFO, "NIC Link is Up\n");
-		else
-			PMD_DRV_LOG(INFO, "NIC Link is Down\n");
+	if (_rte_eth_linkstatus_set(dev, &link) == 0)
 		return 0;
-	}
 
+	if (link.link_status)
+		PMD_DRV_LOG(INFO, "NIC Link is Up\n");
+	else
+		PMD_DRV_LOG(INFO, "NIC Link is Down\n");
 	return -1;
 }
 
@@ -1354,8 +1298,7 @@ nfp_net_dev_link_status_print(struct rte_eth_dev *dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_eth_link link;
 
-	memset(&link, 0, sizeof(link));
-	nfp_net_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 	if (link.link_status)
 		RTE_LOG(INFO, PMD, "Port %d: Link Up - speed %u Mbps - %s\n",
 			dev->data->port_id, link.link_speed,
@@ -1408,9 +1351,7 @@ nfp_net_dev_interrupt_handler(void *param)
 
 	PMD_DRV_LOG(DEBUG, "We got a LSC interrupt!!!\n");
 
-	/* get the link status */
-	memset(&link, 0, sizeof(link));
-	nfp_net_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 
 	nfp_net_link_update(dev, 0);
 
-- 
2.15.1

[dpdk-dev] [PATCH v3 07/15] net/e1000: use rte_eth_linkstatus helpers

[Stephen Hemminger]

Use new rte_eth_linkstatus_get/set API.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/e1000/em_ethdev.c  | 70 +++--------------------------------------
 drivers/net/e1000/igb_ethdev.c | 71 +++---------------------------------------
 2 files changed, 10 insertions(+), 131 deletions(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f457be55e706..4d32c6aa93be 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_ethdev_pci.h>
 #include <rte_memory.h>
 #include <rte_eal.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
 
@@ -193,57 +192,6 @@ static const struct eth_dev_ops eth_em_ops = {
 	.txq_info_get         = em_txq_info_get,
 };
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_em_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_em_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
 
 /**
  *  eth_em_dev_is_ich8 - Check for ICH8 device
@@ -769,7 +717,7 @@ eth_em_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -1127,7 +1075,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int link_check, count;
 
 	link_check = 0;
@@ -1162,8 +1110,6 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_read_link_status(dev, &link);
-	old = link;
 
 	/* Now we check if a transition has happened */
 	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
@@ -1182,14 +1128,8 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_DOWN;
 		link.link_autoneg = ETH_LINK_SPEED_FIXED;
 	}
-	rte_em_dev_atomic_write_link_status(dev, &link);
-
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
 
-	/* changed */
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 /*
@@ -1599,8 +1539,8 @@ eth_em_interrupt_action(struct rte_eth_dev *dev,
 	if (ret < 0)
 		return 0;
 
-	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
+
 	if (link.link_status) {
 		PMD_INIT_LOG(INFO, " Port %d: Link Up - speed %u Mbps - %s",
 			     dev->data->port_id, link.link_speed,
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 1333b62c3378..997e7a47ac0e 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_ethdev_pci.h>
 #include <rte_memory.h>
 #include <rte_eal.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
 
@@ -522,57 +521,6 @@ static const struct rte_igb_xstats_name_off rte_igbvf_stats_strings[] = {
 #define IGBVF_NB_XSTATS (sizeof(rte_igbvf_stats_strings) / \
 		sizeof(rte_igbvf_stats_strings[0]))
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_igb_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_igb_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
 
 static inline void
 igb_intr_enable(struct rte_eth_dev *dev)
@@ -1524,7 +1472,7 @@ eth_igb_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -1600,7 +1548,7 @@ eth_igb_close(struct rte_eth_dev *dev)
 	}
 
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
@@ -2347,7 +2295,7 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int link_check, count;
 
 	link_check = 0;
@@ -2388,8 +2336,6 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		rte_delay_ms(IGB_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_read_link_status(dev, &link);
-	old = link;
 
 	/* Now we check if a transition has happened */
 	if (link_check) {
@@ -2408,14 +2354,8 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_DOWN;
 		link.link_autoneg = ETH_LINK_SPEED_FIXED;
 	}
-	rte_igb_dev_atomic_write_link_status(dev, &link);
 
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
-
-	/* changed */
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 /*
@@ -2853,8 +2793,7 @@ eth_igb_interrupt_action(struct rte_eth_dev *dev,
 		if (ret < 0)
 			return 0;
 
-		memset(&link, 0, sizeof(link));
-		rte_igb_dev_atomic_read_link_status(dev, &link);
+		_rte_eth_linkstatus_get(dev, &link);
 		if (link.link_status) {
 			PMD_INIT_LOG(INFO,
 				     " Port %d: Link Up - speed %u Mbps - %s",
-- 
2.15.1

[dpdk-dev] [PATCH v3 08/15] net/ixgbe: use rte_eth_linkstatus functions

[Stephen Hemminger]

Use the new helper functions from eth_dev for
handling atomic link_info update.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 99 +++++++---------------------------------
 1 file changed, 17 insertions(+), 82 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 43e0132f81fe..d33ffa626e4a 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_debug.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_memory.h>
 #include <rte_eal.h>
@@ -781,58 +780,6 @@ static const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[] = {
 #define IXGBEVF_NB_XSTATS (sizeof(rte_ixgbevf_stats_strings) /	\
 		sizeof(rte_ixgbevf_stats_strings[0]))
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_ixgbe_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_ixgbe_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 /*
  * This function is the same as ixgbe_is_sfp() in base/ixgbe.h.
  */
@@ -2753,7 +2700,7 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	/* Clear recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_ixgbe_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -3938,7 +3885,6 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 			    int wait_to_complete, int vf)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
 	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
@@ -3947,13 +3893,11 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	u32 speed = 0;
 	int wait = 1;
 	bool autoneg = false;
-
-	link.link_status = ETH_LINK_DOWN;
-	link.link_speed = 0;
-	link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	link.link_autoneg = ETH_LINK_AUTONEG;
-	memset(&old, 0, sizeof(old));
-	rte_ixgbe_dev_atomic_read_link_status(dev, &old);
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+		.link_duplex = ETH_LINK_HALF_DUPLEX,
+		.link_autoneg = ETH_LINK_AUTONEG,
+	};
 
 	hw->mac.get_link_status = true;
 
@@ -3977,19 +3921,15 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	if (diag != 0) {
 		link.link_speed = ETH_SPEED_NUM_100M;
 		link.link_duplex = ETH_LINK_FULL_DUPLEX;
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+
+		return _rte_eth_linkstatus_set(dev, &link);
 	}
 
 	if (link_up == 0) {
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
 		intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+		return _rte_eth_linkstatus_set(dev, &link);
 	}
+
 	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 	link.link_status = ETH_LINK_UP;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
@@ -4021,12 +3961,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		link.link_speed = ETH_SPEED_NUM_10G;
 		break;
 	}
-	rte_ixgbe_dev_atomic_write_link_status(dev, &link);
 
-	if (link.link_status == old.link_status)
-		return -1;
-
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
@@ -4225,8 +4161,8 @@ ixgbe_dev_link_status_print(struct rte_eth_dev *dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_eth_link link;
 
-	memset(&link, 0, sizeof(link));
-	rte_ixgbe_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
+
 	if (link.link_status) {
 		PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s",
 					(int)(dev->data->port_id),
@@ -4261,7 +4197,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	int64_t timeout;
-	struct rte_eth_link link;
 	struct ixgbe_hw *hw =
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
@@ -4278,9 +4213,10 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 	}
 
 	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE) {
+		struct rte_eth_link link;
+
 		/* get the link status before link update, for predicting later */
-		memset(&link, 0, sizeof(link));
-		rte_ixgbe_dev_atomic_read_link_status(dev, &link);
+		_rte_eth_linkstatus_get(dev, &link);
 
 		ixgbe_dev_link_update(dev, 0);
 
@@ -6799,9 +6735,8 @@ ixgbe_start_timecounters(struct rte_eth_dev *dev)
 	uint32_t shift = 0;
 
 	/* Get current link speed. */
-	memset(&link, 0, sizeof(link));
 	ixgbe_dev_link_update(dev, 1);
-	rte_ixgbe_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 
 	switch (link.link_speed) {
 	case ETH_SPEED_NUM_100M:
-- 
2.15.1

[dpdk-dev] [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions

[Stephen Hemminger]

Use the new API (_rte_eth_linkstatus_set) to handle link status update.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
 drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
 2 files changed, 11 insertions(+), 39 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 2f5f86f84877..e0a12b32b1a3 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -238,22 +238,12 @@ static int
 sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct sfc_adapter *sa = dev->data->dev_private;
-	struct rte_eth_link *dev_link = &dev->data->dev_link;
-	struct rte_eth_link old_link;
 	struct rte_eth_link current_link;
 
 	sfc_log_init(sa, "entry");
 
-retry:
-	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
-	*(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
-
 	if (sa->state != SFC_ADAPTER_STARTED) {
 		sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, &current_link);
-		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					 *(uint64_t *)&old_link,
-					 *(uint64_t *)&current_link))
-			goto retry;
 	} else if (wait_to_complete) {
 		efx_link_mode_t link_mode;
 
@@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 			link_mode = EFX_LINK_UNKNOWN;
 		sfc_port_link_mode_to_info(link_mode, &current_link);
 
-		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					 *(uint64_t *)&old_link,
-					 *(uint64_t *)&current_link))
-			goto retry;
 	} else {
 		sfc_ev_mgmt_qpoll(sa);
-		*(int64_t *)&current_link =
-			rte_atomic64_read((rte_atomic64_t *)dev_link);
+		_rte_eth_linkstatus_get(dev, &current_link);
 	}
 
-	if (old_link.link_status != current_link.link_status)
-		sfc_info(sa, "Link status is %s",
-			 current_link.link_status ? "UP" : "DOWN");
+	if (_rte_eth_linkstatus_set(dev, &current_link) == 0)
+		return 0;
+
+	sfc_info(sa, "Link status is %s",
+		 current_link.link_status ? "UP" : "DOWN");
 
-	return old_link.link_status == current_link.link_status ? 0 : -1;
+	return -1;
 }
 
 static void
diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index a16dc27b380e..3e96536a9d60 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
 {
 	struct sfc_evq *evq = arg;
 	struct sfc_adapter *sa = evq->sa;
-	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
 	struct rte_eth_link new_link;
-	uint64_t new_link_u64;
-	uint64_t old_link_u64;
-
-	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
 
 	sfc_port_link_mode_to_info(link_mode, &new_link);
+	if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
+		return B_FALSE;
 
-	new_link_u64 = *(uint64_t *)&new_link;
-	do {
-		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
-		if (old_link_u64 == new_link_u64)
-			break;
-
-		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					old_link_u64, new_link_u64)) {
-			evq->sa->port.lsc_seq++;
-			break;
-		}
-	} while (B_TRUE);
-
-	return B_FALSE;
+	evq->sa->port.lsc_seq++;
+	return B_TRUE;
 }
 
 static const efx_ev_callbacks_t sfc_ev_callbacks = {
-- 
2.15.1

Re: [dpdk-dev] [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions

[Andrew Rybchenko]

On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
> Use the new API (_rte_eth_linkstatus_set) to handle link status update.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
>   drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
>   2 files changed, 11 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index 2f5f86f84877..e0a12b32b1a3 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -238,22 +238,12 @@ static int
>   sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>   {
>   	struct sfc_adapter *sa = dev->data->dev_private;
> -	struct rte_eth_link *dev_link = &dev->data->dev_link;
> -	struct rte_eth_link old_link;
>   	struct rte_eth_link current_link;
>   
>   	sfc_log_init(sa, "entry");
>   
> -retry:
> -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
> -	*(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
> -
>   	if (sa->state != SFC_ADAPTER_STARTED) {
>   		sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, &current_link);
> -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> -					 *(uint64_t *)&old_link,
> -					 *(uint64_t *)&current_link))
> -			goto retry;
>   	} else if (wait_to_complete) {
>   		efx_link_mode_t link_mode;
>   
> @@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>   			link_mode = EFX_LINK_UNKNOWN;
>   		sfc_port_link_mode_to_info(link_mode, &current_link);
>   
> -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> -					 *(uint64_t *)&old_link,
> -					 *(uint64_t *)&current_link))
> -			goto retry;
>   	} else {
>   		sfc_ev_mgmt_qpoll(sa);
> -		*(int64_t *)&current_link =
> -			rte_atomic64_read((rte_atomic64_t *)dev_link);
> +		_rte_eth_linkstatus_get(dev, &current_link);
>   	}
>   
> -	if (old_link.link_status != current_link.link_status)
> -		sfc_info(sa, "Link status is %s",
> -			 current_link.link_status ? "UP" : "DOWN");
> +	if (_rte_eth_linkstatus_set(dev, &current_link) == 0)
> +		return 0;
> +
> +	sfc_info(sa, "Link status is %s",
> +		 current_link.link_status ? "UP" : "DOWN");
>   
> -	return old_link.link_status == current_link.link_status ? 0 : -1;
> +	return -1;
>   }
>   
>   static void
> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
> index a16dc27b380e..3e96536a9d60 100644
> --- a/drivers/net/sfc/sfc_ev.c
> +++ b/drivers/net/sfc/sfc_ev.c
> @@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
>   {
>   	struct sfc_evq *evq = arg;
>   	struct sfc_adapter *sa = evq->sa;
> -	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
>   	struct rte_eth_link new_link;
> -	uint64_t new_link_u64;
> -	uint64_t old_link_u64;
> -
> -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
>   
>   	sfc_port_link_mode_to_info(link_mode, &new_link);
> +	if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
> +		return B_FALSE;
>   
> -	new_link_u64 = *(uint64_t *)&new_link;
> -	do {
> -		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
> -		if (old_link_u64 == new_link_u64)
> -			break;
> -
> -		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> -					old_link_u64, new_link_u64)) {
> -			evq->sa->port.lsc_seq++;
> -			break;
> -		}
> -	} while (B_TRUE);
> -
> -	return B_FALSE;
> +	evq->sa->port.lsc_seq++;
> +	return B_TRUE;

It still returns B_TRUE, but should return B_FALSE as before.
Also before the patch lsc_seq is incremented in the case of any
changes in link status, but now in the case of up/down change only.

>   }
>   
>   static const efx_ev_callbacks_t sfc_ev_callbacks = {

Re: [dpdk-dev] [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions

[Stephen Hemminger]

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

> On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
> > Use the new API (_rte_eth_linkstatus_set) to handle link status update.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >   drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
> >   drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
> >   2 files changed, 11 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> > index 2f5f86f84877..e0a12b32b1a3 100644
> > --- a/drivers/net/sfc/sfc_ethdev.c
> > +++ b/drivers/net/sfc/sfc_ethdev.c
> > @@ -238,22 +238,12 @@ static int
> >   sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> >   {
> >   	struct sfc_adapter *sa = dev->data->dev_private;
> > -	struct rte_eth_link *dev_link = &dev->data->dev_link;
> > -	struct rte_eth_link old_link;
> >   	struct rte_eth_link current_link;
> >   
> >   	sfc_log_init(sa, "entry");
> >   
> > -retry:
> > -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
> > -	*(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
> > -
> >   	if (sa->state != SFC_ADAPTER_STARTED) {
> >   		sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, &current_link);
> > -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > -					 *(uint64_t *)&old_link,
> > -					 *(uint64_t *)&current_link))
> > -			goto retry;
> >   	} else if (wait_to_complete) {
> >   		efx_link_mode_t link_mode;
> >   
> > @@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
> >   			link_mode = EFX_LINK_UNKNOWN;
> >   		sfc_port_link_mode_to_info(link_mode, &current_link);
> >   
> > -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > -					 *(uint64_t *)&old_link,
> > -					 *(uint64_t *)&current_link))
> > -			goto retry;
> >   	} else {
> >   		sfc_ev_mgmt_qpoll(sa);
> > -		*(int64_t *)&current_link =
> > -			rte_atomic64_read((rte_atomic64_t *)dev_link);
> > +		_rte_eth_linkstatus_get(dev, &current_link);
> >   	}
> >   
> > -	if (old_link.link_status != current_link.link_status)
> > -		sfc_info(sa, "Link status is %s",
> > -			 current_link.link_status ? "UP" : "DOWN");
> > +	if (_rte_eth_linkstatus_set(dev, &current_link) == 0)
> > +		return 0;
> > +
> > +	sfc_info(sa, "Link status is %s",
> > +		 current_link.link_status ? "UP" : "DOWN");
> >   
> > -	return old_link.link_status == current_link.link_status ? 0 : -1;
> > +	return -1;
> >   }
> >   
> >   static void
> > diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
> > index a16dc27b380e..3e96536a9d60 100644
> > --- a/drivers/net/sfc/sfc_ev.c
> > +++ b/drivers/net/sfc/sfc_ev.c
> > @@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
> >   {
> >   	struct sfc_evq *evq = arg;
> >   	struct sfc_adapter *sa = evq->sa;
> > -	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
> >   	struct rte_eth_link new_link;
> > -	uint64_t new_link_u64;
> > -	uint64_t old_link_u64;
> > -
> > -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
> >   
> >   	sfc_port_link_mode_to_info(link_mode, &new_link);
> > +	if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
> > +		return B_FALSE;
> >   
> > -	new_link_u64 = *(uint64_t *)&new_link;
> > -	do {
> > -		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
> > -		if (old_link_u64 == new_link_u64)
> > -			break;
> > -
> > -		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > -					old_link_u64, new_link_u64)) {
> > -			evq->sa->port.lsc_seq++;
> > -			break;
> > -		}
> > -	} while (B_TRUE);
> > -
> > -	return B_FALSE;
> > +	evq->sa->port.lsc_seq++;
> > +	return B_TRUE;  
> 
> It still returns B_TRUE, but should return B_FALSE as before.
> Also before the patch lsc_seq is incremented in the case of any
> changes in link status, but now in the case of up/down change only.

The old code looked broken and did not match the comments.
It always returned B_FALSE independent of whether link status changed
or not.

Re: [dpdk-dev] [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions

[Andrew Rybchenko]

On 01/09/2018 07:27 PM, Stephen Hemminger wrote:
> On Tue, 9 Jan 2018 13:35:58 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
>>> Use the new API (_rte_eth_linkstatus_set) to handle link status update.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>>    drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
>>>    drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
>>>    2 files changed, 11 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
>>> index 2f5f86f84877..e0a12b32b1a3 100644
>>> --- a/drivers/net/sfc/sfc_ethdev.c
>>> +++ b/drivers/net/sfc/sfc_ethdev.c
>>> @@ -238,22 +238,12 @@ static int
>>>    sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>>>    {
>>>    	struct sfc_adapter *sa = dev->data->dev_private;
>>> -	struct rte_eth_link *dev_link = &dev->data->dev_link;
>>> -	struct rte_eth_link old_link;
>>>    	struct rte_eth_link current_link;
>>>    
>>>    	sfc_log_init(sa, "entry");
>>>    
>>> -retry:
>>> -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
>>> -	*(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
>>> -
>>>    	if (sa->state != SFC_ADAPTER_STARTED) {
>>>    		sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, &current_link);
>>> -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>>> -					 *(uint64_t *)&old_link,
>>> -					 *(uint64_t *)&current_link))
>>> -			goto retry;
>>>    	} else if (wait_to_complete) {
>>>    		efx_link_mode_t link_mode;
>>>    
>>> @@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>>>    			link_mode = EFX_LINK_UNKNOWN;
>>>    		sfc_port_link_mode_to_info(link_mode, &current_link);
>>>    
>>> -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>>> -					 *(uint64_t *)&old_link,
>>> -					 *(uint64_t *)&current_link))
>>> -			goto retry;
>>>    	} else {
>>>    		sfc_ev_mgmt_qpoll(sa);
>>> -		*(int64_t *)&current_link =
>>> -			rte_atomic64_read((rte_atomic64_t *)dev_link);
>>> +		_rte_eth_linkstatus_get(dev, &current_link);
>>>    	}
>>>    
>>> -	if (old_link.link_status != current_link.link_status)
>>> -		sfc_info(sa, "Link status is %s",
>>> -			 current_link.link_status ? "UP" : "DOWN");
>>> +	if (_rte_eth_linkstatus_set(dev, &current_link) == 0)
>>> +		return 0;
>>> +
>>> +	sfc_info(sa, "Link status is %s",
>>> +		 current_link.link_status ? "UP" : "DOWN");
>>>    
>>> -	return old_link.link_status == current_link.link_status ? 0 : -1;
>>> +	return -1;
>>>    }
>>>    
>>>    static void
>>> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
>>> index a16dc27b380e..3e96536a9d60 100644
>>> --- a/drivers/net/sfc/sfc_ev.c
>>> +++ b/drivers/net/sfc/sfc_ev.c
>>> @@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
>>>    {
>>>    	struct sfc_evq *evq = arg;
>>>    	struct sfc_adapter *sa = evq->sa;
>>> -	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
>>>    	struct rte_eth_link new_link;
>>> -	uint64_t new_link_u64;
>>> -	uint64_t old_link_u64;
>>> -
>>> -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
>>>    
>>>    	sfc_port_link_mode_to_info(link_mode, &new_link);
>>> +	if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
>>> +		return B_FALSE;
>>>    
>>> -	new_link_u64 = *(uint64_t *)&new_link;
>>> -	do {
>>> -		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
>>> -		if (old_link_u64 == new_link_u64)
>>> -			break;
>>> -
>>> -		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>>> -					old_link_u64, new_link_u64)) {
>>> -			evq->sa->port.lsc_seq++;
>>> -			break;
>>> -		}
>>> -	} while (B_TRUE);
>>> -
>>> -	return B_FALSE;
>>> +	evq->sa->port.lsc_seq++;
>>> +	return B_TRUE;
>> It still returns B_TRUE, but should return B_FALSE as before.
>> Also before the patch lsc_seq is incremented in the case of any
>> changes in link status, but now in the case of up/down change only.
> The old code looked broken and did not match the comments.
> It always returned B_FALSE independent of whether link status changed
> or not.

Which comments does it not match?
Yes, because it is internal callback and return value is unrelated to link
status changes. Return value affects further events processing.
If B_TRUE is returned, it means something bad has happened and
events processing should be aborted.

[dpdk-dev] [PATCH v3 10/15] net/i40e: use rte_eth_linkstatus functions

[Stephen Hemminger]

Use new rte_linkstatus update API

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/i40e/i40e_ethdev.c    | 44 ++++++---------------------------------
 drivers/net/i40e/i40e_ethdev_vf.c | 19 ++---------------
 2 files changed, 8 insertions(+), 55 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 285d92b3e7df..75affed2936f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -627,34 +627,6 @@ static struct rte_pci_driver rte_i40e_pmd = {
 	.remove = eth_i40e_pci_remove,
 };
 
-static inline int
-rte_i40e_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				     struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-static inline int
-rte_i40e_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				      struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 RTE_PMD_REGISTER_PCI(net_i40e, rte_i40e_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_i40e, pci_id_i40e_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_i40e, "* igb_uio | uio_pci_generic | vfio-pci");
@@ -2327,17 +2299,16 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 #define MAX_REPEAT_TIME 10  /* 1s (10 * 100ms) in total */
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct i40e_link_status link_status;
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int status;
 	unsigned rep_cnt = MAX_REPEAT_TIME;
 	bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
 
 	memset(&link, 0, sizeof(link));
-	memset(&old, 0, sizeof(old));
-	memset(&link_status, 0, sizeof(link_status));
-	rte_i40e_dev_atomic_read_link_status(dev, &old);
 
 	do {
+		memset(&link_status, 0, sizeof(link_status));
+
 		/* Get link status information from hardware */
 		status = i40e_aq_get_link_info(hw, enable_lse,
 						&link_status, NULL);
@@ -2390,13 +2361,10 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 			ETH_LINK_SPEED_FIXED);
 
 out:
-	rte_i40e_dev_atomic_write_link_status(dev, &link);
-	if (link.link_status == old.link_status)
-		return -1;
-
+	status = _rte_eth_linkstatus_set(dev, &link);
 	i40e_notify_all_vfs_link_status(dev);
 
-	return 0;
+	return status;
 }
 
 /* Get all the statistics of a VSI */
@@ -9854,7 +9822,7 @@ i40e_start_timecounters(struct rte_eth_dev *dev)
 	/* Get current link speed. */
 	memset(&link, 0, sizeof(link));
 	i40e_dev_link_update(dev, 1);
-	rte_i40e_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 
 	switch (link.link_speed) {
 	case ETH_SPEED_NUM_40G:
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index b96d77a0caed..44c6c5b62a90 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1034,20 +1034,6 @@ static const struct rte_pci_id pci_id_i40evf_map[] = {
 	{ .vendor_id = 0, /* sentinel */ },
 };
 
-static inline int
-i40evf_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				    struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 /* Disable IRQ0 */
 static inline void
 i40evf_disable_irq0(struct i40e_hw *hw)
@@ -2069,6 +2055,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
 	 * while Linux driver does not
 	 */
 
+	memset(&new_link, 0, sizeof(new_link));
 	/* Linux driver PF host */
 	switch (vf->link_speed) {
 	case I40E_LINK_SPEED_100MB:
@@ -2100,9 +2087,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
 	new_link.link_autoneg =
 		dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED;
 
-	i40evf_dev_atomic_write_link_status(dev, &new_link);
-
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &new_link);
 }
 
 static void
-- 
2.15.1

[dpdk-dev] [PATCH v3 11/15] net/liquidio: use _rte_eth_linkstatus_set

[Stephen Hemminger]

Use the new link update API, and cleanup the logic in the the
link update routine.

Tested-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/liquidio/lio_ethdev.c | 54 ++++++---------------------------------
 1 file changed, 8 insertions(+), 46 deletions(-)

diff --git a/drivers/net/liquidio/lio_ethdev.c b/drivers/net/liquidio/lio_ethdev.c
index 84b8a3288d12..c7181ebc58de 100644
--- a/drivers/net/liquidio/lio_ethdev.c
+++ b/drivers/net/liquidio/lio_ethdev.c
@@ -930,32 +930,6 @@ lio_dev_vlan_filter_set(struct rte_eth_dev *eth_dev, uint16_t vlan_id, int on)
 	return 0;
 }
 
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param eth_dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-lio_dev_atomic_write_link_status(struct rte_eth_dev *eth_dev,
-				 struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &eth_dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static uint64_t
 lio_hweight64(uint64_t w)
 {
@@ -975,23 +949,17 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev,
 		    int wait_to_complete __rte_unused)
 {
 	struct lio_device *lio_dev = LIO_DEV(eth_dev);
-	struct rte_eth_link link, old;
-
-	/* Initialize */
-	link.link_status = ETH_LINK_DOWN;
-	link.link_speed = ETH_SPEED_NUM_NONE;
-	link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	link.link_autoneg = ETH_LINK_AUTONEG;
-	memset(&old, 0, sizeof(old));
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+		.link_speed = ETH_SPEED_NUM_NONE,
+		.link_duplex = ETH_LINK_HALF_DUPLEX,
+		.link_autoneg = ETH_LINK_AUTONEG,
+	};
 
 	/* Return what we found */
 	if (lio_dev->linfo.link.s.link_up == 0) {
 		/* Interface is down */
-		if (lio_dev_atomic_write_link_status(eth_dev, &link))
-			return -1;
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+		return _rte_eth_linkstatus_set(eth_dev, &link);
 	}
 
 	link.link_status = ETH_LINK_UP; /* Interface is up */
@@ -1008,13 +976,7 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev,
 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
 	}
 
-	if (lio_dev_atomic_write_link_status(eth_dev, &link))
-		return -1;
-
-	if (link.link_status == old.link_status)
-		return -1;
-
-	return 0;
+	return _rte_eth_linkstatus_set(eth_dev, &link);
 }
 
 /**
-- 
2.15.1

[dpdk-dev] [PATCH v3 12/15] net/thunderx: use _rte_eth_linkstatus_set

[Stephen Hemminger]

Use new helper function.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/thunderx/nicvf_ethdev.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index d65d3cee727e..e41b379f3c9c 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -43,7 +43,6 @@
 #include <sys/queue.h>
 
 #include <rte_alarm.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
 #include <rte_common.h>
@@ -76,19 +75,6 @@ static void nicvf_dev_stop_cleanup(struct rte_eth_dev *dev, bool cleanup);
 static void nicvf_vf_stop(struct rte_eth_dev *dev, struct nicvf *nic,
 			  bool cleanup);
 
-static inline int
-nicvf_atomic_write_link_status(struct rte_eth_dev *dev,
-			       struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-		*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
 
 static inline void
 nicvf_set_eth_link_status(struct nicvf *nic, struct rte_eth_link *link)
@@ -170,7 +156,8 @@ nicvf_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		memset(&link, 0, sizeof(link));
 		nicvf_set_eth_link_status(nic, &link);
 	}
-	return nicvf_atomic_write_link_status(dev, &link);
+
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.15.1

[dpdk-dev] [PATCH v3 13/15] net/szedata: use _rte_eth_linkstatus_set

[Stephen Hemminger]

Yet another driver which was not returing correct value on
link change.

Since this driver can't be built on x86 could not even
do a compile test.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/szedata2/rte_eth_szedata2.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index 74f151c4ad8e..bb8df1fbd191 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -50,7 +50,6 @@
 #include <rte_memcpy.h>
 #include <rte_kvargs.h>
 #include <rte_dev.h>
-#include <rte_atomic.h>
 
 #include "rte_eth_szedata2.h"
 #include "szedata2_iobuf.h"
@@ -1173,14 +1172,15 @@ static int
 eth_link_update(struct rte_eth_dev *dev,
 		int wait_to_complete __rte_unused)
 {
-	struct rte_eth_link link;
-	struct rte_eth_link *link_ptr = &link;
-	struct rte_eth_link *dev_link = &dev->data->dev_link;
 	struct pmd_internals *internals = (struct pmd_internals *)
 		dev->data->dev_private;
 	const volatile struct szedata2_ibuf *ibuf;
 	uint32_t i;
 	bool link_is_up = false;
+	struct rte_eth_link link = {
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_SPEED_FIXED,
+	};
 
 	switch (get_link_speed(internals)) {
 	case SZEDATA2_LINK_SPEED_10G:
@@ -1197,9 +1197,6 @@ eth_link_update(struct rte_eth_dev *dev,
 		break;
 	}
 
-	/* szedata2 uses only full duplex */
-	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-
 	for (i = 0; i < szedata2_ibuf_count; i++) {
 		ibuf = ibuf_ptr_by_index(internals->pci_rsc, i);
 		/*
@@ -1212,14 +1209,9 @@ eth_link_update(struct rte_eth_dev *dev,
 		}
 	}
 
-	link.link_status = (link_is_up) ? ETH_LINK_UP : ETH_LINK_DOWN;
-
-	link.link_autoneg = ETH_LINK_SPEED_FIXED;
+	link.link_status = link_is_up ? ETH_LINK_UP : ETH_LINK_DOWN;
 
-	rte_atomic64_cmpset((uint64_t *)dev_link, *(uint64_t *)dev_link,
-			*(uint64_t *)link_ptr);
-
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.15.1

[dpdk-dev] [PATCH v3 14/15] net/octeontx: use rte_eth_linkstatus_set

[Stephen Hemminger]

Common function matches this drivers usage.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/octeontx/octeontx_ethdev.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index bd24ec330fce..670cdec0679b 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -490,20 +490,6 @@ octeontx_dev_promisc_disable(struct rte_eth_dev *dev)
 	octeontx_port_promisc_set(nic, 0);
 }
 
-static inline int
-octeontx_atomic_write_link_status(struct rte_eth_dev *dev,
-				  struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-		*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static int
 octeontx_port_link_status(struct octeontx_nic *nic)
 {
@@ -575,7 +561,7 @@ octeontx_dev_link_update(struct rte_eth_dev *dev,
 	link.link_duplex = ETH_LINK_AUTONEG;
 	link.link_autoneg = ETH_LINK_SPEED_AUTONEG;
 
-	return octeontx_atomic_write_link_status(dev, &link);
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.15.1

[dpdk-dev] [PATCH v3 15/15] net/enic: use _rte_eth_linkstatus_set

[Stephen Hemminger]

This driver was not doing atomic update of link status information.
And the return value was different than others.
The hardware also does not do autonegotiation (at least on Linux).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/enic/enic_ethdev.c |  5 ++---
 drivers/net/enic/enic_main.c   | 16 +++++++---------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 669dbf3363fd..fbeb9955d722 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -446,10 +446,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)
 
 	ENICPMD_FUNC_TRACE();
 	enic_disable(enic);
+
 	memset(&link, 0, sizeof(link));
-	rte_atomic64_cmpset((uint64_t *)&eth_dev->data->dev_link,
-		*(uint64_t *)&eth_dev->data->dev_link,
-		*(uint64_t *)&link);
+	_rte_eth_linkstatus_set(eth_dev, &link);
 }
 
 /*
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 8af0ccd3cdcf..f6238d4373a8 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -413,16 +413,14 @@ enic_free_consistent(void *priv,
 int enic_link_update(struct enic *enic)
 {
 	struct rte_eth_dev *eth_dev = enic->rte_dev;
-	int ret;
-	int link_status = 0;
+	struct rte_eth_link link = {
+		.link_status = enic_get_link_status(enic),
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_speed = vnic_dev_port_speed(enic->vdev),
+		.link_autoneg = ETH_LINK_FIXED,
+	};
 
-	link_status = enic_get_link_status(enic);
-	ret = (link_status == enic->link_status);
-	enic->link_status = link_status;
-	eth_dev->data->dev_link.link_status = link_status;
-	eth_dev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	eth_dev->data->dev_link.link_speed = vnic_dev_port_speed(enic->vdev);
-	return ret;
+	return _rte_eth_linkstatus_set(eth_dev, &link);
 }
 
 static void
-- 
2.15.1

[dpdk-dev] [PATCH v4 01/15] eal: introduce atomic exchange operation

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

To handle atomic update of link status (64 bit), every driver
was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right;
therefore make it a EAL routine.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 .../common/include/arch/x86/rte_atomic.h           | 24 +++++++
 .../common/include/arch/x86/rte_atomic_32.h        | 12 ++++
 .../common/include/arch/x86/rte_atomic_64.h        | 12 ++++
 lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++++++
 4 files changed, 126 insertions(+)

diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic.h b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
index 8469f97e1..20d10cc18 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
@@ -59,6 +59,18 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)
 	return res;
 }
 
+static inline uint16_t
+rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val)
+{
+	asm volatile(
+			MPLOCKED
+			"xchgw %0, %1;"
+			: "=r" (val), "=m" (*dst)
+			: "0" (val),  "m" (*dst)
+			: "memory");         /* no-clobber list */
+	return val;
+}
+
 static inline int rte_atomic16_test_and_set(rte_atomic16_t *v)
 {
 	return rte_atomic16_cmpset((volatile uint16_t *)&v->cnt, 0, 1);
@@ -133,6 +145,18 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)
 	return res;
 }
 
+static inline uint32_t
+rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val)
+{
+	asm volatile(
+			MPLOCKED
+			"xchgl %0, %1;"
+			: "=r" (val), "=m" (*dst)
+			: "0" (val),  "m" (*dst)
+			: "memory");         /* no-clobber list */
+	return val;
+}
+
 static inline int rte_atomic32_test_and_set(rte_atomic32_t *v)
 {
 	return rte_atomic32_cmpset((volatile uint32_t *)&v->cnt, 0, 1);
diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h
index fb3abf187..8d711b6f6 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_32.h
@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
 	return res;
 }
 
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dest, uint64_t val)
+{
+	uint64_t old;
+
+	do {
+		old = *dest;
+	} while (rte_atomic64_cmpset(dest, old, val));
+
+	return old;
+}
+
 static inline void
 rte_atomic64_init(rte_atomic64_t *v)
 {
diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
index 1a53a766b..fd2ec9c53 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
@@ -71,6 +71,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
 	return res;
 }
 
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val)
+{
+	asm volatile(
+			MPLOCKED
+			"xchgq %0, %1;"
+			: "=r" (val), "=m" (*dst)
+			: "0" (val),  "m" (*dst)
+			: "memory");         /* no-clobber list */
+	return val;
+}
+
 static inline void
 rte_atomic64_init(rte_atomic64_t *v)
 {
diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h b/lib/librte_eal/common/include/generic/rte_atomic.h
index 16af5ca57..97854df3d 100644
--- a/lib/librte_eal/common/include/generic/rte_atomic.h
+++ b/lib/librte_eal/common/include/generic/rte_atomic.h
@@ -139,6 +139,32 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)
 }
 #endif
 
+/**
+ * Atomic exchange.
+ *
+ * (atomic) equivalent to:
+ *   ret = *dst
+ *   *dst = val;
+ *   return ret;
+ *
+ * @param dst
+ *   The destination location into which the value will be written.
+ * @param val
+ *   The new value.
+ * @return
+ *   The original value at that location
+ */
+static inline uint16_t
+rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val);
+
+#ifdef RTE_FORCE_INTRINSICS
+static inline uint16_t
+rte_atomic16_exchange(volatile uint16_t *dst, uint16_t val)
+{
+	return __atomic_exchange_2(dst, val, __ATOMIC_SEQ_CST);
+}
+#endif
+
 /**
  * The atomic counter structure.
  */
@@ -392,6 +418,32 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)
 }
 #endif
 
+/**
+ * Atomic exchange.
+ *
+ * (atomic) equivalent to:
+ *   ret = *dst
+ *   *dst = val;
+ *   return ret;
+ *
+ * @param dst
+ *   The destination location into which the value will be written.
+ * @param val
+ *   The new value.
+ * @return
+ *   The original value at that location
+ */
+static inline uint32_t
+rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val);
+
+#ifdef RTE_FORCE_INTRINSICS
+static inline uint32_t
+rte_atomic32_exchange(volatile uint32_t *dst, uint32_t val)
+{
+	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST);
+}
+#endif
+
 /**
  * The atomic counter structure.
  */
@@ -644,6 +696,32 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
 }
 #endif
 
+/**
+ * Atomic exchange.
+ *
+ * (atomic) equivalent to:
+ *   ret = *dst
+ *   *dst = val;
+ *   return ret;
+ *
+ * @param dst
+ *   The destination location into which the value will be written.
+ * @param val
+ *   The new value.
+ * @return
+ *   The original value at that location
+ */
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val);
+
+#ifdef RTE_FORCE_INTRINSICS
+static inline uint64_t
+rte_atomic64_exchange(volatile uint64_t *dst, uint64_t val)
+{
+	return __atomic_exchange_4(dst, val, __ATOMIC_SEQ_CST);
+}
+#endif
+
 /**
  * The atomic counter structure.
  */
-- 
2.14.3

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

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

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 +++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev_version.map |  8 ++++++++
 3 files changed, 72 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index b3495992d..4d67248e5 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1500,6 +1500,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;
+}
+
+void
+_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
+			struct rte_eth_link *link)
+{
+	volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link);
+	uint64_t tmp;
+
+	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+	/* can't use rte_atomic64_read because it returns signed int */
+	do {
+		tmp = *src;
+	} while (!rte_atomic64_cmpset(src, tmp, tmp));
+
+	memcpy(link, &tmp, sizeof(tmp));
+}
+
 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 f0eeefe6a..ffeac5aeb 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2220,6 +2220,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
+ *  0 if link state has changed, -1 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.
  *
diff --git a/lib/librte_ether/rte_ethdev_version.map b/lib/librte_ether/rte_ethdev_version.map
index e9681ac8e..5ff629cb9 100644
--- a/lib/librte_ether/rte_ethdev_version.map
+++ b/lib/librte_ether/rte_ethdev_version.map
@@ -198,6 +198,14 @@ DPDK_17.11 {
 
 } DPDK_17.08;
 
+DPDK_18.02 {
+	global:
+
+	_rte_eth_linkstatus_get;
+	_rte_eth_linkstatus_set;
+
+} DPDK_17.11;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.14.3

[dpdk-dev] [PATCH v4 03/15] net/virtio: use eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use the new comon code in ethdev to handle link status update.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_ethdev.c | 63 ++++++--------------------------------
 1 file changed, 10 insertions(+), 53 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ebb968c4b..05da9bc9a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -14,7 +14,6 @@
 #include <rte_string_fns.h>
 #include <rte_memzone.h>
 #include <rte_malloc.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
@@ -774,46 +773,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
 	.mac_addr_set            = virtio_mac_addr_set,
 };
 
-static inline int
-virtio_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-			*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-virtio_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-		struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static void
 virtio_update_stats(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
@@ -1916,8 +1875,10 @@ static void
 virtio_dev_stop(struct rte_eth_dev *dev)
 {
 	struct virtio_hw *hw = dev->data->dev_private;
-	struct rte_eth_link link;
 	struct rte_intr_conf *intr_conf = &dev->data->dev_conf.intr_conf;
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+	};
 
 	PMD_INIT_LOG(DEBUG, "stop");
 
@@ -1925,21 +1886,18 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 		virtio_intr_disable(dev);
 
 	hw->started = 0;
-	memset(&link, 0, sizeof(link));
-	virtio_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
 virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 {
-	struct rte_eth_link link, old;
-	uint16_t status;
 	struct virtio_hw *hw = dev->data->dev_private;
-	memset(&link, 0, sizeof(link));
-	virtio_dev_atomic_read_link_status(dev, &link);
-	old = link;
-	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	link.link_speed  = ETH_SPEED_NUM_10G;
+	uint16_t status;
+	struct rte_eth_link link = {
+		.link_speed = ETH_SPEED_NUM_10G,
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+	};
 
 	if (hw->started == 0) {
 		link.link_status = ETH_LINK_DOWN;
@@ -1960,9 +1918,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 	} else {
 		link.link_status = ETH_LINK_UP;
 	}
-	virtio_dev_atomic_write_link_status(dev, &link);
 
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.14.3

[dpdk-dev] [PATCH v4 04/15] net/vmxnet3: use rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use new _rte_eth_link_update helper.
Also remove no longer necessary include of rte_atomic.h

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 85 +++++++-----------------------------
 1 file changed, 15 insertions(+), 70 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index a34a6f924..5df1b8d51 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_debug.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_memory.h>
 #include <rte_memzone.h>
@@ -160,59 +159,6 @@ gpa_zone_reserve(struct rte_eth_dev *dev, uint32_t size,
 	return rte_memzone_reserve_aligned(z_name, size, socket_id, 0, align);
 }
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-
-static int
-vmxnet3_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				    struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to write to.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static int
-vmxnet3_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				     struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 /*
  * This function is based on vmxnet3_disable_intr()
  */
@@ -817,9 +763,13 @@ vmxnet3_dev_start(struct rte_eth_dev *dev)
 static void
 vmxnet3_dev_stop(struct rte_eth_dev *dev)
 {
-	struct rte_eth_link link;
 	struct vmxnet3_hw *hw = dev->data->dev_private;
-
+	struct rte_eth_link link = {
+		.link_speed = ETH_SPEED_NUM_10G,
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_FIXED,
+		.link_status = ETH_LINK_DOWN,
+	};
 	PMD_INIT_FUNC_TRACE();
 
 	if (hw->adapter_stopped == 1) {
@@ -852,8 +802,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	vmxnet3_dev_clear_queues(dev);
 
 	/* Clear recorded link status */
-	memset(&link, 0, sizeof(link));
-	vmxnet3_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 /*
@@ -1133,25 +1082,21 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev,
 			  __rte_unused int wait_to_complete)
 {
 	struct vmxnet3_hw *hw = dev->data->dev_private;
-	struct rte_eth_link old = { 0 }, link;
+	struct rte_eth_link link = {
+		.link_speed = ETH_SPEED_NUM_10G,
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_AUTONEG,
+		.link_status = ETH_LINK_UP,
+	};
 	uint32_t ret;
 
-	memset(&link, 0, sizeof(link));
-	vmxnet3_dev_atomic_read_link_status(dev, &old);
-
 	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_GET_LINK);
 	ret = VMXNET3_READ_BAR1_REG(hw, VMXNET3_REG_CMD);
 
-	if (ret & 0x1) {
+	if (ret & 0x1)
 		link.link_status = ETH_LINK_UP;
-		link.link_duplex = ETH_LINK_FULL_DUPLEX;
-		link.link_speed = ETH_SPEED_NUM_10G;
-		link.link_autoneg = ETH_LINK_AUTONEG;
-	}
-
-	vmxnet3_dev_atomic_write_link_status(dev, &link);
 
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.14.3

[dpdk-dev] [PATCH v4 05/15] net/dpaa2: use rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use new helper function to update the link status.
As a good side effect this fixes a but because this driver was not
returning correct status (should be -1 in link_status changed).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++-----------------------------------
 1 file changed, 7 insertions(+), 58 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 821c8622e..73d5605c1 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -57,58 +57,6 @@ static int dpaa2_dev_set_link_up(struct rte_eth_dev *dev);
 static int dpaa2_dev_set_link_down(struct rte_eth_dev *dev);
 static int dpaa2_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-dpaa2_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				  struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &dev->data->dev_link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-dpaa2_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				   struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static int
 dpaa2_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
 {
@@ -837,7 +785,7 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 static void
@@ -869,7 +817,7 @@ dpaa2_dev_close(struct rte_eth_dev *dev)
 	}
 
 	memset(&link, 0, sizeof(link));
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 static void
@@ -1327,8 +1275,8 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 		RTE_LOG(ERR, PMD, "dpni is NULL\n");
 		return 0;
 	}
-	memset(&old, 0, sizeof(old));
-	dpaa2_dev_atomic_read_link_status(dev, &old);
+
+	_rte_eth_linkstatus_get(dev, &old);
 
 	ret = dpni_get_link_state(dpni, CMD_PRI_LOW, priv->token, &state);
 	if (ret < 0) {
@@ -1350,13 +1298,14 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 	else
 		link.link_duplex = ETH_LINK_FULL_DUPLEX;
 
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	ret = _rte_eth_linkstatus_set(dev, &link);
 
 	if (link.link_status)
 		PMD_DRV_LOG(INFO, "Port %d Link is Up\n", dev->data->port_id);
 	else
 		PMD_DRV_LOG(INFO, "Port %d Link is Down", dev->data->port_id);
-	return 0;
+
+	return ret;
 }
 
 /**
-- 
2.14.3

Re: [dpdk-dev] [PATCH v4 05/15] net/dpaa2: use rte_eth_linkstatus_set

[Shreyansh Jain]

On Thursday 11 January 2018 10:36 PM, Ferruh Yigit wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> Use new helper function to update the link status.
> As a good side effect this fixes a but because this driver was not
                               ^^^^^^
                           needs rephrasing

> returning correct status (should be -1 in link_status changed).
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++-----------------------------------
>   1 file changed, 7 insertions(+), 58 deletions(-)

Other than the change in commit message highlighted above:

Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>

Thanks.

Re: [dpdk-dev] [PATCH v4 05/15] net/dpaa2: use rte_eth_linkstatus_set

[Shreyansh Jain]

On Tuesday 16 January 2018 03:14 PM, Shreyansh Jain wrote:
> On Thursday 11 January 2018 10:36 PM, Ferruh Yigit wrote:
>> From: Stephen Hemminger <stephen@networkplumber.org>
>>
>> Use new helper function to update the link status.
>> As a good side effect this fixes a but because this driver was not
>                                ^^^^^^
>                            needs rephrasing
> 
>> returning correct status (should be -1 in link_status changed).
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>>   drivers/net/dpaa2/dpaa2_ethdev.c | 65 
>> +++++-----------------------------------
>>   1 file changed, 7 insertions(+), 58 deletions(-)
> 
> Other than the change in commit message highlighted above:
> 
> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> 
> Thanks.
> 

Also, I forgot to add in previous email, I am assuming 
_rte_eth_linkstatus_get would be rte_eth_linkstatus_get eventually.

I concur with your comments in [1] - it certainly would be better to 
have either @internal or _rte - and not both.

http://dpdk.org/ml/archives/dev/2018-January/086742.html

[dpdk-dev] [PATCH v4 06/15] net/nfp: use rte_eth_linkstatus functions

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use new _rte_eth_linkstatus_get/set helper function.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/nfp/nfp_net.c | 75 +++++------------------------------------------
 1 file changed, 8 insertions(+), 67 deletions(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index d14f2442b..e882f1126 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -213,57 +213,6 @@ nn_cfg_writeq(struct nfp_net_hw *hw, int off, uint64_t val)
 	nn_writeq(rte_cpu_to_le_64(val), hw->ctrl_bar + off);
 }
 
-/*
- * Atomically reads link status information from global structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-nfp_net_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				    struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &dev->data->dev_link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/*
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-nfp_net_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				     struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static void
 nfp_net_rx_queue_release_mbufs(struct nfp_net_rxq *rxq)
 {
@@ -999,7 +948,7 @@ static int
 nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 {
 	struct nfp_net_hw *hw;
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	uint32_t nn_link_status;
 
 	static const uint32_t ls_to_ethtool[] = {
@@ -1017,9 +966,6 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	memset(&old, 0, sizeof(old));
-	nfp_net_dev_atomic_read_link_status(dev, &old);
-
 	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
 
 	memset(&link, 0, sizeof(struct rte_eth_link));
@@ -1037,15 +983,13 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 	else
 		link.link_speed = ls_to_ethtool[nn_link_status];
 
-	if (old.link_status != link.link_status) {
-		nfp_net_dev_atomic_write_link_status(dev, &link);
-		if (link.link_status)
-			PMD_DRV_LOG(INFO, "NIC Link is Up\n");
-		else
-			PMD_DRV_LOG(INFO, "NIC Link is Down\n");
+	if (_rte_eth_linkstatus_set(dev, &link) == 0)
 		return 0;
-	}
 
+	if (link.link_status)
+		PMD_DRV_LOG(INFO, "NIC Link is Up\n");
+	else
+		PMD_DRV_LOG(INFO, "NIC Link is Down\n");
 	return -1;
 }
 
@@ -1376,8 +1320,7 @@ nfp_net_dev_link_status_print(struct rte_eth_dev *dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_eth_link link;
 
-	memset(&link, 0, sizeof(link));
-	nfp_net_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 	if (link.link_status)
 		RTE_LOG(INFO, PMD, "Port %d: Link Up - speed %u Mbps - %s\n",
 			dev->data->port_id, link.link_speed,
@@ -1430,9 +1373,7 @@ nfp_net_dev_interrupt_handler(void *param)
 
 	PMD_DRV_LOG(DEBUG, "We got a LSC interrupt!!!\n");
 
-	/* get the link status */
-	memset(&link, 0, sizeof(link));
-	nfp_net_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 
 	nfp_net_link_update(dev, 0);
 
-- 
2.14.3

[dpdk-dev] [PATCH v4 07/15] net/e1000: use rte_eth_linkstatus helpers

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use new rte_eth_linkstatus_get/set API.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/e1000/em_ethdev.c  | 70 +++--------------------------------------
 drivers/net/e1000/igb_ethdev.c | 71 +++---------------------------------------
 2 files changed, 10 insertions(+), 131 deletions(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 274432305..2aa9b7eac 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_ethdev_pci.h>
 #include <rte_memory.h>
 #include <rte_eal.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
 
@@ -196,57 +195,6 @@ static const struct eth_dev_ops eth_em_ops = {
 	.txq_info_get         = em_txq_info_get,
 };
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_em_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_em_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
 
 /**
  *  eth_em_dev_is_ich8 - Check for ICH8 device
@@ -772,7 +720,7 @@ eth_em_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -1130,7 +1078,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int link_check, count;
 
 	link_check = 0;
@@ -1165,8 +1113,6 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_read_link_status(dev, &link);
-	old = link;
 
 	/* Now we check if a transition has happened */
 	if (link_check && (link.link_status == ETH_LINK_DOWN)) {
@@ -1185,14 +1131,8 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_DOWN;
 		link.link_autoneg = ETH_LINK_FIXED;
 	}
-	rte_em_dev_atomic_write_link_status(dev, &link);
-
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
 
-	/* changed */
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 /*
@@ -1602,8 +1542,8 @@ eth_em_interrupt_action(struct rte_eth_dev *dev,
 	if (ret < 0)
 		return 0;
 
-	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
+
 	if (link.link_status) {
 		PMD_INIT_LOG(INFO, " Port %d: Link Up - speed %u Mbps - %s",
 			     dev->data->port_id, link.link_speed,
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 52dc40706..1a1560e60 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_ethdev_pci.h>
 #include <rte_memory.h>
 #include <rte_eal.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
 
@@ -522,57 +521,6 @@ static const struct rte_igb_xstats_name_off rte_igbvf_stats_strings[] = {
 #define IGBVF_NB_XSTATS (sizeof(rte_igbvf_stats_strings) / \
 		sizeof(rte_igbvf_stats_strings[0]))
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_igb_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_igb_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
 
 static inline void
 igb_intr_enable(struct rte_eth_dev *dev)
@@ -1529,7 +1477,7 @@ eth_igb_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -1605,7 +1553,7 @@ eth_igb_close(struct rte_eth_dev *dev)
 	}
 
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
@@ -2352,7 +2300,7 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int link_check, count;
 
 	link_check = 0;
@@ -2393,8 +2341,6 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		rte_delay_ms(IGB_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_read_link_status(dev, &link);
-	old = link;
 
 	/* Now we check if a transition has happened */
 	if (link_check) {
@@ -2413,14 +2359,8 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_DOWN;
 		link.link_autoneg = ETH_LINK_FIXED;
 	}
-	rte_igb_dev_atomic_write_link_status(dev, &link);
 
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
-
-	/* changed */
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 /*
@@ -2858,8 +2798,7 @@ eth_igb_interrupt_action(struct rte_eth_dev *dev,
 		if (ret < 0)
 			return 0;
 
-		memset(&link, 0, sizeof(link));
-		rte_igb_dev_atomic_read_link_status(dev, &link);
+		_rte_eth_linkstatus_get(dev, &link);
 		if (link.link_status) {
 			PMD_INIT_LOG(INFO,
 				     " Port %d: Link Up - speed %u Mbps - %s",
-- 
2.14.3

[dpdk-dev] [PATCH v4 08/15] net/ixgbe: use rte_eth_linkstatus functions

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use the new helper functions from eth_dev for
handling atomic link_info update.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 99 +++++++---------------------------------
 1 file changed, 17 insertions(+), 82 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 2fb9d5266..d62824506 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -20,7 +20,6 @@
 #include <rte_debug.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_memory.h>
 #include <rte_eal.h>
@@ -787,58 +786,6 @@ static const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[] = {
 #define IXGBEVF_NB_XSTATS (sizeof(rte_ixgbevf_stats_strings) /	\
 		sizeof(rte_ixgbevf_stats_strings[0]))
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_ixgbe_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_ixgbe_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 /*
  * This function is the same as ixgbe_is_sfp() in base/ixgbe.h.
  */
@@ -2760,7 +2707,7 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	/* Clear recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_ixgbe_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_linkstatus_set(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -3945,7 +3892,6 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 			    int wait_to_complete, int vf)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
 	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
@@ -3954,13 +3900,11 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	u32 speed = 0;
 	int wait = 1;
 	bool autoneg = false;
-
-	link.link_status = ETH_LINK_DOWN;
-	link.link_speed = 0;
-	link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	link.link_autoneg = ETH_LINK_AUTONEG;
-	memset(&old, 0, sizeof(old));
-	rte_ixgbe_dev_atomic_read_link_status(dev, &old);
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+		.link_duplex = ETH_LINK_HALF_DUPLEX,
+		.link_autoneg = ETH_LINK_AUTONEG,
+	};
 
 	hw->mac.get_link_status = true;
 
@@ -3984,19 +3928,15 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	if (diag != 0) {
 		link.link_speed = ETH_SPEED_NUM_100M;
 		link.link_duplex = ETH_LINK_FULL_DUPLEX;
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+
+		return _rte_eth_linkstatus_set(dev, &link);
 	}
 
 	if (link_up == 0) {
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
 		intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+		return _rte_eth_linkstatus_set(dev, &link);
 	}
+
 	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 	link.link_status = ETH_LINK_UP;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
@@ -4028,12 +3968,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		link.link_speed = ETH_SPEED_NUM_10G;
 		break;
 	}
-	rte_ixgbe_dev_atomic_write_link_status(dev, &link);
 
-	if (link.link_status == old.link_status)
-		return -1;
-
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
@@ -4232,8 +4168,8 @@ ixgbe_dev_link_status_print(struct rte_eth_dev *dev)
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	struct rte_eth_link link;
 
-	memset(&link, 0, sizeof(link));
-	rte_ixgbe_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
+
 	if (link.link_status) {
 		PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s",
 					(int)(dev->data->port_id),
@@ -4268,7 +4204,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	int64_t timeout;
-	struct rte_eth_link link;
 	struct ixgbe_hw *hw =
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
@@ -4285,9 +4220,10 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 	}
 
 	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE) {
+		struct rte_eth_link link;
+
 		/* get the link status before link update, for predicting later */
-		memset(&link, 0, sizeof(link));
-		rte_ixgbe_dev_atomic_read_link_status(dev, &link);
+		_rte_eth_linkstatus_get(dev, &link);
 
 		ixgbe_dev_link_update(dev, 0);
 
@@ -6806,9 +6742,8 @@ ixgbe_start_timecounters(struct rte_eth_dev *dev)
 	uint32_t shift = 0;
 
 	/* Get current link speed. */
-	memset(&link, 0, sizeof(link));
 	ixgbe_dev_link_update(dev, 1);
-	rte_ixgbe_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 
 	switch (link.link_speed) {
 	case ETH_SPEED_NUM_100M:
-- 
2.14.3

[dpdk-dev] [PATCH v4 09/15] net/sfc: use new rte_eth_linkstatus functions

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use the new API (_rte_eth_linkstatus_set) to handle link status update.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
 drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
 2 files changed, 11 insertions(+), 39 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index af867a7d1..3a23c44b7 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -232,22 +232,12 @@ static int
 sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct sfc_adapter *sa = dev->data->dev_private;
-	struct rte_eth_link *dev_link = &dev->data->dev_link;
-	struct rte_eth_link old_link;
 	struct rte_eth_link current_link;
 
 	sfc_log_init(sa, "entry");
 
-retry:
-	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
-	*(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
-
 	if (sa->state != SFC_ADAPTER_STARTED) {
 		sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, &current_link);
-		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					 *(uint64_t *)&old_link,
-					 *(uint64_t *)&current_link))
-			goto retry;
 	} else if (wait_to_complete) {
 		efx_link_mode_t link_mode;
 
@@ -255,21 +245,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 			link_mode = EFX_LINK_UNKNOWN;
 		sfc_port_link_mode_to_info(link_mode, &current_link);
 
-		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					 *(uint64_t *)&old_link,
-					 *(uint64_t *)&current_link))
-			goto retry;
 	} else {
 		sfc_ev_mgmt_qpoll(sa);
-		*(int64_t *)&current_link =
-			rte_atomic64_read((rte_atomic64_t *)dev_link);
+		_rte_eth_linkstatus_get(dev, &current_link);
 	}
 
-	if (old_link.link_status != current_link.link_status)
-		sfc_info(sa, "Link status is %s",
-			 current_link.link_status ? "UP" : "DOWN");
+	if (_rte_eth_linkstatus_set(dev, &current_link) == 0)
+		return 0;
+
+	sfc_info(sa, "Link status is %s",
+		 current_link.link_status ? "UP" : "DOWN");
 
-	return old_link.link_status == current_link.link_status ? 0 : -1;
+	return -1;
 }
 
 static void
diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index 7abe61ae5..242b1fae4 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -382,29 +382,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
 {
 	struct sfc_evq *evq = arg;
 	struct sfc_adapter *sa = evq->sa;
-	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
 	struct rte_eth_link new_link;
-	uint64_t new_link_u64;
-	uint64_t old_link_u64;
-
-	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
 
 	sfc_port_link_mode_to_info(link_mode, &new_link);
+	if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
+		return B_FALSE;
 
-	new_link_u64 = *(uint64_t *)&new_link;
-	do {
-		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
-		if (old_link_u64 == new_link_u64)
-			break;
-
-		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					old_link_u64, new_link_u64)) {
-			evq->sa->port.lsc_seq++;
-			break;
-		}
-	} while (B_TRUE);
-
-	return B_FALSE;
+	evq->sa->port.lsc_seq++;
+	return B_TRUE;
 }
 
 static const efx_ev_callbacks_t sfc_ev_callbacks = {
-- 
2.14.3

Re: [dpdk-dev] [PATCH v4 09/15] net/sfc: use new rte_eth_linkstatus functions

[Andrew Rybchenko]

On 01/11/2018 08:06 PM, Ferruh Yigit wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
>
> Use the new API (_rte_eth_linkstatus_set) to handle link status update.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
>   drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
>   2 files changed, 11 insertions(+), 39 deletions(-)

<...>

> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
> index 7abe61ae5..242b1fae4 100644
> --- a/drivers/net/sfc/sfc_ev.c
> +++ b/drivers/net/sfc/sfc_ev.c
> @@ -382,29 +382,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
>   {
>   	struct sfc_evq *evq = arg;
>   	struct sfc_adapter *sa = evq->sa;
> -	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
>   	struct rte_eth_link new_link;
> -	uint64_t new_link_u64;
> -	uint64_t old_link_u64;
> -
> -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
>   
>   	sfc_port_link_mode_to_info(link_mode, &new_link);
> +	if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
> +		return B_FALSE;
>   
> -	new_link_u64 = *(uint64_t *)&new_link;
> -	do {
> -		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
> -		if (old_link_u64 == new_link_u64)
> -			break;
> -
> -		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> -					old_link_u64, new_link_u64)) {
> -			evq->sa->port.lsc_seq++;
> -			break;
> -		}
> -	} while (B_TRUE);
> -
> -	return B_FALSE;
> +	evq->sa->port.lsc_seq++;
> +	return B_TRUE;
>   }
>   
>   static const efx_ev_callbacks_t sfc_ev_callbacks = {

I've provided review notes for the callback a couple of times already,
but it is ignored.
The callback returns B_FALSE before the change and it must be preserved.
The return value must not depend on link status changes.
lsc_seq should be incremented in the case of any changes in link,
not up/down changes only and it should be preserved.

[dpdk-dev] [PATCH v4 10/15] net/i40e: use rte_eth_linkstatus functions

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use new rte_linkstatus update API

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/i40e/i40e_ethdev.c    | 44 ++++++---------------------------------
 drivers/net/i40e/i40e_ethdev_vf.c | 19 ++---------------
 2 files changed, 8 insertions(+), 55 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index fb2b4d8c4..9f32edbac 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -628,34 +628,6 @@ static struct rte_pci_driver rte_i40e_pmd = {
 	.remove = eth_i40e_pci_remove,
 };
 
-static inline int
-rte_i40e_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				     struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-static inline int
-rte_i40e_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				      struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 RTE_PMD_REGISTER_PCI(net_i40e, rte_i40e_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_i40e, pci_id_i40e_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_i40e, "* igb_uio | uio_pci_generic | vfio-pci");
@@ -2345,17 +2317,16 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 #define MAX_REPEAT_TIME 10  /* 1s (10 * 100ms) in total */
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct i40e_link_status link_status;
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int status;
 	unsigned rep_cnt = MAX_REPEAT_TIME;
 	bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
 
 	memset(&link, 0, sizeof(link));
-	memset(&old, 0, sizeof(old));
-	memset(&link_status, 0, sizeof(link_status));
-	rte_i40e_dev_atomic_read_link_status(dev, &old);
 
 	do {
+		memset(&link_status, 0, sizeof(link_status));
+
 		/* Get link status information from hardware */
 		status = i40e_aq_get_link_info(hw, enable_lse,
 						&link_status, NULL);
@@ -2408,13 +2379,10 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 			ETH_LINK_SPEED_FIXED);
 
 out:
-	rte_i40e_dev_atomic_write_link_status(dev, &link);
-	if (link.link_status == old.link_status)
-		return -1;
-
+	status = _rte_eth_linkstatus_set(dev, &link);
 	i40e_notify_all_vfs_link_status(dev);
 
-	return 0;
+	return status;
 }
 
 /* Get all the statistics of a VSI */
@@ -10032,7 +10000,7 @@ i40e_start_timecounters(struct rte_eth_dev *dev)
 	/* Get current link speed. */
 	memset(&link, 0, sizeof(link));
 	i40e_dev_link_update(dev, 1);
-	rte_i40e_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_linkstatus_get(dev, &link);
 
 	switch (link.link_speed) {
 	case ETH_SPEED_NUM_40G:
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 86f94e926..dfda62272 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1034,20 +1034,6 @@ static const struct rte_pci_id pci_id_i40evf_map[] = {
 	{ .vendor_id = 0, /* sentinel */ },
 };
 
-static inline int
-i40evf_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				    struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 /* Disable IRQ0 */
 static inline void
 i40evf_disable_irq0(struct i40e_hw *hw)
@@ -2075,6 +2061,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
 	 * while Linux driver does not
 	 */
 
+	memset(&new_link, 0, sizeof(new_link));
 	/* Linux driver PF host */
 	switch (vf->link_speed) {
 	case I40E_LINK_SPEED_100MB:
@@ -2106,9 +2093,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
 	new_link.link_autoneg =
 		dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED;
 
-	i40evf_dev_atomic_write_link_status(dev, &new_link);
-
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &new_link);
 }
 
 static void
-- 
2.14.3

[dpdk-dev] [PATCH v4 11/15] net/liquidio: use _rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use the new link update API, and cleanup the logic in the the
link update routine.

Tested-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/liquidio/lio_ethdev.c | 54 ++++++---------------------------------
 1 file changed, 8 insertions(+), 46 deletions(-)

diff --git a/drivers/net/liquidio/lio_ethdev.c b/drivers/net/liquidio/lio_ethdev.c
index 8f6ef6381..5e5ce7b5f 100644
--- a/drivers/net/liquidio/lio_ethdev.c
+++ b/drivers/net/liquidio/lio_ethdev.c
@@ -904,32 +904,6 @@ lio_dev_vlan_filter_set(struct rte_eth_dev *eth_dev, uint16_t vlan_id, int on)
 	return 0;
 }
 
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param eth_dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-lio_dev_atomic_write_link_status(struct rte_eth_dev *eth_dev,
-				 struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &eth_dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-				*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static uint64_t
 lio_hweight64(uint64_t w)
 {
@@ -949,23 +923,17 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev,
 		    int wait_to_complete __rte_unused)
 {
 	struct lio_device *lio_dev = LIO_DEV(eth_dev);
-	struct rte_eth_link link, old;
-
-	/* Initialize */
-	link.link_status = ETH_LINK_DOWN;
-	link.link_speed = ETH_SPEED_NUM_NONE;
-	link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	link.link_autoneg = ETH_LINK_AUTONEG;
-	memset(&old, 0, sizeof(old));
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+		.link_speed = ETH_SPEED_NUM_NONE,
+		.link_duplex = ETH_LINK_HALF_DUPLEX,
+		.link_autoneg = ETH_LINK_AUTONEG,
+	};
 
 	/* Return what we found */
 	if (lio_dev->linfo.link.s.link_up == 0) {
 		/* Interface is down */
-		if (lio_dev_atomic_write_link_status(eth_dev, &link))
-			return -1;
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+		return _rte_eth_linkstatus_set(eth_dev, &link);
 	}
 
 	link.link_status = ETH_LINK_UP; /* Interface is up */
@@ -982,13 +950,7 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev,
 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
 	}
 
-	if (lio_dev_atomic_write_link_status(eth_dev, &link))
-		return -1;
-
-	if (link.link_status == old.link_status)
-		return -1;
-
-	return 0;
+	return _rte_eth_linkstatus_set(eth_dev, &link);
 }
 
 /**
-- 
2.14.3

[dpdk-dev] [PATCH v4 12/15] net/thunderx: use _rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Use new helper function.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/thunderx/nicvf_ethdev.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index b9a873f2f..7266b557a 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -15,7 +15,6 @@
 #include <sys/queue.h>
 
 #include <rte_alarm.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
 #include <rte_common.h>
@@ -69,20 +68,6 @@ nicvf_init_log(void)
 		rte_log_set_level(nicvf_logtype_driver, RTE_LOG_NOTICE);
 }
 
-static inline int
-nicvf_atomic_write_link_status(struct rte_eth_dev *dev,
-			       struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-		*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static inline void
 nicvf_set_eth_link_status(struct nicvf *nic, struct rte_eth_link *link)
 {
@@ -163,7 +148,8 @@ nicvf_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		memset(&link, 0, sizeof(link));
 		nicvf_set_eth_link_status(nic, &link);
 	}
-	return nicvf_atomic_write_link_status(dev, &link);
+
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.14.3

[dpdk-dev] [PATCH v4 13/15] net/szedata: use _rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Yet another driver which was not returing correct value on
link change.

Since this driver can't be built on x86 could not even
do a compile test.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/szedata2/rte_eth_szedata2.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index 45aebed33..b0fdf30f8 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -50,7 +50,6 @@
 #include <rte_memcpy.h>
 #include <rte_kvargs.h>
 #include <rte_dev.h>
-#include <rte_atomic.h>
 
 #include "rte_eth_szedata2.h"
 #include "szedata2_iobuf.h"
@@ -1173,14 +1172,15 @@ static int
 eth_link_update(struct rte_eth_dev *dev,
 		int wait_to_complete __rte_unused)
 {
-	struct rte_eth_link link;
-	struct rte_eth_link *link_ptr = &link;
-	struct rte_eth_link *dev_link = &dev->data->dev_link;
 	struct pmd_internals *internals = (struct pmd_internals *)
 		dev->data->dev_private;
 	const volatile struct szedata2_ibuf *ibuf;
 	uint32_t i;
 	bool link_is_up = false;
+	struct rte_eth_link link = {
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_autoneg = ETH_LINK_FIXED,
+	};
 
 	switch (get_link_speed(internals)) {
 	case SZEDATA2_LINK_SPEED_10G:
@@ -1197,9 +1197,6 @@ eth_link_update(struct rte_eth_dev *dev,
 		break;
 	}
 
-	/* szedata2 uses only full duplex */
-	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-
 	for (i = 0; i < szedata2_ibuf_count; i++) {
 		ibuf = ibuf_ptr_by_index(internals->pci_rsc, i);
 		/*
@@ -1212,14 +1209,9 @@ eth_link_update(struct rte_eth_dev *dev,
 		}
 	}
 
-	link.link_status = (link_is_up) ? ETH_LINK_UP : ETH_LINK_DOWN;
-
-	link.link_autoneg = ETH_LINK_FIXED;
+	link.link_status = link_is_up ? ETH_LINK_UP : ETH_LINK_DOWN;
 
-	rte_atomic64_cmpset((uint64_t *)dev_link, *(uint64_t *)dev_link,
-			*(uint64_t *)link_ptr);
-
-	return 0;
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.14.3

[dpdk-dev] [PATCH v4 14/15] net/octeontx: use rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

Common function matches this drivers usage.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/octeontx/octeontx_ethdev.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index adca3435e..d00750fa5 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -487,20 +487,6 @@ octeontx_dev_promisc_disable(struct rte_eth_dev *dev)
 	octeontx_port_promisc_set(nic, 0);
 }
 
-static inline int
-octeontx_atomic_write_link_status(struct rte_eth_dev *dev,
-				  struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &dev->data->dev_link;
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-		*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
 static int
 octeontx_port_link_status(struct octeontx_nic *nic)
 {
@@ -572,7 +558,7 @@ octeontx_dev_link_update(struct rte_eth_dev *dev,
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	link.link_autoneg = ETH_LINK_AUTONEG;
 
-	return octeontx_atomic_write_link_status(dev, &link);
+	return _rte_eth_linkstatus_set(dev, &link);
 }
 
 static int
-- 
2.14.3

[dpdk-dev] [PATCH v4 15/15] net/enic: use _rte_eth_linkstatus_set

[Ferruh Yigit]

From: Stephen Hemminger <stephen@networkplumber.org>

This driver was not doing atomic update of link status information.
And the return value was different than others.
The hardware also does not do autonegotiation (at least on Linux).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/enic/enic_ethdev.c |  5 ++---
 drivers/net/enic/enic_main.c   | 16 +++++++---------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index ad7a4e306..08b990dc2 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -426,10 +426,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)
 
 	ENICPMD_FUNC_TRACE();
 	enic_disable(enic);
+
 	memset(&link, 0, sizeof(link));
-	rte_atomic64_cmpset((uint64_t *)&eth_dev->data->dev_link,
-		*(uint64_t *)&eth_dev->data->dev_link,
-		*(uint64_t *)&link);
+	_rte_eth_linkstatus_set(eth_dev, &link);
 }
 
 /*
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9bbe054b3..e2a75de66 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -379,16 +379,14 @@ enic_free_consistent(void *priv,
 int enic_link_update(struct enic *enic)
 {
 	struct rte_eth_dev *eth_dev = enic->rte_dev;
-	int ret;
-	int link_status = 0;
+	struct rte_eth_link link = {
+		.link_status = enic_get_link_status(enic),
+		.link_duplex = ETH_LINK_FULL_DUPLEX,
+		.link_speed = vnic_dev_port_speed(enic->vdev),
+		.link_autoneg = ETH_LINK_FIXED,
+	};
 
-	link_status = enic_get_link_status(enic);
-	ret = (link_status == enic->link_status);
-	enic->link_status = link_status;
-	eth_dev->data->dev_link.link_status = link_status;
-	eth_dev->data->dev_link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	eth_dev->data->dev_link.link_speed = vnic_dev_port_speed(enic->vdev);
-	return ret;
+	return _rte_eth_linkstatus_set(eth_dev, &link);
 }
 
 static void
-- 
2.14.3

Re: [dpdk-dev] [PATCH v4 15/15] net/enic: use _rte_eth_linkstatus_set

[Hyong Youb Kim]

On Thu, Jan 11, 2018 at 05:06:58PM +0000, Ferruh Yigit wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> This driver was not doing atomic update of link status information.
> And the return value was different than others.
> The hardware also does not do autonegotiation (at least on Linux).
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Here is an ack, in case you need it. Please feel free to add it to v4 if you
make it (i.e. dropping the leading _).

Acked-by: Hyong Youb Kim <hyonkim@cisco.com>

FWIW, the NIC hardware (Cisco VIC) does autonegotiate. But, the link
settings (autoneg/fixed/10/25/40) are controlled by a management
entity (e.g. UCS manager). The drivers including the netdev enic
driver cannot change them and currently do not know if the current
speed has been autonegotiated or fixed by admin. So, they simply
report the current link speed as fixed. I know, it is a little
unconventional.

-Hyong