common ethdev linkstatus functions

PATCH Superseded Stephen Hemminger 5 participants 45 messages Jan 16, 2018 – Jan 22, 2018

Subsystems (2)

Network drivers VMware VMXNET3 Ethernet driver

lore

Share & tools

lore

Stephen Hemminger

author

2018-01-16 18:38 UTC #0 lore raw

v5 cover common ethdev linkstatus functions

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.

Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.

v5
- checkpatch whitespace cleanup

v4
- incorporate review feedback
- rename _rte_linkstatus to rte_linkstatus
- change return value of _rte_linkstatus
- optimize get on 64bit platforms
- change return value of rte_linkstatus_set

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                   | 75 ++---------------
 drivers/net/e1000/em_ethdev.c                      | 69 ++--------------
 drivers/net/e1000/igb_ethdev.c                     | 70 ++--------------
 drivers/net/enic/enic_ethdev.c                     |  5 +-
 drivers/net/enic/enic_main.c                       | 17 ++--
 drivers/net/i40e/i40e_ethdev.c                     | 43 ++--------
 drivers/net/i40e/i40e_ethdev_vf.c                  | 18 +---
 drivers/net/ixgbe/ixgbe_ethdev.c                   | 96 ++++------------------
 drivers/net/liquidio/lio_ethdev.c                  | 53 ++----------
 drivers/net/nfp/nfp_net.c                          | 77 ++---------------
 drivers/net/octeontx/octeontx_ethdev.c             | 17 +---
 drivers/net/sfc/sfc_ethdev.c                       | 21 +----
 drivers/net/sfc/sfc_ev.c                           | 20 +----
 drivers/net/szedata2/rte_eth_szedata2.c            | 19 ++---
 drivers/net/thunderx/nicvf_ethdev.c                | 18 +---
 drivers/net/virtio/virtio_ethdev.c                 | 65 +++------------
 drivers/net/vmxnet3/vmxnet3_ethdev.c               | 86 ++++---------------
 .../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                      | 22 +----
 lib/librte_ether/rte_ethdev.h                      | 62 ++++++++++++++
 23 files changed, 302 insertions(+), 677 deletions(-)

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #1 lore raw

PATCH v5 1/15 eal: introduce atomic exchange operation

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 <[email protected]>

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

lib/librte_eal/common/include/arch/x86/rte_atomic.h +24 -0

index 8469f97e193a..20d10cc18e4e 100644
@@ -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);

lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +12 -0

index fb3abf187998..43fa59355ac5 100644
@@ -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)
 {

lib/librte_eal/common/include/arch/x86/rte_atomic_64.h +12 -0

index 1a53a766bd72..fd2ec9c53796 100644
@@ -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)
 {

lib/librte_eal/common/include/generic/rte_atomic.h +78 -0

96 lines not on this page · raw message

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #2 lore raw

PATCH v5 2/15 ethdev: add linkstatus get/set helper functions

In reply to 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 <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 22 +++------------
 lib/librte_ether/rte_ethdev.h | 62 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 18 deletions(-)

lib/librte_ether/rte_ethdev.c +4 ~~-18~~

index a524af740f4a..d6433af35a19 100644
@@ -1447,20 +1447,6 @@ rte_eth_allmulticast_get(uint16_t port_id)
 	return dev->data->all_multicast;
 }
 
-static inline int
-rte_eth_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;
-}
-
 void
 rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
 {

@@ -1469,8 +1455,8 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0)
-		rte_eth_dev_atomic_read_link_status(dev, eth_link);
+	if (dev->data->dev_conf.intr_conf.lsc)
+		rte_eth_linkstatus_get(dev, eth_link);
 	else {
 		RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
 		(*dev->dev_ops->link_update)(dev, 1);

@@ -1486,8 +1472,8 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0)
-		rte_eth_dev_atomic_read_link_status(dev, eth_link);
+	if (dev->data->dev_conf.intr_conf.lsc)
+		rte_eth_linkstatus_get(dev, eth_link);
 	else {
 		RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
 		(*dev->dev_ops->link_update)(dev, 0);

lib/librte_ether/rte_ethdev.h +62 -0

68 lines not on this page · raw message

Signature · 1 line

2.15.1

Andrew Rybchenko

2018-01-17 07:49 UTC #3 lore raw

Re: [PATCH v5 02/15] ethdev: add linkstatus get/set helper functions

Quoted 69 lines · 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 <[email protected]>

---
  lib/librte_ether/rte_ethdev.c | 22 +++------------
  lib/librte_ether/rte_ethdev.h | 62 +++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 66 insertions(+), 18 deletions(-)

lib/librte_ether/rte_ethdev.c +4 ~~-18~~

index a524af740f4a..d6433af35a19 100644
@@ -1447,20 +1447,6 @@ rte_eth_allmulticast_get(uint16_t port_id)
  	return dev->data->all_multicast;
  }
 
-static inline int
-rte_eth_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;
-}
-
  void
  rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
  {

@@ -1469,8 +1455,8 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
  	RTE_ETH_VALID_PORTID_OR_RET(port_id);
  	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0)
-		rte_eth_dev_atomic_read_link_status(dev, eth_link);
+	if (dev->data->dev_conf.intr_conf.lsc)
+		rte_eth_linkstatus_get(dev, eth_link);
  	else {
  		RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
  		(*dev->dev_ops->link_update)(dev, 1);

@@ -1486,8 +1472,8 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
  	RTE_ETH_VALID_PORTID_OR_RET(port_id);
  	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0)
-		rte_eth_dev_atomic_read_link_status(dev, eth_link);
+	if (dev->data->dev_conf.intr_conf.lsc)
+		rte_eth_linkstatus_get(dev, eth_link);
  	else {
  		RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
  		(*dev->dev_ops->link_update)(dev, 0);

lib/librte_ether/rte_ethdev.h +4 -0

index b10e2a92da71..ec8255656967 100644
@@ -2218,6 +2218,68 @@ 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.

lib/librte_ether/rte_ethdev.h int rte_eth_dev_set_link_down(uint16_t port_id); · line 2234

Quoted 10 lines

lib/librte_ether/rte_ethdev.h +10 -0

+ * 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 status has changed;
+ *  0 if link status is unchanged.

Here "link status" is used as up/down status of the link.

lib/librte_ether/rte_ethdev.h int rte_eth_dev_set_link_down(uint16_t port_id); · line 2238

Quoted 4 lines

lib/librte_ether/rte_ethdev.h +4 -0

+ */
+static inline int
+rte_eth_linkstatus_set(struct rte_eth_dev *dev,
+		       const struct rte_eth_link *new_link)

Here (and function and the second parameter description) "link status" is used as all information about link.
rte_ethdev API calls it just "link" (link_update, link_get_wait/nowait). It is not a big problem, but it is better to avoid it.

Thanks.

Quoted 47 lines

lib/librte_ether/rte_ethdev.h +44 -0

+{
+	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;
+}
+
+/**
+ * @internal
+ * Atomically get the link speed and status.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param link
+ *  link status value.
+ */
+static inline 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 *dst = (uint64_t *)link;
+
+	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+	/* can't use rte_atomic64_read because it returns signed int */
+#ifdef __LP64__
+	*dst = *src;
+#else
+	do {
+		*dst = *src;
+	} while (!rte_atomic64_cmpset(src, *dst, *dst));
+#endif
+}
+
  /**
   * Reset a Ethernet device and keep its port id.
   *

Stephen Hemminger

author

2018-01-16 18:38 UTC #4 lore raw

PATCH v5 3/15 net/virtio: use eth_linkstatus_set

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c | 65 +++++++-------------------------------
 1 file changed, 12 insertions(+), 53 deletions(-)

drivers/net/virtio/virtio_ethdev.c +12 ~~-53~~

index 21f2131a9efd..7eb97977e73c 100644
@@ -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,9 @@ 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);
+	rte_eth_linkstatus_set(dev, &link);
 
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return 0;
 }
 
 static int

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #5 lore raw

PATCH v5 4/15 net/vmxnet3: use rte_eth_linkstatus_set

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 86 +++++++-----------------------------
 1 file changed, 16 insertions(+), 70 deletions(-)

drivers/net/vmxnet3/vmxnet3_ethdev.c +16 ~~-70~~

index c2f12cdf2a4d..761731a304e0 100644
@@ -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,22 @@ __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;
+	rte_eth_linkstatus_set(dev, &link);
+	return 0;
 }
 
 static int

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #6 lore raw

PATCH v5 5/15 net/dpaa2: use rte_eth_linkstatus_set

In reply to Stephen Hemminger

Use new helper function to update the link status.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/dpaa2/dpaa2_ethdev.c | 75 ++++------------------------------------
 1 file changed, 7 insertions(+), 68 deletions(-)

drivers/net/dpaa2/dpaa2_ethdev.c +7 ~~-68~~

index 2f99d95d2216..c2aed380e408 100644
@@ -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

@@ -1284,15 +1232,13 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 	int ret;
 	struct dpaa2_dev_priv *priv = dev->data->dev_private;
 	struct fsl_mc_io *dpni = (struct fsl_mc_io *)priv->hw;
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	struct dpni_link_state state = {0};
 
 	if (dpni == NULL) {
 		RTE_LOG(ERR, PMD, "dpni is NULL\n");
 		return 0;
 	}
-	memset(&old, 0, sizeof(old));
-	dpaa2_dev_atomic_read_link_status(dev, &old);
 
 	ret = dpni_get_link_state(dpni, CMD_PRI_LOW, priv->token, &state);
 	if (ret < 0) {

@@ -1300,11 +1246,6 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 		return -1;
 	}
 
-	if ((old.link_status == state.up) && (old.link_speed == state.rate)) {
-		RTE_LOG(DEBUG, PMD, "No change in status\n");
-		return -1;
-	}
-
 	memset(&link, 0, sizeof(struct rte_eth_link));
 	link.link_status = state.up;
 	link.link_speed = state.rate;

@@ -1314,12 +1255,10 @@ 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);
-
-	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);
+	if (rte_eth_linkstatus_set(dev, &link)) {
+		PMD_DRV_LOG(INFO, "Port %d Link is %s\n", dev->data->port_id,
+			    link.link_status ? "Up" : "Down");
+	}
 	return 0;
 }

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #7 lore raw

PATCH v5 6/15 net/nfp: use rte_eth_linkstatus functions

In reply to Stephen Hemminger

Use new rte_eth_linkstatus_get/set helper function.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/nfp/nfp_net.c | 77 +++++------------------------------------------
 1 file changed, 7 insertions(+), 70 deletions(-)

drivers/net/nfp/nfp_net.c +7 ~~-70~~

index 0501156bacb7..80a676dacf60 100644
@@ -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,16 +961,10 @@ 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");
-		return 0;
-	}
-
-	return -1;
+	if (rte_eth_linkstatus_set(dev, &link))
+		PMD_DRV_LOG(INFO, "NIC Link is %s\n",
+			    link.link_status ? "Up" : "Down");
+	return 0;
 }
 
 static int

@@ -1354,8 +1294,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 +1347,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);

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #8 lore raw

PATCH v5 7/15 net/e1000: use rte_eth_linkstatus helpers

In reply to Stephen Hemminger

Use new rte_eth_linkstatus_get/set API.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/e1000/em_ethdev.c  | 69 +++--------------------------------------
 drivers/net/e1000/igb_ethdev.c | 70 +++---------------------------------------
 2 files changed, 10 insertions(+), 129 deletions(-)

drivers/net/e1000/em_ethdev.c +5 ~~-64~~

index f457be55e706..1042ca7b1daf 100644
@@ -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,13 +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);
+	rte_eth_linkstatus_set(dev, &link);
 
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
-
-	/* changed */
 	return 0;
 }

@@ -1599,8 +1540,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,

drivers/net/e1000/igb_ethdev.c +5 ~~-65~~

119 lines not on this page · raw message

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #9 lore raw

PATCH v5 8/15 net/ixgbe: use rte_eth_linkstatus functions

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/ixgbe/ixgbe_ethdev.c | 96 +++++++---------------------------------
 1 file changed, 17 insertions(+), 79 deletions(-)

drivers/net/ixgbe/ixgbe_ethdev.c +17 ~~-79~~

index 43e0132f81fe..c2af00f17e21 100644
@@ -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,17 @@ 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;
+		rte_eth_linkstatus_set(dev, &link);
+
 		return 0;
 	}
 
 	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;
+		rte_eth_linkstatus_set(dev, &link);
 		return 0;
 	}
+
 	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 	link.link_status = ETH_LINK_UP;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;

@@ -4021,11 +3963,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;
 
+	rte_eth_linkstatus_set(dev, &link);
 	return 0;
 }

@@ -4225,8 +4164,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 +4200,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 +4216,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 +6738,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:

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #10 lore raw

PATCH v5 9/15 net/sfc: use new rte_eth_linkstatus functions

In reply to Stephen Hemminger

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/sfc/sfc_ethdev.c | 21 +++------------------
 drivers/net/sfc/sfc_ev.c     | 20 ++------------------
 2 files changed, 5 insertions(+), 36 deletions(-)

drivers/net/sfc/sfc_ethdev.c +3 ~~-18~~

index 2f5f86f84877..7b16c51c578c 100644
@@ -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,16 @@ 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)
+	if (rte_eth_linkstatus_set(dev, &current_link))
 		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 0;
 }
 
 static void

drivers/net/sfc/sfc_ev.c +2 ~~-18~~

index a16dc27b380e..80c249f7526c 100644
@@ -404,27 +404,11 @@ 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);
-
-	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);
+	if (rte_eth_linkstatus_set(sa->eth_dev, &new_link))
+		evq->sa->port.lsc_seq++;
 
 	return B_FALSE;
 }

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #11 lore raw

PATCH v5 10/15 net/i40e: use rte_eth_linkstatus functions

In reply to Stephen Hemminger

Use new rte_linkstatus update API

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/i40e/i40e_ethdev.c    | 43 +++++----------------------------------
 drivers/net/i40e/i40e_ethdev_vf.c | 18 ++--------------
 2 files changed, 7 insertions(+), 54 deletions(-)

drivers/net/i40e/i40e_ethdev.c +5 ~~-38~~

index fc386154f0b9..517918624658 100644
@@ -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,10 +2361,7 @@ 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;
-
+	rte_eth_linkstatus_set(dev, &link);
 	i40e_notify_all_vfs_link_status(dev);
 
 	return 0;

@@ -9854,9 +9822,8 @@ i40e_start_timecounters(struct rte_eth_dev *dev)
 	uint32_t tsync_inc_h;
 
 	/* 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:

drivers/net/i40e/i40e_ethdev_vf.c +2 ~~-16~~

index b96d77a0caed..0605fc107240 100644
@@ -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,8 +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);
-
+	rte_eth_linkstatus_set(dev, &new_link);
 	return 0;
 }

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #12 lore raw

PATCH v5 11/15 net/liquidio: use rte_eth_linkstatus_set

In reply to Stephen Hemminger

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

Tested-by Shijith Thotton <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/liquidio/lio_ethdev.c | 53 ++++++---------------------------------
 1 file changed, 8 insertions(+), 45 deletions(-)

drivers/net/liquidio/lio_ethdev.c +8 ~~-45~~

index 5b50c9334103..cdb21a896f6c 100644
@@ -901,32 +901,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)
 {

@@ -946,23 +920,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 */

@@ -979,12 +947,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;
-
+	rte_eth_linkstatus_set(eth_dev, &link);
 	return 0;
 }

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #13 lore raw

PATCH v5 12/15 net/thunderx: use rte_eth_linkstatus_set

In reply to Stephen Hemminger

Use new helper function.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/thunderx/nicvf_ethdev.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

drivers/net/thunderx/nicvf_ethdev.c +3 ~~-15~~

index 72dc8ae24ba9..94a2a9b7683d 100644
@@ -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>

@@ -48,19 +47,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)

@@ -142,7 +128,9 @@ 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);
+
+	rte_eth_linkstatus_set(dev, &link);
+	return 0;
 }
 
 static int

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #14 lore raw

PATCH v5 13/15 net/szedata: use _rte_eth_linkstatus_set

In reply to 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 <[email protected]>

---
 drivers/net/szedata2/rte_eth_szedata2.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

drivers/net/szedata2/rte_eth_szedata2.c +6 ~~-13~~

index 74f151c4ad8e..c902e29d8ff2 100644
@@ -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,13 +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;
-
-	rte_atomic64_cmpset((uint64_t *)dev_link, *(uint64_t *)dev_link,
-			*(uint64_t *)link_ptr);
+	link.link_status = link_is_up ? ETH_LINK_UP : ETH_LINK_DOWN;
 
+	rte_eth_linkstatus_set(dev, &link);
 	return 0;
 }

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #15 lore raw

PATCH v5 14/15 net/octeontx: use rte_eth_linkstatus_set

In reply to Stephen Hemminger

Common function matches this drivers usage.

Signed-off-by Stephen Hemminger <[email protected]>

---
 drivers/net/octeontx/octeontx_ethdev.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

drivers/net/octeontx/octeontx_ethdev.c +2 ~~-15~~

index 4f71f9def7e9..fd03d22646f4 100644
@@ -463,20 +463,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)
 {

@@ -548,7 +534,8 @@ 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);
+	rte_eth_linkstatus_set(dev, &link);
+	return 0;
 }
 
 static int

Signature · 1 line

2.15.1

Stephen Hemminger

author

2018-01-16 18:38 UTC #16 lore raw

PATCH v5 15/15 net/enic: use rte_eth_linkstatus_set

In reply to 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 <[email protected]>

---
 drivers/net/enic/enic_ethdev.c |  5 ++---
 drivers/net/enic/enic_main.c   | 17 ++++++++---------
 2 files changed, 10 insertions(+), 12 deletions(-)

drivers/net/enic/enic_ethdev.c +2 -3

index 669dbf3363fd..68b625468b63 100644
@@ -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);
 }
 
 /*

drivers/net/enic/enic_main.c +8 -9

index 8af0ccd3cdcf..7674263c38cc 100644
@@ -413,16 +413,15 @@ 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;
+	rte_eth_linkstatus_set(eth_dev, &link);
+	return 0;
 }
 
 static void

Signature · 1 line

2.15.1

Shreyansh Jain

2018-01-17 05:04 UTC #17 lore raw

In reply to Stephen Hemminger

Quoted 25 lines · 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.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.
v5
- checkpatch whitespace cleanup
v4
- incorporate review feedback
- rename _rte_linkstatus to rte_linkstatus
- change return value of _rte_linkstatus
- optimize get on 64bit platforms
- change return value of rte_linkstatus_set

Quoted 9 lines

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

[...]

1. "--in-reply-to" is recommended way ([1])

2. Specifically here: there is a v4 already by Ferruh (targeting maintainers directly, I think) and then another by you. And with separate threads it is difficult to compare them. Maybe you prefer flat email threads - it helps people like me who don't.

3. I had put my Ack on v4 by Ferruh, but it seems dpaa2 specific code has changed from Ferruh's v4 to your v4/v5. Please ignore that Ack. I will send my acceptance on your v5.

[1] http://dpdk.org/dev

Andrew Rybchenko

2018-01-17 07:56 UTC #18 lore raw

In reply to Stephen Hemminger

Quoted 9 lines · 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).

Quoted 5 lines

Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.

I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now.

Quoted 62 lines

v5
- checkpatch whitespace cleanup
v4
- incorporate review feedback
- rename _rte_linkstatus to rte_linkstatus
- change return value of _rte_linkstatus
- optimize get on 64bit platforms
- change return value of rte_linkstatus_set
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                   | 75 ++---------------
  drivers/net/e1000/em_ethdev.c                      | 69 ++--------------
  drivers/net/e1000/igb_ethdev.c                     | 70 ++--------------
  drivers/net/enic/enic_ethdev.c                     |  5 +-
  drivers/net/enic/enic_main.c                       | 17 ++--
  drivers/net/i40e/i40e_ethdev.c                     | 43 ++--------
  drivers/net/i40e/i40e_ethdev_vf.c                  | 18 +---
  drivers/net/ixgbe/ixgbe_ethdev.c                   | 96 ++++------------------
  drivers/net/liquidio/lio_ethdev.c                  | 53 ++----------
  drivers/net/nfp/nfp_net.c                          | 77 ++---------------
  drivers/net/octeontx/octeontx_ethdev.c             | 17 +---
  drivers/net/sfc/sfc_ethdev.c                       | 21 +----
  drivers/net/sfc/sfc_ev.c                           | 20 +----
  drivers/net/szedata2/rte_eth_szedata2.c            | 19 ++---
  drivers/net/thunderx/nicvf_ethdev.c                | 18 +---
  drivers/net/virtio/virtio_ethdev.c                 | 65 +++------------
  drivers/net/vmxnet3/vmxnet3_ethdev.c               | 86 ++++---------------
  .../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                      | 22 +----
  lib/librte_ether/rte_ethdev.h                      | 62 ++++++++++++++
  23 files changed, 302 insertions(+), 677 deletions(-)

Ferruh Yigit

2018-01-17 14:32 UTC #19 lore raw

Quoted 16 lines · Andrew Rybchenko

Quoted 9 lines · 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).
Quoted 5 lines
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.

Quoted 7 lines

I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now.

Again, why not return previous link status, it is simple enough to prevent inconsistent usage.

rte_eth_link_get() already discards the return value, so won't be a problem there.

For the cases PMD would like know about link changes, they will need to implement almost same link_update function with a return value, so why not use existing link_update function?

Like been in virtio, link_update() used in interrupt handler, and calls a callback process if status changes. When link_update() return status changed to void, I guess they will need to implement another version of the link_update with return and use it.

Quoted 64 lines

Quoted 63 lines
v5
- checkpatch whitespace cleanup
v4
- incorporate review feedback
- rename _rte_linkstatus to rte_linkstatus
- change return value of _rte_linkstatus
- optimize get on 64bit platforms
- change return value of rte_linkstatus_set
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                   | 75 ++---------------
  drivers/net/e1000/em_ethdev.c                      | 69 ++--------------
  drivers/net/e1000/igb_ethdev.c                     | 70 ++--------------
  drivers/net/enic/enic_ethdev.c                     |  5 +-
  drivers/net/enic/enic_main.c                       | 17 ++--
  drivers/net/i40e/i40e_ethdev.c                     | 43 ++--------
  drivers/net/i40e/i40e_ethdev_vf.c                  | 18 +---
  drivers/net/ixgbe/ixgbe_ethdev.c                   | 96 ++++------------------
  drivers/net/liquidio/lio_ethdev.c                  | 53 ++----------
  drivers/net/nfp/nfp_net.c                          | 77 ++---------------
  drivers/net/octeontx/octeontx_ethdev.c             | 17 +---
  drivers/net/sfc/sfc_ethdev.c                       | 21 +----
  drivers/net/sfc/sfc_ev.c                           | 20 +----
  drivers/net/szedata2/rte_eth_szedata2.c            | 19 ++---
  drivers/net/thunderx/nicvf_ethdev.c                | 18 +---
  drivers/net/virtio/virtio_ethdev.c                 | 65 +++------------
  drivers/net/vmxnet3/vmxnet3_ethdev.c               | 86 ++++---------------
  .../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                      | 22 +----
  lib/librte_ether/rte_ethdev.h                      | 62 ++++++++++++++
  23 files changed, 302 insertions(+), 677 deletions(-)

Stephen Hemminger

author

2018-01-17 16:05 UTC #20 lore raw

Quoted 30 lines · On Wed, 17 Jan 2018 14:32:17 +0000 Ferruh Yigit <[email protected]> wrote:

Quoted 16 lines · Andrew Rybchenko
Quoted 9 lines · 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).
Quoted 5 lines
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.
Quoted 7 lines
I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now.
Again, why not return previous link status, it is simple enough to prevent inconsistent usage.
rte_eth_link_get() already discards the return value, so won't be a problem there.

Quoted 8 lines

For the cases PMD would like know about link changes, they will need to implement almost same link_update function with a return value, so why not use existing link_update function?
Like been in virtio, link_update() used in interrupt handler, and calls a callback process if status changes. When link_update() return status changed to void, I guess they will need to implement another version of the link_update with return and use it.

The interrupt and non-interrupt model are different.
Also the driver internally may want to do something different, this is about the return value for dev_ops->link_update. The code in rte_eth_dev never used the return value. The bonding driver was expecting it to work but it doesn't. Anyway drivers shouldn't in general be directly calling other devices eth_dev_ops

Ferruh Yigit

2018-01-17 16:18 UTC #21 lore raw

Quoted 37 lines · Stephen Hemminger

Quoted 34 lines · On Wed, 17 Jan 2018 14:32:17 +0000 Ferruh Yigit <[email protected]> wrote:
Quoted 16 lines · Andrew Rybchenko
Quoted 9 lines · 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).
Quoted 5 lines
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.
Quoted 7 lines
I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now.
Again, why not return previous link status, it is simple enough to prevent inconsistent usage.
rte_eth_link_get() already discards the return value, so won't be a problem there.
For the cases PMD would like know about link changes, they will need to implement almost same link_update function with a return value, so why not use existing link_update function?

Quoted 6 lines

Quoted 4 lines
Like been in virtio, link_update() used in interrupt handler, and calls a callback process if status changes. When link_update() return status changed to void, I guess they will need to implement another version of the link_update with return and use it.
The interrupt and non-interrupt model are different.

Yes. But for virtio specific usage:

virtio_interrupt_handler()
virtio_dev_link_update() == 0
_rte_eth_dev_callback_process()

meantime same exact virtio_dev_link_update() used as:
.link_update = virtio_dev_link_update,

so updating virtio_dev_link_update() to not return status change, will update logic in virtio_interrupt_handler(), no?

Quoted 2 lines

Also the driver internally may want to do something different, this is about the return value for dev_ops->link_update.

Agreed, driver may do something different. And the function needs to be implemented will be very close to dev_ops->link_update. I thought making dev_ops->link_update more generic can prevent duplication there. And aligns with virtio usage..

Quoted 3 lines

The code in rte_eth_dev never
used the return value. The bonding driver was expecting it to work but it doesn't.

Agreed.

Quoted 2 lines

Anyway drivers shouldn't in general be directly calling other devices eth_dev_ops

I guess now there are a few overlay PMDs does this.

Ferruh Yigit

2018-01-19 16:35 UTC #22 lore raw

Quoted 51 lines · Ferruh Yigit

Quoted 37 lines · Stephen Hemminger
Quoted 34 lines · On Wed, 17 Jan 2018 14:32:17 +0000 Ferruh Yigit <[email protected]> wrote:
Quoted 16 lines · Andrew Rybchenko
Quoted 9 lines · 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).
Quoted 5 lines
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.
Quoted 7 lines
I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now.
Again, why not return previous link status, it is simple enough to prevent inconsistent usage.
rte_eth_link_get() already discards the return value, so won't be a problem there.
For the cases PMD would like know about link changes, they will need to implement almost same link_update function with a return value, so why not use existing link_update function?
Quoted 6 lines
Quoted 4 lines
Like been in virtio, link_update() used in interrupt handler, and calls a callback process if status changes. When link_update() return status changed to void, I guess they will need to implement another version of the link_update with return and use it.
The interrupt and non-interrupt model are different.
Yes. But for virtio specific usage:
virtio_interrupt_handler()
virtio_dev_link_update() == 0
_rte_eth_dev_callback_process()

Quoted 5 lines

meantime same exact virtio_dev_link_update() used as:
.link_update = virtio_dev_link_update,
so updating virtio_dev_link_update() to not return status change, will update logic in virtio_interrupt_handler(), no?

I would like to see this patch in, because it is useful and almost done. The concern I mentioned above effects virtio.

Can virtio maintainers check if it is OK to get this as it is please?

Quoted 20 lines

Quoted 3 lines
Also the driver internally may want to do something different, this is about the return value for dev_ops->link_update.
Agreed, driver may do something different. And the function needs to be implemented will be very close to dev_ops->link_update. I thought making dev_ops->link_update more generic can prevent duplication there. And aligns with virtio usage..
Quoted 3 lines
The code in rte_eth_dev never
used the return value. The bonding driver was expecting it to work but it doesn't.
Agreed.
Quoted 2 lines
Anyway drivers shouldn't in general be directly calling other devices eth_dev_ops
I guess now there are a few overlay PMDs does this.

Ferruh Yigit

2018-01-21 18:35 UTC #23 lore raw

Quoted 52 lines · Ferruh Yigit

Quoted 51 lines · Ferruh Yigit
Quoted 37 lines · Stephen Hemminger
Quoted 34 lines · On Wed, 17 Jan 2018 14:32:17 +0000 Ferruh Yigit <[email protected]> wrote:
Quoted 16 lines · Andrew Rybchenko
Quoted 9 lines · 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).
Quoted 5 lines
Mostly only compile tested only, don't have all of the hardware available (except ixgbe and virtio) to test.
Note: the eth_dev_link_update function return value is inconsistent across drivers. Should be changed to be void.
Quoted 7 lines
I would say "link_update" callback return value is inconsistent across drivers. I'm not sure which direction is right here: make it consistent or make it void. Also any changes in link information could be important. As I understand it should not happen without up/down, but bugs with loss of intermediate transitions are definitely possible. So, notifying about any changes in link information is definitely safer. May be not now.
Again, why not return previous link status, it is simple enough to prevent inconsistent usage.
rte_eth_link_get() already discards the return value, so won't be a problem there.
For the cases PMD would like know about link changes, they will need to implement almost same link_update function with a return value, so why not use existing link_update function?
Quoted 6 lines
Quoted 4 lines
Like been in virtio, link_update() used in interrupt handler, and calls a callback process if status changes. When link_update() return status changed to void, I guess they will need to implement another version of the link_update with return and use it.
The interrupt and non-interrupt model are different.
Yes. But for virtio specific usage:
virtio_interrupt_handler()
virtio_dev_link_update() == 0
_rte_eth_dev_callback_process()

Quoted 8 lines

Quoted 5 lines
meantime same exact virtio_dev_link_update() used as:
.link_update = virtio_dev_link_update,
so updating virtio_dev_link_update() to not return status change, will update logic in virtio_interrupt_handler(), no?
I would like to see this patch in, because it is useful and almost done. The concern I mentioned above effects virtio.

Let's go step by step. I will update patchset to keep same behavior in drivers but switch to new APIs to prevent code duplication.

In next step we can define the return value and implement missing PMDs.

Quoted 24 lines

Can virtio maintainers check if it is OK to get this as it is please?
Quoted 20 lines
Quoted 3 lines
Also the driver internally may want to do something different, this is about the return value for dev_ops->link_update.
Agreed, driver may do something different. And the function needs to be implemented will be very close to dev_ops->link_update. I thought making dev_ops->link_update more generic can prevent duplication there. And aligns with virtio usage..
Quoted 3 lines
The code in rte_eth_dev never
used the return value. The bonding driver was expecting it to work but it doesn't.
Agreed.
Quoted 2 lines
Anyway drivers shouldn't in general be directly calling other devices eth_dev_ops
I guess now there are a few overlay PMDs does this.

Ferruh Yigit

2018-01-21 19:00 UTC #24 lore raw

PATCH v6 1/14 eal: introduce atomic exchange operation

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>

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 <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
v6:
*fix build error caused by rte_atomic64_t_cmpset

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

lib/librte_eal/common/include/arch/x86/rte_atomic.h +24 -0

index 36cfabc38..55bfc3903 100644
@@ -60,6 +60,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);

@@ -134,6 +146,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);

lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +12 -0

index fb3abf187..8d711b6f6 100644
@@ -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)
 {

lib/librte_eal/common/include/arch/x86/rte_atomic_64.h +12 -0

index 1a53a766b..fd2ec9c53 100644
@@ -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)
 {

lib/librte_eal/common/include/generic/rte_atomic.h +78 -0

96 lines not on this page · raw message

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #25 lore raw

PATCH v6 2/14 ethdev: add linkstatus get/set helper functions

From: Stephen Hemminger <[email protected]>

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 <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 22 +++------------
 lib/librte_ether/rte_ethdev.h | 62 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 18 deletions(-)

lib/librte_ether/rte_ethdev.c +4 ~~-18~~

index 8a365393d..1d6f159e8 100644
@@ -1540,20 +1540,6 @@ rte_eth_allmulticast_get(uint16_t port_id)
 	return dev->data->all_multicast;
 }
 
-static inline int
-rte_eth_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;
-}
-
 void
 rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
 {

@@ -1562,8 +1548,8 @@ rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0)
-		rte_eth_dev_atomic_read_link_status(dev, eth_link);
+	if (dev->data->dev_conf.intr_conf.lsc)
+		rte_eth_linkstatus_get(dev, eth_link);
 	else {
 		RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
 		(*dev->dev_ops->link_update)(dev, 1);

@@ -1579,8 +1565,8 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0)
-		rte_eth_dev_atomic_read_link_status(dev, eth_link);
+	if (dev->data->dev_conf.intr_conf.lsc)
+		rte_eth_linkstatus_get(dev, eth_link);
 	else {
 		RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
 		(*dev->dev_ops->link_update)(dev, 0);

lib/librte_ether/rte_ethdev.h +62 -0

68 lines not on this page · raw message

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #26 lore raw

PATCH v6 3/14 net/virtio: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

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

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/virtio/virtio_ethdev.c | 53 +++++---------------------------------
 1 file changed, 6 insertions(+), 47 deletions(-)

drivers/net/virtio/virtio_ethdev.c +6 ~~-47~~

index 17ac04931..dcbbfa90c 100644
@@ -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>

@@ -785,46 +784,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)
 {

@@ -2023,8 +1982,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");

@@ -2033,8 +1994,7 @@ 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);
 	rte_spinlock_unlock(&hw->state_lock);
 }

@@ -2044,8 +2004,7 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 	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);
+	rte_eth_linkstatus_get(dev, &link);
 	old = link;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
 	link.link_speed  = ETH_SPEED_NUM_10G;

@@ -2069,7 +2028,7 @@ 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);
+	rte_eth_linkstatus_set(dev, &link);
 
 	return (old.link_status == link.link_status) ? -1 : 0;
 }

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #27 lore raw

PATCH v6 4/14 net/vmxnet3: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

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

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 80 +++++++-----------------------------
 1 file changed, 14 insertions(+), 66 deletions(-)

drivers/net/vmxnet3/vmxnet3_ethdev.c +14 ~~-66~~

index d3b704b40..fc495388e 100644
@@ -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,8 +763,10 @@ 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_status = ETH_LINK_DOWN,
+	};
 
 	PMD_INIT_FUNC_TRACE();

@@ -852,8 +800,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);
 }
 
 /*

@@ -1132,23 +1079,24 @@ __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 old;
+	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_DOWN,
+	};
 	uint32_t ret;
 
-	memset(&link, 0, sizeof(link));
-	vmxnet3_dev_atomic_read_link_status(dev, &old);
+	rte_eth_linkstatus_get(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);
+	rte_eth_linkstatus_set(dev, &link);
 
 	return (old.link_status == link.link_status) ? -1 : 0;
 }

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #28 lore raw

PATCH v6 5/14 net/dpaa2: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

Use new helper function to update the link status.

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Use correct APIs
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/dpaa2/dpaa2_ethdev.c | 77 +++++++---------------------------------
 1 file changed, 12 insertions(+), 65 deletions(-)

drivers/net/dpaa2/dpaa2_ethdev.c +12 ~~-65~~

index a0a3d5264..9a3a4182b 100644
@@ -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)
 {

@@ -821,7 +769,9 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
 	struct dpaa2_dev_priv *priv = dev->data->dev_private;
 	struct fsl_mc_io *dpni = (struct fsl_mc_io *)priv->hw;
 	int ret;
-	struct rte_eth_link link;
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+	};
 	struct rte_intr_handle *intr_handle = dev->intr_handle;
 
 	PMD_INIT_FUNC_TRACE();

@@ -851,8 +801,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

@@ -862,7 +811,9 @@ dpaa2_dev_close(struct rte_eth_dev *dev)
 	struct dpaa2_dev_priv *priv = dev->data->dev_private;
 	struct fsl_mc_io *dpni = (struct fsl_mc_io *)priv->hw;
 	int i, ret;
-	struct rte_eth_link link;
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+	};
 	struct dpaa2_queue *dpaa2_q;
 
 	PMD_INIT_FUNC_TRACE();

@@ -883,8 +834,7 @@ dpaa2_dev_close(struct rte_eth_dev *dev)
 		return;
 	}
 
-	memset(&link, 0, sizeof(link));
-	dpaa2_dev_atomic_write_link_status(dev, &link);
+	rte_eth_linkstatus_set(dev, &link);
 }
 
 static void

@@ -1342,8 +1292,7 @@ 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) {

@@ -1365,12 +1314,10 @@ 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);
+	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);
+	PMD_DRV_LOG(INFO, "Port %d Link is %s\n", dev->data->port_id,
+		    link.link_status ? "Up" : "Down");
 	return 0;
 }

Signature · 1 line

2.14.3

Shreyansh Jain

Acked-by on v6 5/14

2018-01-22 13:24 UTC #29 lore raw

Re: [PATCH v6 05/14] net/dpaa2: use ethdev linkstatus helper functions

Quoted 6 lines · Ferruh Yigit

From: Stephen Hemminger <[email protected]>
Use new helper function to update the link status.
Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by Ferruh Yigit <[email protected]>

Quoted 8 lines

---
v6:
* Use correct APIs
* Keep logic exact same, only use new APIs to get/set link
---
  drivers/net/dpaa2/dpaa2_ethdev.c | 77 +++++++---------------------------------
  1 file changed, 12 insertions(+), 65 deletions(-)

Acked-by Shreyansh Jain <[email protected]>

Ferruh Yigit

2018-01-21 19:00 UTC #30 lore raw

PATCH v6 6/14 net/nfp: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

Use new rte_eth_linkstatus_get/set helper function.

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/nfp/nfp_net.c | 70 +++++------------------------------------------
 1 file changed, 7 insertions(+), 63 deletions(-)

drivers/net/nfp/nfp_net.c +7 ~~-63~~

index d14f2442b..5b886528b 100644
@@ -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)
 {

@@ -1017,8 +966,7 @@ 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);
+	rte_eth_linkstatus_get(dev, &old);
 
 	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);

@@ -1038,11 +986,10 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 		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");
+		rte_eth_linkstatus_set(dev, &link);
+
+		PMD_DRV_LOG(INFO, "NIC Link is %s\n",
+			    link.link_status ? "Up" : "Down");
 		return 0;
 	}

@@ -1376,8 +1323,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 +1376,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);

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #31 lore raw

PATCH v6 7/14 net/e1000: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

Use new rte_eth_linkstatus_get/set API.

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/e1000/em_ethdev.c  | 63 ++++-------------------------------------
 drivers/net/e1000/igb_ethdev.c | 64 ++++--------------------------------------
 2 files changed, 10 insertions(+), 117 deletions(-)

drivers/net/e1000/em_ethdev.c +5 ~~-58~~

index dcffb0797..8bedaf77d 100644
@@ -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>

@@ -197,57 +196,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

@@ -802,7 +750,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 */

@@ -1194,8 +1142,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 			break;
 		rte_delay_ms(EM_LINK_UPDATE_CHECK_INTERVAL);
 	}
-	memset(&link, 0, sizeof(link));
-	rte_em_dev_atomic_read_link_status(dev, &link);
+	rte_eth_linkstatus_get(dev, &link);
 	old = link;
 
 	/* Now we check if a transition has happened */

@@ -1215,7 +1162,7 @@ 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);
+	rte_eth_linkstatus_set(dev, &link);
 
 	/* not changed */
 	if (old.link_status == link.link_status)

@@ -1631,8 +1578,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,

drivers/net/e1000/igb_ethdev.c +5 ~~-59~~

106 lines not on this page · raw message

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #32 lore raw

PATCH v6 8/14 net/ixgbe: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

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

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/ixgbe/ixgbe_ethdev.c | 87 ++++++++--------------------------------
 1 file changed, 16 insertions(+), 71 deletions(-)

drivers/net/ixgbe/ixgbe_ethdev.c +16 ~~-71~~

index 4f4334df2..2d520446c 100644
@@ -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.
  */

@@ -2758,7 +2705,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 */

@@ -3943,7 +3890,7 @@ 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;
+	struct rte_eth_link old;
 	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);

@@ -3952,13 +3899,13 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	u32 speed = 0;
 	int wait = 1;
 	bool autoneg = false;
+	struct rte_eth_link link = {
+		.link_status = ETH_LINK_DOWN,
+		.link_duplex = ETH_LINK_HALF_DUPLEX,
+		.link_autoneg = ETH_LINK_AUTONEG,
+	};
 
-	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);
+	rte_eth_linkstatus_get(dev, &old);
 
 	hw->mac.get_link_status = true;

@@ -3982,14 +3929,14 @@ 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);
+		rte_eth_linkstatus_set(dev, &link);
 		if (link.link_status == old.link_status)
 			return -1;
 		return 0;
 	}
 
 	if (link_up == 0) {
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
+		rte_eth_linkstatus_set(dev, &link);
 		intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
 		if (link.link_status == old.link_status)
 			return -1;

@@ -4026,7 +3973,7 @@ 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);
+	rte_eth_linkstatus_set(dev, &link);
 
 	if (link.link_status == old.link_status)
 		return -1;

@@ -4230,8 +4177,7 @@ 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),

@@ -4266,7 +4212,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);

@@ -4283,9 +4228,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);

@@ -6804,9 +6750,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:

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #33 lore raw

PATCH v6 9/14 net/i40e: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

Use new rte_linkstatus update API

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Ferruh Yigit <[email protected]>

---
v6:
* Keep logic exact same, only use new APIs to get/set link

---
 drivers/net/i40e/i40e_ethdev.c    | 39 +++++----------------------------------
 drivers/net/i40e/i40e_ethdev_vf.c | 19 +++----------------
 2 files changed, 8 insertions(+), 50 deletions(-)

drivers/net/i40e/i40e_ethdev.c +5 ~~-34~~

index 5ea9f9917..0b102c0de 100644
@@ -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");

@@ -2353,11 +2325,11 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 	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);
+	rte_eth_linkstatus_get(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);

@@ -2410,7 +2382,7 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 			ETH_LINK_SPEED_FIXED);
 
 out:
-	rte_i40e_dev_atomic_write_link_status(dev, &link);
+	rte_eth_linkstatus_set(dev, &link);
 	if (link.link_status == old.link_status)
 		return -1;

@@ -10034,9 +10006,8 @@ i40e_start_timecounters(struct rte_eth_dev *dev)
 	uint32_t tsync_inc_h;
 
 	/* 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:

drivers/net/i40e/i40e_ethdev_vf.c +3 ~~-16~~

index 0cca0d324..8e61fa84f 100644
@@ -1036,20 +1036,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)

@@ -2077,6 +2063,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,9 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
 	new_link.link_status = vf->link_up ? ETH_LINK_UP :
 					     ETH_LINK_DOWN;
 	new_link.link_autoneg =
-		dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_FIXED;
+		dev->data->dev_conf.link_speeds & ETH_LINK_FIXED;
 
-	i40evf_dev_atomic_write_link_status(dev, &new_link);
+	rte_eth_linkstatus_set(dev, &new_link);
 
 	return 0;
 }

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #34 lore raw

PATCH v6 10/14 net/liquidio: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

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

Signed-off-by Stephen Hemminger <[email protected]>

Tested-by Shijith Thotton <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
 drivers/net/liquidio/lio_ethdev.c | 56 +++++++--------------------------------
 1 file changed, 9 insertions(+), 47 deletions(-)

drivers/net/liquidio/lio_ethdev.c +9 ~~-47~~

index 8f6ef6381..9adbcc12c 100644
@@ -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,24 +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) {
+	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 */
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;

@@ -982,12 +949,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;
-
+	rte_eth_linkstatus_set(eth_dev, &link);
 	return 0;
 }

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #35 lore raw

PATCH v6 11/14 net/thunderx: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

Use new helper function.

Signed-off-by Stephen Hemminger <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
 drivers/net/thunderx/nicvf_ethdev.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

drivers/net/thunderx/nicvf_ethdev.c +3 ~~-16~~

index c42ba30a4..0076c5ae3 100644
@@ -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,9 @@ 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);
+
+	rte_eth_linkstatus_set(dev, &link);
+	return 0;
 }
 
 static int

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #36 lore raw

PATCH v6 12/14 net/szedata2: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

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 <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
 drivers/net/szedata2/rte_eth_szedata2.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

drivers/net/szedata2/rte_eth_szedata2.c +6 ~~-13~~

index 45aebed33..958c5ccb3 100644
@@ -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,13 +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;
-
-	rte_atomic64_cmpset((uint64_t *)dev_link, *(uint64_t *)dev_link,
-			*(uint64_t *)link_ptr);
+	link.link_status = link_is_up ? ETH_LINK_UP : ETH_LINK_DOWN;
 
+	rte_eth_linkstatus_set(dev, &link);
 	return 0;
 }

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #37 lore raw

PATCH v6 13/14 net/octeontx: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

Common function matches this drivers usage.

Signed-off-by Stephen Hemminger <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
 drivers/net/octeontx/octeontx_ethdev.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

drivers/net/octeontx/octeontx_ethdev.c +2 ~~-15~~

index adca3435e..a54ea1d22 100644
@@ -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,8 @@ 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);
+	rte_eth_linkstatus_set(dev, &link);
+	return 0;
 }
 
 static int

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:00 UTC #38 lore raw

PATCH v6 14/14 net/enic: use ethdev linkstatus helper functions

In reply to Ferruh Yigit

From: Stephen Hemminger <[email protected]>

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 <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

---
 drivers/net/enic/enic_ethdev.c |  5 ++---
 drivers/net/enic/enic_main.c   | 17 ++++++++---------
 2 files changed, 10 insertions(+), 12 deletions(-)

drivers/net/enic/enic_ethdev.c +2 -3

index ad7a4e306..d3e98a33f 100644
@@ -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);
 }
 
 /*

drivers/net/enic/enic_main.c +8 -9

index 9bbe054b3..669085156 100644
@@ -379,16 +379,15 @@ 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;
+	rte_eth_linkstatus_set(eth_dev, &link);
+	return 0;
 }
 
 static void

Signature · 1 line

2.14.3

Ferruh Yigit

2018-01-21 19:26 UTC #39 lore raw

Re: [PATCH v6 01/14] eal: introduce atomic exchange operation

In reply to Ferruh Yigit

Quoted 9 lines · Ferruh Yigit

From: Stephen Hemminger <[email protected]>
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 <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>

Series applied to dpdk-next-net/master, thanks.

Thomas Monjalon

2018-01-21 23:51 UTC #40 lore raw

Re: [PATCH v6 01/14] eal: introduce atomic exchange operation

21/01/2018 20:25, Ferruh Yigit:

Quoted 11 lines · Ferruh Yigit

Quoted 9 lines
From: Stephen Hemminger <[email protected]>
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 <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>
Series applied to dpdk-next-net/master, thanks.

I need to drop this series when pulling in master, because PPC is not supported.

Chao, please could you help on this feature?
Thanks

Stephen Hemminger

author

2018-01-22 16:56 UTC #41 lore raw

Re: [PATCH v6 01/14] eal: introduce atomic exchange operation

Quoted 11 lines · On Mon, 22 Jan 2018 00:50:55 +0100 Thomas Monjalon <[email protected]> wrote:

21/01/2018 20:25, Ferruh Yigit:
Quoted 10 lines · Ferruh Yigit
Quoted 9 lines
From: Stephen Hemminger <[email protected]>
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 <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>

Quoted 7 lines

Quoted 1 line
Series applied to dpdk-next-net/master, thanks.
I need to drop this series when pulling in master, because PPC is not supported.
Chao, please could you help on this feature?
Thanks

The generic code should work for PPC.

Thomas Monjalon

2018-01-22 21:03 UTC #42 lore raw

Re: [PATCH v6 01/14] eal: introduce atomic exchange operation

22/01/2018 17:56, Stephen Hemminger:

Quoted 16 lines

Quoted 13 lines · On Mon, 22 Jan 2018 00:50:55 +0100 Thomas Monjalon <[email protected]> wrote:
21/01/2018 20:25, Ferruh Yigit:
Quoted 11 lines · Ferruh Yigit
Quoted 9 lines
From: Stephen Hemminger <[email protected]>
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 <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>
Series applied to dpdk-next-net/master, thanks.

Quoted 7 lines

Quoted 5 lines
I need to drop this series when pulling in master, because PPC is not supported.
Chao, please could you help on this feature?
Thanks
The generic code should work for PPC.

No, the generic code is inside #ifdef RTE_FORCE_INTRINSICS. PPC does not define RTE_FORCE_INTRINSICS,
and ARM could disable it I guess.

Stephen Hemminger

author

2018-01-22 21:48 UTC #43 lore raw

Re: [PATCH v6 01/14] eal: introduce atomic exchange operation

Quoted 20 lines · On Mon, 22 Jan 2018 22:03:02 +0100 Thomas Monjalon <[email protected]> wrote:

22/01/2018 17:56, Stephen Hemminger:
Quoted 19 lines
Quoted 16 lines · On Mon, 22 Jan 2018 00:50:55 +0100 Thomas Monjalon <[email protected]> wrote:
21/01/2018 20:25, Ferruh Yigit:
Quoted 11 lines · Ferruh Yigit
Quoted 9 lines
From: Stephen Hemminger <[email protected]>
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 <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>
Series applied to dpdk-next-net/master, thanks.
I need to drop this series when pulling in master, because PPC is not supported.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Chao, please could you help on this feature?
Thanks
The generic code should work for PPC.
No, the generic code is inside #ifdef RTE_FORCE_INTRINSICS. PPC does not define RTE_FORCE_INTRINSICS,
and ARM could disable it I guess.

I will add a non intrinsic generic version using cmpset.

Thomas Monjalon

2018-01-22 22:09 UTC #44 lore raw

Re: [PATCH v6 01/14] eal: introduce atomic exchange operation

22/01/2018 22:48, Stephen Hemminger:

Quoted 26 lines

Quoted 23 lines · On Mon, 22 Jan 2018 22:03:02 +0100 Thomas Monjalon <[email protected]> wrote:
22/01/2018 17:56, Stephen Hemminger:
Quoted 22 lines
Quoted 18 lines · On Mon, 22 Jan 2018 00:50:55 +0100 Thomas Monjalon <[email protected]> wrote:
21/01/2018 20:25, Ferruh Yigit:
Quoted 11 lines · Ferruh Yigit
Quoted 9 lines
From: Stephen Hemminger <[email protected]>
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 <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>
Series applied to dpdk-next-net/master, thanks.
I need to drop this series when pulling in master, because PPC is not supported.
Chao, please could you help on this feature?
Thanks

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
The generic code should work for PPC.
No, the generic code is inside #ifdef RTE_FORCE_INTRINSICS. PPC does not define RTE_FORCE_INTRINSICS,
and ARM could disable it I guess.
I will add a non intrinsic generic version using cmpset.

OK thanks