[RFC 00/14] link status API improvement and bugfixes

[RFC 00/14] link status API improvement and bugfixes

[Stephen Hemminger]

While writing new driver, I 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 several drivers have
bugs in this are because they don't return the correct value.
Also, some drivers were not fully initializing all the space (including
padding).

This is compile test only at this point, I don't have any of the hardware
available (except virtio) to test.

Stephen Hemminger (14):
  ethdev: add link status read/write functions
  virtio: use eth_link_read/write (and bug fix)
  bnxt: use rte_link_update
  vmxnet3: use rte_eth_link_update
  dpaa2: use rte_eth_link_update
  nfp: use rte_eth_link_update
  e1000: use rte_eth_link_update
  ixgbe: use rte_eth_link_update
  sfc: use new rte_eth_link helpers
  i40e: use rte_eth_link_update (and bug fix)
  liquidio: use _rte_eth_link_update
  thunderx: use _rte_eth_link_update
  szedata: use _rte_eth_link_update
  enic: use _rte_eth_link_update

 drivers/net/bnxt/bnxt_ethdev.c          | 21 +-------
 drivers/net/dpaa2/dpaa2_ethdev.c        | 66 +++---------------------
 drivers/net/e1000/em_ethdev.c           | 70 ++------------------------
 drivers/net/e1000/igb_ethdev.c          | 71 ++------------------------
 drivers/net/enic/enic_ethdev.c          |  5 +-
 drivers/net/enic/enic_main.c            | 16 +++---
 drivers/net/i40e/i40e_ethdev.c          | 44 +++-------------
 drivers/net/i40e/i40e_ethdev_vf.c       | 19 +------
 drivers/net/ixgbe/ixgbe_ethdev.c        | 89 +++++----------------------------
 drivers/net/liquidio/lio_ethdev.c       | 76 +++++++---------------------
 drivers/net/nfp/nfp_net.c               | 74 +++------------------------
 drivers/net/sfc/sfc_ethdev.c            | 27 +++-------
 drivers/net/sfc/sfc_ev.c                | 23 ++-------
 drivers/net/szedata2/rte_eth_szedata2.c | 18 +++----
 drivers/net/thunderx/nicvf_ethdev.c     | 17 +------
 drivers/net/virtio/virtio_ethdev.c      | 54 +++-----------------
 drivers/net/vmxnet3/vmxnet3_ethdev.c    | 66 ++----------------------
 lib/librte_ether/rte_ethdev.c           | 36 +++++++++++++
 lib/librte_ether/rte_ethdev.h           | 28 +++++++++++
 19 files changed, 171 insertions(+), 649 deletions(-)

-- 
2.11.0

[RFC 01/14] ethdev: add link status read/write functions

[Stephen Hemminger]

Many drivers are all doing copy/paste of the same code to atomicly
update the link status. Reduce duplication, and allow for future
changes by having common function for this.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a1b744704f3a..7532fc6b65f0 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
 }
 
 int
+_rte_eth_link_update(struct rte_eth_dev *dev,
+		    const struct rte_eth_link *link)
+{
+	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
+	struct rte_eth_link old;
+
+	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+	old = *dev_link;
+
+	/* Only reason we use cmpset rather than set is
+	 * that on some architecture may use sign bit as a flag value.
+	 */
+	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
+				    *(volatile uint64_t *)dev_link,
+				   *(const uint64_t *)link) == 0)
+		continue;
+
+	return (old.link_status == link->link_status) ? -1 : 0;
+}
+
+void _rte_eth_link_read(struct rte_eth_dev *dev,
+			struct rte_eth_link *link)
+{
+	const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
+	volatile uint64_t *dst = (uint64_t *)link;
+
+	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+	/* Note: this should never fail since both destination and expected
+	 * values are the same and are a pointer from caller.
+	 */
+	rte_atomic64_cmpset(dst, *dst, *src);
+}
+
+int
 rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index f6837278521c..974657933f23 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct rte_eth_link *link);
  */
 void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
 
+
+/**
+ * @internal
+ * Atomically write the link status for the specific device.
+ * It is for use by DPDK device driver use only.
+ * User applications should not call it
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param link
+ *  New link status value.
+ * @return
+ *  -1 if link state has changed, 0 if the same.
+ */
+int _rte_eth_link_update(struct rte_eth_dev *dev,
+			 const struct rte_eth_link *link);
+
+/**
+ * @internal
+ * Atomically read the link speed and status.
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ * @param link
+ *  link status value.
+ */
+void _rte_eth_link_read(struct rte_eth_dev *dev,
+			struct rte_eth_link *link);
+
 /**
  * Retrieve the general I/O statistics of an Ethernet device.
  *
-- 
2.11.0

[RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Stephen Hemminger]

Use the new code in ethdev to handle link status update.
Also, virtio was not correctly setting the autoneg flags
since its speed should be marked as fixed.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/virtio/virtio_ethdev.c | 54 +++++---------------------------------
 1 file changed, 6 insertions(+), 48 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 00a3122780ba..776ad4961a37 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -43,7 +43,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_ether.h>
@@ -794,46 +793,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)
 {
@@ -1829,20 +1788,20 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 
 	hw->started = 0;
 	memset(&link, 0, sizeof(link));
-	virtio_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_link_update(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;
+	struct rte_eth_link link;
+	uint16_t status;
+
 	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;
+	link.link_autoneg = ETH_LINK_SPEED_FIXED;
 
 	if (hw->started == 0) {
 		link.link_status = ETH_LINK_DOWN;
@@ -1863,9 +1822,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 	} else {
 		link.link_status = ETH_LINK_UP;
 	}
-	virtio_dev_atomic_write_link_status(dev, &link);
 
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return _rte_eth_link_update(dev, &link);
 }
 
 static void
-- 
2.11.0

[RFC 03/14] bnxt: use rte_link_update

[Stephen Hemminger]

Use new API to update link status, and fix incorrect return
value.  The link_update operation should have been returning -1
if link changed.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_ethdev.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index c9d11228be46..4c79eb51fc22 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -469,20 +469,6 @@ static int bnxt_dev_configure_op(struct rte_eth_dev *eth_dev)
 	return 0;
 }
 
-static inline int
-rte_bnxt_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 void bnxt_print_link_info(struct rte_eth_dev *eth_dev)
 {
 	struct rte_eth_link *link = &eth_dev->data->dev_link;
@@ -685,12 +671,9 @@ int bnxt_link_update_op(struct rte_eth_dev *eth_dev, int wait_to_complete)
 	} while (!new.link_status && cnt--);
 
 out:
-	/* Timed out or success */
-	if (new.link_status != eth_dev->data->dev_link.link_status ||
-	new.link_speed != eth_dev->data->dev_link.link_speed) {
-		rte_bnxt_atomic_write_link_status(eth_dev, &new);
+	rc = _rte_eth_link_update(eth_dev, &new);
+	if (rc)
 		bnxt_print_link_info(eth_dev);
-	}
 
 	return rc;
 }
-- 
2.11.0

[RFC 04/14] vmxnet3: use rte_eth_link_update

[Stephen Hemminger]

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

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

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 72ec67c01cdd..910afb9f0eeb 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -48,7 +48,6 @@
 #include <rte_log.h>
 #include <rte_debug.h>
 #include <rte_pci.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_memory.h>
 #include <rte_memzone.h>
@@ -57,7 +56,6 @@
 #include <rte_ether.h>
 #include <rte_ethdev.h>
 #include <rte_ethdev_pci.h>
-#include <rte_atomic.h>
 #include <rte_string_fns.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
@@ -186,59 +184,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()
  */
@@ -878,7 +823,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 
 	/* Clear recorded link status */
 	memset(&link, 0, sizeof(link));
-	vmxnet3_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_link_update(dev, &link);
 }
 
 /*
@@ -1154,12 +1099,9 @@ __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;
 	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);
 
@@ -1170,9 +1112,7 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev,
 		link.link_autoneg = ETH_LINK_SPEED_FIXED;
 	}
 
-	vmxnet3_dev_atomic_write_link_status(dev, &link);
-
-	return (old.link_status == link.link_status) ? -1 : 0;
+	return _rte_eth_link_update(dev, &link);
 }
 
 static int
-- 
2.11.0

[RFC 05/14] dpaa2: use rte_eth_link_update

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/dpaa2/dpaa2_ethdev.c | 66 +++++-----------------------------------
 1 file changed, 8 insertions(+), 58 deletions(-)

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 44a5bc2bdb3d..80ce1c5ef8cb 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -59,58 +59,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)
 {
@@ -675,7 +623,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
@@ -707,7 +655,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
@@ -1021,8 +969,8 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,
 		RTE_LOG(ERR, PMD, "dpni is NULL\n");
 		return 0;
 	}
-	memset(&old, 0, sizeof(old));
-	dpaa2_dev_atomic_read_link_status(dev, &old);
+
+	_rte_eth_link_read(dev, &old);
 
 	ret = dpni_get_link_state(dpni, CMD_PRI_LOW, priv->token, &state);
 	if (ret < 0) {
@@ -1044,13 +992,15 @@ 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_link_write(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\n", dev->data->port_id);
-	return 0;
+
+
+	return (old.link_status == link.link_status) ? -1 : 0;
 }
 
 /**
-- 
2.11.0

[RFC 06/14] nfp: use rte_eth_link_update

[Stephen Hemminger]

Use new _rte_eth_link_update helper function.

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

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 92b03c4cb1fe..50b71ab8ca7c 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -203,57 +203,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)
 {
@@ -862,7 +811,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[] = {
@@ -880,9 +829,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));
@@ -907,15 +853,13 @@ 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");
+	if (_rte_eth_link_update(dev, &link) == 0)
 		return 0;
-	}
 
+	if (link.link_status)
+		PMD_DRV_LOG(INFO, "NIC Link is Up\n");
+	else
+		PMD_DRV_LOG(INFO, "NIC Link is Down\n");
 	return -1;
 }
 
@@ -1236,8 +1180,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_link_read(dev, &link);
 	if (link.link_status)
 		RTE_LOG(INFO, PMD, "Port %d: Link Up - speed %u Mbps - %s\n",
 			(int)(dev->data->port_id), (unsigned)link.link_speed,
@@ -1291,8 +1234,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_link_read(dev, &link);
 
 	nfp_net_link_update(dev, 0);
 
-- 
2.11.0

[RFC 07/14] e1000: use rte_eth_link_update

[Stephen Hemminger]

Use new helper API.

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

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 3d4ab93687f6..8d9783adcec0 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -49,7 +49,6 @@
 #include <rte_memory.h>
 #include <rte_memzone.h>
 #include <rte_eal.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
 
@@ -222,57 +221,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
@@ -793,7 +741,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_link_update(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -1150,7 +1098,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;
@@ -1185,8 +1133,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)) {
@@ -1205,14 +1151,8 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_DOWN;
 		link.link_autoneg = ETH_LINK_SPEED_FIXED;
 	}
-	rte_em_dev_atomic_write_link_status(dev, &link);
-
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
 
-	/* changed */
-	return 0;
+	return _rte_eth_link_update(dev, &link);
 }
 
 /*
@@ -1620,8 +1560,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_link_read(dev, &link);
+
 	if (link.link_status) {
 		PMD_INIT_LOG(INFO, " Port %d: Link Up - speed %u Mbps - %s",
 			     dev->data->port_id, (unsigned)link.link_speed,
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index da03d9bb348e..1a28d2fee286 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -49,7 +49,6 @@
 #include <rte_memory.h>
 #include <rte_memzone.h>
 #include <rte_eal.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_dev.h>
 
@@ -549,57 +548,6 @@ static const struct rte_igb_xstats_name_off rte_igbvf_stats_strings[] = {
 #define IGBVF_NB_XSTATS (sizeof(rte_igbvf_stats_strings) / \
 		sizeof(rte_igbvf_stats_strings[0]))
 
-/**
- * Atomically reads the link status information from global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_igb_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-/**
- * Atomically writes the link status information into global
- * structure rte_eth_dev.
- *
- * @param dev
- *   - Pointer to the structure rte_eth_dev to read from.
- *   - Pointer to the buffer to be saved with the link status.
- *
- * @return
- *   - On success, zero.
- *   - On failure, negative value.
- */
-static inline int
-rte_igb_dev_atomic_write_link_status(struct rte_eth_dev *dev,
-				struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = &(dev->data->dev_link);
-	struct rte_eth_link *src = link;
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
 
 static inline void
 igb_intr_enable(struct rte_eth_dev *dev)
@@ -1545,7 +1493,7 @@ eth_igb_stop(struct rte_eth_dev *dev)
 
 	/* clear the recorded link status */
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_link_update(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -1621,7 +1569,7 @@ eth_igb_close(struct rte_eth_dev *dev)
 	}
 
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_write_link_status(dev, &link);
+	_rte_eth_link_update(dev, &link);
 }
 
 static int
@@ -2366,7 +2314,7 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct e1000_hw *hw =
 		E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct rte_eth_link link, old;
+	struct rte_eth_link link;
 	int link_check, count;
 
 	link_check = 0;
@@ -2407,8 +2355,6 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		rte_delay_ms(IGB_LINK_UPDATE_CHECK_INTERVAL);
 	}
 	memset(&link, 0, sizeof(link));
-	rte_igb_dev_atomic_read_link_status(dev, &link);
-	old = link;
 
 	/* Now we check if a transition has happened */
 	if (link_check) {
@@ -2427,14 +2373,8 @@ eth_igb_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		link.link_status = ETH_LINK_DOWN;
 		link.link_autoneg = ETH_LINK_SPEED_FIXED;
 	}
-	rte_igb_dev_atomic_write_link_status(dev, &link);
 
-	/* not changed */
-	if (old.link_status == link.link_status)
-		return -1;
-
-	/* changed */
-	return 0;
+	return _rte_eth_link_update(dev, &link);
 }
 
 /*
@@ -2865,8 +2805,7 @@ eth_igb_interrupt_action(struct rte_eth_dev *dev,
 		if (ret < 0)
 			return 0;
 
-		memset(&link, 0, sizeof(link));
-		rte_igb_dev_atomic_read_link_status(dev, &link);
+		_rte_eth_link_read(dev, &link);
 		if (link.link_status) {
 			PMD_INIT_LOG(INFO,
 				     " Port %d: Link Up - speed %u Mbps - %s",
-- 
2.11.0

[RFC 08/14] ixgbe: use rte_eth_link_update

[Stephen Hemminger]

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

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

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index c54d32560bfc..5427360bd440 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -48,7 +48,6 @@
 #include <rte_log.h>
 #include <rte_debug.h>
 #include <rte_pci.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_memory.h>
 #include <rte_memzone.h>
@@ -57,7 +56,6 @@
 #include <rte_ether.h>
 #include <rte_ethdev.h>
 #include <rte_ethdev_pci.h>
-#include <rte_atomic.h>
 #include <rte_malloc.h>
 #include <rte_random.h>
 #include <rte_dev.h>
@@ -809,58 +807,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 +2699,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_link_update(dev, &link);
 
 	if (!rte_intr_allow_others(intr_handle))
 		/* resume to the default handler */
@@ -3901,7 +3847,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 link;
 	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
 	struct ixgbe_interrupt *intr =
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
@@ -3911,11 +3857,10 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	int wait = 1;
 	bool autoneg = false;
 
+	memset(&link, 0, sizeof(link));
 	link.link_status = ETH_LINK_DOWN;
 	link.link_speed = 0;
 	link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	memset(&old, 0, sizeof(old));
-	rte_ixgbe_dev_atomic_read_link_status(dev, &old);
 
 	hw->mac.get_link_status = true;
 
@@ -3939,19 +3884,15 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 	if (diag != 0) {
 		link.link_speed = ETH_SPEED_NUM_100M;
 		link.link_duplex = ETH_LINK_FULL_DUPLEX;
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+
+		return _rte_eth_link_update(dev, &link);
 	}
 
 	if (link_up == 0) {
-		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
 		intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
-		if (link.link_status == old.link_status)
-			return -1;
-		return 0;
+		return _rte_eth_link_update(dev, &link);
 	}
+
 	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 	link.link_status = ETH_LINK_UP;
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
@@ -3975,12 +3916,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		link.link_speed = ETH_SPEED_NUM_10G;
 		break;
 	}
-	rte_ixgbe_dev_atomic_write_link_status(dev, &link);
-
-	if (link.link_status == old.link_status)
-		return -1;
 
-	return 0;
+	return _rte_eth_link_update(dev, &link);
 }
 
 static int
@@ -4174,8 +4111,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_link_read(dev, &link);
+
 	if (link.link_status) {
 		PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s",
 					(int)(dev->data->port_id),
@@ -4228,8 +4165,7 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 
 	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE) {
 		/* 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_link_read(dev, &link);
 
 		ixgbe_dev_link_update(dev, 0);
 
@@ -6691,9 +6627,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_link_read(dev, &link);
 
 	switch (link.link_speed) {
 	case ETH_SPEED_NUM_100M:
-- 
2.11.0

[RFC 09/14] sfc: use new rte_eth_link helpers

[Stephen Hemminger]

Use the new API (_rte_eth_link_update) to handle link status update.
ALso fixes a bug where this driver was not returning -1 when link status changed.

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

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 6b06f08f7c1a..f0a40ad06f23 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -231,22 +231,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;
 
@@ -254,21 +244,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 			link_mode = EFX_LINK_UNKNOWN;
 		sfc_port_link_mode_to_info(link_mode, &current_link);
 
-		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					 *(uint64_t *)&old_link,
-					 *(uint64_t *)&current_link))
-			goto retry;
 	} else {
 		sfc_ev_mgmt_qpoll(sa);
-		*(int64_t *)&current_link =
-			rte_atomic64_read((rte_atomic64_t *)dev_link);
+		_rte_eth_link_read(dev, &current_link);
 	}
 
-	if (old_link.link_status != current_link.link_status)
-		sfc_info(sa, "Link status is %s",
-			 current_link.link_status ? "UP" : "DOWN");
+	if (_rte_eth_link_update(dev, &current_link) == 0)
+		return 0;
+
+	sfc_info(sa, "Link status is %s",
+		 current_link.link_status ? "UP" : "DOWN");
 
-	return old_link.link_status == current_link.link_status ? 0 : -1;
+	return -1;
 }
 
 static void
diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index a16dc27b380e..fc88baef0ef3 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
 {
 	struct sfc_evq *evq = arg;
 	struct sfc_adapter *sa = evq->sa;
-	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
 	struct rte_eth_link new_link;
-	uint64_t new_link_u64;
-	uint64_t old_link_u64;
-
-	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
 
 	sfc_port_link_mode_to_info(link_mode, &new_link);
+	if (_rte_eth_link_update(sa->eth_dev, &new_link) == 0)
+		return B_FALSE;
 
-	new_link_u64 = *(uint64_t *)&new_link;
-	do {
-		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
-		if (old_link_u64 == new_link_u64)
-			break;
-
-		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-					old_link_u64, new_link_u64)) {
-			evq->sa->port.lsc_seq++;
-			break;
-		}
-	} while (B_TRUE);
-
-	return B_FALSE;
+	evq->sa->port.lsc_seq++;
+	return B_TRUE;
 }
 
 static const efx_ev_callbacks_t sfc_ev_callbacks = {
-- 
2.11.0

[RFC 10/14] i40e: use rte_eth_link_update (and bug fix)

[Stephen Hemminger]

Use new rte_link_update API, and as a side effect fix bug
where driver was not correctly returning link status changes.

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

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 97a73e1cb913..0f824b022ce2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -650,34 +650,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");
@@ -2252,17 +2224,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);
@@ -2315,13 +2286,10 @@ i40e_dev_link_update(struct rte_eth_dev *dev,
 			ETH_LINK_SPEED_FIXED);
 
 out:
-	rte_i40e_dev_atomic_write_link_status(dev, &link);
-	if (link.link_status == old.link_status)
-		return -1;
-
+	status = _rte_eth_link_update(dev, &link);
 	i40e_notify_all_vfs_link_status(dev);
 
-	return 0;
+	return status;
 }
 
 /* Get all the statistics of a VSI */
@@ -9746,7 +9714,7 @@ i40e_start_timecounters(struct rte_eth_dev *dev)
 	/* Get current link speed. */
 	memset(&link, 0, sizeof(link));
 	i40e_dev_link_update(dev, 1);
-	rte_i40e_dev_atomic_read_link_status(dev, &link);
+	_rte_eth_link_read(dev, &link);
 
 	switch (link.link_speed) {
 	case ETH_SPEED_NUM_40G:
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index bab09f814215..491957bc65fa 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1124,20 +1124,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)
@@ -2170,6 +2156,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:
@@ -2199,9 +2186,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,
 	new_link.link_status = vf->link_up ? ETH_LINK_UP :
 					     ETH_LINK_DOWN;
 
-	i40evf_dev_atomic_write_link_status(dev, &new_link);
-
-	return 0;
+	return _rte_eth_link_update(dev, &new_link);
 }
 
 static void
-- 
2.11.0

[RFC 11/14] liquidio: use _rte_eth_link_update

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/liquidio/lio_ethdev.c | 76 ++++++++++-----------------------------
 1 file changed, 19 insertions(+), 57 deletions(-)

diff --git a/drivers/net/liquidio/lio_ethdev.c b/drivers/net/liquidio/lio_ethdev.c
index 479936a52ff9..95dc7232601e 100644
--- a/drivers/net/liquidio/lio_ethdev.c
+++ b/drivers/net/liquidio/lio_ethdev.c
@@ -888,32 +888,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)
 {
@@ -933,45 +907,33 @@ 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;
+	struct rte_eth_link link;
 
 	/* Initialize */
-	link.link_status = ETH_LINK_DOWN;
-	link.link_speed = ETH_SPEED_NUM_NONE;
-	link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	memset(&old, 0, sizeof(old));
-
+	memset(&link, 0, sizeof(link));
 	/* 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;
-	}
-
-	link.link_status = ETH_LINK_UP; /* Interface is up */
-	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	switch (lio_dev->linfo.link.s.speed) {
-	case LIO_LINK_SPEED_10000:
-		link.link_speed = ETH_SPEED_NUM_10G;
-		break;
-	case LIO_LINK_SPEED_25000:
-		link.link_speed = ETH_SPEED_NUM_25G;
-		break;
-	default:
+		link.link_status = ETH_LINK_DOWN;
 		link.link_speed = ETH_SPEED_NUM_NONE;
 		link.link_duplex = ETH_LINK_HALF_DUPLEX;
-	}
-
-	if (lio_dev_atomic_write_link_status(eth_dev, &link))
-		return -1;
+	} else {
+		link.link_status = ETH_LINK_UP; /* Interface is up */
+		link.link_duplex = ETH_LINK_FULL_DUPLEX;
+		switch (lio_dev->linfo.link.s.speed) {
+		case LIO_LINK_SPEED_10000:
+			link.link_speed = ETH_SPEED_NUM_10G;
+			break;
+		case LIO_LINK_SPEED_25000:
+			link.link_speed = ETH_SPEED_NUM_25G;
+			break;
+		default:
+			link.link_speed = ETH_SPEED_NUM_NONE;
+			link.link_duplex = ETH_LINK_HALF_DUPLEX;
+		}
 
-	if (link.link_status == old.link_status)
-		return -1;
+	}
 
-	return 0;
+	return _rte_eth_link_update(eth_dev, &link);
 }
 
 /**
-- 
2.11.0

[RFC 12/14] thunderx: use _rte_eth_link_update

[Stephen Hemminger]

Use new helper function.

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

diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index edc17f1d4002..113e75bc14de 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -43,7 +43,6 @@
 #include <sys/queue.h>
 
 #include <rte_alarm.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
 #include <rte_common.h>
@@ -75,19 +74,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)
@@ -169,7 +155,8 @@ nicvf_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		memset(&link, 0, sizeof(link));
 		nicvf_set_eth_link_status(nic, &link);
 	}
-	return nicvf_atomic_write_link_status(dev, &link);
+
+	return _rte_eth_link_update(dev, &link);
 }
 
 static int
-- 
2.11.0

[RFC 13/14] szedata: use _rte_eth_link_update

[Stephen Hemminger]

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

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/szedata2/rte_eth_szedata2.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index 9c0d57cc14c0..b81ba8e79c64 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -50,7 +50,6 @@
 #include <rte_memcpy.h>
 #include <rte_kvargs.h>
 #include <rte_dev.h>
-#include <rte_atomic.h>
 
 #include "rte_eth_szedata2.h"
 #include "szedata2_iobuf.h"
@@ -1171,9 +1170,7 @@ 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 rte_eth_link link, old;
 	struct pmd_internals *internals = (struct pmd_internals *)
 		dev->data->dev_private;
 	const volatile struct szedata2_ibuf *ibuf;
@@ -1195,8 +1192,12 @@ eth_link_update(struct rte_eth_dev *dev,
 		break;
 	}
 
+	_rte_eth_link_read(dev, &old);
+	memset(&link, 0, sizeof(link));
+
 	/* szedata2 uses only full duplex */
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
+	link.link_autoneg = ETH_LINK_SPEED_FIXED;
 
 	for (i = 0; i < szedata2_ibuf_count; i++) {
 		ibuf = ibuf_ptr_by_index(internals->pci_rsc, i);
@@ -1210,14 +1211,11 @@ eth_link_update(struct rte_eth_dev *dev,
 		}
 	}
 
-	link.link_status = (link_is_up) ? ETH_LINK_UP : ETH_LINK_DOWN;
+	link.link_status = link_is_up ? ETH_LINK_UP : ETH_LINK_DOWN;
 
-	link.link_autoneg = ETH_LINK_SPEED_FIXED;
+	_rte_eth_link_write(dev, &link);
 
-	rte_atomic64_cmpset((uint64_t *)dev_link, *(uint64_t *)dev_link,
-			*(uint64_t *)link_ptr);
-
-	return 0;
+	return (old.link_status == link.link_status) ? -1 : 0;
 }
 
 static int
-- 
2.11.0

[RFC 14/14] enic: use _rte_eth_link_update

[Stephen Hemminger]

This driver was not doing atomic update of link status information.
And the return value was different than others.

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

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index da8fec2d00ae..d2644ce608a5 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -418,10 +418,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_link_update(eth_dev, &link);
 }
 
 /*
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 40dbec7fa2d5..e629ee0b3a35 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -409,16 +409,16 @@ 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;
+	int link_status = enic_get_link_status(enic);
 
-	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;
+	memset(&link, 0, sizeof(link));
+	link.link_status = link_status;
+	link.link_duplex = ETH_LINK_FULL_DUPLEX;
+	link.link_speed = vnic_dev_port_speed(enic->vdev);
+
+	return _rte_eth_link_update(eth_dev, &link);
 }
 
 static void
-- 
2.11.0

Re: [RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Andrew Rybchenko]

On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> Use the new code in ethdev to handle link status update.
> Also, virtio was not correctly setting the autoneg flags
> since its speed should be marked as fixed.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 54 +++++---------------------------------
>   1 file changed, 6 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 00a3122780ba..776ad4961a37 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -43,7 +43,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_ether.h>
> @@ -794,46 +793,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)
>   {
> @@ -1829,20 +1788,20 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>   
>   	hw->started = 0;
>   	memset(&link, 0, sizeof(link));
> -	virtio_dev_atomic_write_link_status(dev, &link);
> +	_rte_eth_link_update(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;
> +	struct rte_eth_link link;
> +	uint16_t status;
> +
>   	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;
> +	link.link_autoneg = ETH_LINK_SPEED_FIXED;

As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
It looks like it has wrong comment:
         uint16_t link_autoneg : 1;  /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */

since
#define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds) */
#define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed speed) */

whereas
#define ETH_LINK_FIXED          0 /**< No autonegotiation. */
#define ETH_LINK_AUTONEG        1 /**< Autonegotiated. */

In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
defines with opposite values.

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

Re: [RFC 13/14] szedata: use _rte_eth_link_update

[Andrew Rybchenko]

On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> Yet another driver which was not returing correct value on
> link change.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   drivers/net/szedata2/rte_eth_szedata2.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
> index 9c0d57cc14c0..b81ba8e79c64 100644
> --- a/drivers/net/szedata2/rte_eth_szedata2.c
> +++ b/drivers/net/szedata2/rte_eth_szedata2.c
> @@ -50,7 +50,6 @@
>   #include <rte_memcpy.h>
>   #include <rte_kvargs.h>
>   #include <rte_dev.h>
> -#include <rte_atomic.h>
>   
>   #include "rte_eth_szedata2.h"
>   #include "szedata2_iobuf.h"
> @@ -1171,9 +1170,7 @@ 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 rte_eth_link link, old;
>   	struct pmd_internals *internals = (struct pmd_internals *)
>   		dev->data->dev_private;
>   	const volatile struct szedata2_ibuf *ibuf;
> @@ -1195,8 +1192,12 @@ eth_link_update(struct rte_eth_dev *dev,
>   		break;
>   	}
>   
> +	_rte_eth_link_read(dev, &old);
> +	memset(&link, 0, sizeof(link));
> +
>   	/* szedata2 uses only full duplex */
>   	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> +	link.link_autoneg = ETH_LINK_SPEED_FIXED;

See my comment for net/virtio patch. It is applicable here as well.

>   
>   	for (i = 0; i < szedata2_ibuf_count; i++) {
>   		ibuf = ibuf_ptr_by_index(internals->pci_rsc, i);
> @@ -1210,14 +1211,11 @@ eth_link_update(struct rte_eth_dev *dev,
>   		}
>   	}
>   
> -	link.link_status = (link_is_up) ? ETH_LINK_UP : ETH_LINK_DOWN;
> +	link.link_status = link_is_up ? ETH_LINK_UP : ETH_LINK_DOWN;
>   
> -	link.link_autoneg = ETH_LINK_SPEED_FIXED;

In fact, the bug was here before the patch.

> +	_rte_eth_link_write(dev, &link);
>   
> -	rte_atomic64_cmpset((uint64_t *)dev_link, *(uint64_t *)dev_link,
> -			*(uint64_t *)link_ptr);
> -
> -	return 0;
> +	return (old.link_status == link.link_status) ? -1 : 0;
>   }
>   
>   static int

Re: [RFC 01/14] ethdev: add link status read/write functions

[Andrew Rybchenko]

On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> Many drivers are all doing copy/paste of the same code to atomicly
> update the link status. Reduce duplication, and allow for future
> changes by having common function for this.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
>   lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a1b744704f3a..7532fc6b65f0 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
>   }
>   
>   int
> +_rte_eth_link_update(struct rte_eth_dev *dev,
> +		    const struct rte_eth_link *link)
> +{
> +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> +	struct rte_eth_link old;
> +
> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> +	old = *dev_link;
> +
> +	/* Only reason we use cmpset rather than set is
> +	 * that on some architecture may use sign bit as a flag value.

May I ask to provide more details here.

> +	 */
> +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> +				    *(volatile uint64_t *)dev_link,
> +				   *(const uint64_t *)link) == 0)

Shouldn't it be:
do {
       old = *dev_link;
} while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
*(uint64_t *)&old, *(const uint64_t *)link) == 0);

At least it has some sense to guarantee transition from old to new
talking below comparison into account.

> +		continue;
> +
> +	return (old.link_status == link->link_status) ? -1 : 0;
> +}
> +
> +void _rte_eth_link_read(struct rte_eth_dev *dev,
> +			struct rte_eth_link *link)
> +{
> +	const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> +	volatile uint64_t *dst = (uint64_t *)link;
> +
> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> +	/* Note: this should never fail since both destination and expected
> +	 * values are the same and are a pointer from caller.
> +	 */
> +	rte_atomic64_cmpset(dst, *dst, *src);
> +}
> +
> +int
>   rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)
>   {
>   	struct rte_eth_dev *dev;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index f6837278521c..974657933f23 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct rte_eth_link *link);
>    */
>   void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
>   
> +
> +/**
> + * @internal
> + * Atomically write the link status for the specific device.
> + * It is for use by DPDK device driver use only.
> + * User applications should not call it
> + *
> + * @param dev
> + *  Pointer to struct rte_eth_dev.
> + * @param link
> + *  New link status value.
> + * @return
> + *  -1 if link state has changed, 0 if the same.
> + */
> +int _rte_eth_link_update(struct rte_eth_dev *dev,
> +			 const struct rte_eth_link *link);
> +
> +/**
> + * @internal
> + * Atomically read the link speed and status.
> + * @param dev
> + *  Pointer to struct rte_eth_dev.
> + * @param link
> + *  link status value.
> + */
> +void _rte_eth_link_read(struct rte_eth_dev *dev,
> +			struct rte_eth_link *link);
> +
>   /**
>    * Retrieve the general I/O statistics of an Ethernet device.
>    *

Re: [RFC 09/14] sfc: use new rte_eth_link helpers

[Andrew Rybchenko]

On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> Use the new API (_rte_eth_link_update) to handle link status update.
> ALso fixes a bug where this driver was not returning -1 when link status changed.

It is really good that you raise it, since:
  -  as far as I can see the return value of the link_update is never used
  - return value is not described and it is unclear what is meant by "link
     status changed" since initial point is unspecified

We have interpreted link status change as change in data->dev_link
made in the current execution flow, but not in parallel execution flow.

>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   drivers/net/sfc/sfc_ethdev.c | 27 +++++++--------------------
>   drivers/net/sfc/sfc_ev.c     | 23 ++++-------------------
>   2 files changed, 11 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index 6b06f08f7c1a..f0a40ad06f23 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -231,22 +231,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;
>   
> @@ -254,21 +244,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
>   			link_mode = EFX_LINK_UNKNOWN;
>   		sfc_port_link_mode_to_info(link_mode, &current_link);
>   
> -		if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> -					 *(uint64_t *)&old_link,
> -					 *(uint64_t *)&current_link))
> -			goto retry;
>   	} else {
>   		sfc_ev_mgmt_qpoll(sa);
> -		*(int64_t *)&current_link =
> -			rte_atomic64_read((rte_atomic64_t *)dev_link);
> +		_rte_eth_link_read(dev, &current_link);
>   	}
>   
> -	if (old_link.link_status != current_link.link_status)
> -		sfc_info(sa, "Link status is %s",
> -			 current_link.link_status ? "UP" : "DOWN");
> +	if (_rte_eth_link_update(dev, &current_link) == 0)
> +		return 0;
> +
> +	sfc_info(sa, "Link status is %s",
> +		 current_link.link_status ? "UP" : "DOWN");
>   
> -	return old_link.link_status == current_link.link_status ? 0 : -1;
> +	return -1;
>   }
>   
>   static void
> diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
> index a16dc27b380e..fc88baef0ef3 100644
> --- a/drivers/net/sfc/sfc_ev.c
> +++ b/drivers/net/sfc/sfc_ev.c
> @@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
>   {
>   	struct sfc_evq *evq = arg;
>   	struct sfc_adapter *sa = evq->sa;
> -	struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
>   	struct rte_eth_link new_link;
> -	uint64_t new_link_u64;
> -	uint64_t old_link_u64;
> -
> -	EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
>   
>   	sfc_port_link_mode_to_info(link_mode, &new_link);
> +	if (_rte_eth_link_update(sa->eth_dev, &new_link) == 0)
> +		return B_FALSE;

It means that change of link speed without link down (not sure that it is
possible, but still) will happen without link status interrupt. May be link
status is about UP/DOWN only, but I'm not sure that it is a good limitation.

At least it is not what we want in initial implementation.

> -	new_link_u64 = *(uint64_t *)&new_link;
> -	do {
> -		old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link);
> -		if (old_link_u64 == new_link_u64)
> -			break;
> -
> -		if (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> -					old_link_u64, new_link_u64)) {
> -			evq->sa->port.lsc_seq++;
> -			break;
> -		}
> -	} while (B_TRUE);
> -
> -	return B_FALSE;
> +	evq->sa->port.lsc_seq++;
> +	return B_TRUE;
>   }
>   
>   static const efx_ev_callbacks_t sfc_ev_callbacks = {

Re: [RFC 00/14] link status API improvement and bugfixes

[Andrew Rybchenko]

On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> While writing new driver, I 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 several drivers have
> bugs in this are because they don't return the correct value.
> Also, some drivers were not fully initializing all the space (including
> padding).
>
> This is compile test only at this point, I don't have any of the hardware
> available (except virtio) to test.
>
> Stephen Hemminger (14):
>    ethdev: add link status read/write functions
>    virtio: use eth_link_read/write (and bug fix)
>    bnxt: use rte_link_update
>    vmxnet3: use rte_eth_link_update
>    dpaa2: use rte_eth_link_update
>    nfp: use rte_eth_link_update
>    e1000: use rte_eth_link_update
>    ixgbe: use rte_eth_link_update
>    sfc: use new rte_eth_link helpers
>    i40e: use rte_eth_link_update (and bug fix)
>    liquidio: use _rte_eth_link_update
>    thunderx: use _rte_eth_link_update
>    szedata: use _rte_eth_link_update
>    enic: use _rte_eth_link_update
>
>   drivers/net/bnxt/bnxt_ethdev.c          | 21 +-------
>   drivers/net/dpaa2/dpaa2_ethdev.c        | 66 +++---------------------
>   drivers/net/e1000/em_ethdev.c           | 70 ++------------------------
>   drivers/net/e1000/igb_ethdev.c          | 71 ++------------------------
>   drivers/net/enic/enic_ethdev.c          |  5 +-
>   drivers/net/enic/enic_main.c            | 16 +++---
>   drivers/net/i40e/i40e_ethdev.c          | 44 +++-------------
>   drivers/net/i40e/i40e_ethdev_vf.c       | 19 +------
>   drivers/net/ixgbe/ixgbe_ethdev.c        | 89 +++++----------------------------
>   drivers/net/liquidio/lio_ethdev.c       | 76 +++++++---------------------
>   drivers/net/nfp/nfp_net.c               | 74 +++------------------------
>   drivers/net/sfc/sfc_ethdev.c            | 27 +++-------
>   drivers/net/sfc/sfc_ev.c                | 23 ++-------
>   drivers/net/szedata2/rte_eth_szedata2.c | 18 +++----
>   drivers/net/thunderx/nicvf_ethdev.c     | 17 +------
>   drivers/net/virtio/virtio_ethdev.c      | 54 +++-----------------
>   drivers/net/vmxnet3/vmxnet3_ethdev.c    | 66 ++----------------------
>   lib/librte_ether/rte_ethdev.c           | 36 +++++++++++++
>   lib/librte_ether/rte_ethdev.h           | 28 +++++++++++
>   19 files changed, 171 insertions(+), 649 deletions(-)
>

I've provided my comments to individual patches.

In general it is very good that it removes so much duplicated and
nontrivial code with atomtics. It raises many questions and I think
finally it will allow to clarify link status API for PMD maintainers and
users.

Andrew.

Re: [RFC 01/14] ethdev: add link status read/write functions

[Stephen Hemminger]

On Sun, 16 Jul 2017 16:26:06 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> > Many drivers are all doing copy/paste of the same code to atomicly
> > update the link status. Reduce duplication, and allow for future
> > changes by having common function for this.
> >
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >   lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> >   lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> >   2 files changed, 64 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > index a1b744704f3a..7532fc6b65f0 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
> >   }
> >   
> >   int
> > +_rte_eth_link_update(struct rte_eth_dev *dev,
> > +		    const struct rte_eth_link *link)
> > +{
> > +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> > +	struct rte_eth_link old;
> > +
> > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > +
> > +	old = *dev_link;
> > +
> > +	/* Only reason we use cmpset rather than set is
> > +	 * that on some architecture may use sign bit as a flag value.  
> 
> May I ask to provide more details here.


rte_atomic64_set() takes an int64 argument.
This code (taken from ixgbe, virtio and other drivers) uses cmpset
to allow using uint64_t.

My assumption is that some architecture in the past was using the
sign bit a a lock value or something. On 64 bit no special support
for 64bit atomic assignment is necessary. Not sure how this code
got inherited that way.

> 
> > +	 */
> > +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > +				    *(volatile uint64_t *)dev_link,
> > +				   *(const uint64_t *)link) == 0)  
> 
> Shouldn't it be:
> do {
>        old = *dev_link;
> } while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> *(uint64_t *)&old, *(const uint64_t *)link) == 0);
> 
> At least it has some sense to guarantee transition from old to new
> talking below comparison into account.

Since dev_link is volatile, the compiler is required to refetch
the pointer every time it evaluates the expression. Maybe clearer
to alias devlink to a volatile uint64_t ptr.

Re: [RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Stephen Hemminger]

On Sun, 16 Jul 2017 15:33:26 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> > +	link.link_autoneg = ETH_LINK_SPEED_FIXED;  
> 
> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
> It looks like it has wrong comment:
>          uint16_t link_autoneg : 1;  /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
> 
> since
> #define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds) */
> #define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed speed) */
> 
> whereas
> #define ETH_LINK_FIXED          0 /**< No autonegotiation. */
> #define ETH_LINK_AUTONEG        1 /**< Autonegotiated. */
> 
> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
> defines with opposite values.

Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
and others were not. Turns out it makes no difference 
since FIXED == 0, the old code and new code have same effect.

Re: [RFC 09/14] sfc: use new rte_eth_link helpers

[Stephen Hemminger]

On Sun, 16 Jul 2017 16:48:50 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
> > Use the new API (_rte_eth_link_update) to handle link status update.
> > ALso fixes a bug where this driver was not returning -1 when link status changed.  
> 
> It is really good that you raise it, since:
>   -  as far as I can see the return value of the link_update is never used
>   - return value is not described and it is unclear what is meant by "link
>      status changed" since initial point is unspecified
> 
> We have interpreted link status change as change in data->dev_link
> made in the current execution flow, but not in parallel execution flow.

All drivers must do the same thing. Private interpretation is not good.

Re: [RFC 01/14] ethdev: add link status read/write functions

[Andrew Rybchenko]

On 07/17/2017 06:58 PM, Stephen Hemminger wrote:
> On Sun, 16 Jul 2017 16:26:06 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
>>> Many drivers are all doing copy/paste of the same code to atomicly
>>> update the link status. Reduce duplication, and allow for future
>>> changes by having common function for this.
>>>
>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>> ---
>>>    lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
>>>    lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
>>>    2 files changed, 64 insertions(+)
>>>
>>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>>> index a1b744704f3a..7532fc6b65f0 100644
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
>>>    }
>>>    
>>>    int
>>> +_rte_eth_link_update(struct rte_eth_dev *dev,
>>> +		    const struct rte_eth_link *link)
>>> +{
>>> +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
>>> +	struct rte_eth_link old;
>>> +
>>> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>>> +
>>> +	old = *dev_link;
>>> +
>>> +	/* Only reason we use cmpset rather than set is
>>> +	 * that on some architecture may use sign bit as a flag value.
>> May I ask to provide more details here.
>
> rte_atomic64_set() takes an int64 argument.
> This code (taken from ixgbe, virtio and other drivers) uses cmpset
> to allow using uint64_t.
>
> My assumption is that some architecture in the past was using the
> sign bit a a lock value or something. On 64 bit no special support
> for 64bit atomic assignment is necessary. Not sure how this code
> got inherited that way.

Many thanks. May be it would be useful in the comment as well.

>>> +	 */
>>> +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>>> +				    *(volatile uint64_t *)dev_link,
>>> +				   *(const uint64_t *)link) == 0)
>> Shouldn't it be:
>> do {
>>         old = *dev_link;
>> } while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>> *(uint64_t *)&old, *(const uint64_t *)link) == 0);
>>
>> At least it has some sense to guarantee transition from old to new
>> talking below comparison into account.
> Since dev_link is volatile, the compiler is required to refetch
> the pointer every time it evaluates the expression. Maybe clearer
> to alias devlink to a volatile uint64_t ptr.

I meant that dev_link value may change after old value saved in original 
patch,
but before cmpset which actually replaces dev_link value here. As the result
two _rte_eth_link_update() run in parallel changing to the same value 
may return
"changes done", but actually only one did the job.
I'm not sure if it is really important here, since requirements are not 
clear.

Re: ***Spam*** Re: [RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Andrew Rybchenko]

On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> On Sun, 16 Jul 2017 15:33:26 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>>> +	link.link_autoneg = ETH_LINK_SPEED_FIXED;
>> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
>> It looks like it has wrong comment:
>>           uint16_t link_autoneg : 1;  /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
>>
>> since
>> #define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds) */
>> #define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed speed) */
>>
>> whereas
>> #define ETH_LINK_FIXED          0 /**< No autonegotiation. */
>> #define ETH_LINK_AUTONEG        1 /**< Autonegotiated. */
>>
>> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
>> defines with opposite values.
> Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
> and others were not. Turns out it makes no difference
> since FIXED == 0, the old code and new code have same effect.

May be I miss something, but  ETH_LINK_SPEED_FIXED==1, but 
ETH_LINK_FIXED==0.
So, initially it was 0 (fixed speed), but now it is 1 (autoneg).

Re: [RFC 09/14] sfc: use new rte_eth_link helpers

[Andrew Rybchenko]

On 07/17/2017 07:02 PM, Stephen Hemminger wrote:
> On Sun, 16 Jul 2017 16:48:50 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
>>> Use the new API (_rte_eth_link_update) to handle link status update.
>>> ALso fixes a bug where this driver was not returning -1 when link status changed.
>> It is really good that you raise it, since:
>>    -  as far as I can see the return value of the link_update is never used
>>    - return value is not described and it is unclear what is meant by "link
>>       status changed" since initial point is unspecified
>>
>> We have interpreted link status change as change in data->dev_link
>> made in the current execution flow, but not in parallel execution flow.
> All drivers must do the same thing. Private interpretation is not good.

Definitely agree, but to do so, API documentation must clearly define
what is required. It is especially pity that all these "fixes" and 
complexity
is just a waste of time since return value is actually never used.

Re: [RFC 01/14] ethdev: add link status read/write functions

[Stephen Hemminger]

On Mon, 17 Jul 2017 19:12:01 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 07/17/2017 06:58 PM, Stephen Hemminger wrote:
> > On Sun, 16 Jul 2017 16:26:06 +0300
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >  
> >> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:  
> >>> Many drivers are all doing copy/paste of the same code to atomicly
> >>> update the link status. Reduce duplication, and allow for future
> >>> changes by having common function for this.
> >>>
> >>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >>> ---
> >>>    lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> >>>    lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> >>>    2 files changed, 64 insertions(+)
> >>>
> >>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> >>> index a1b744704f3a..7532fc6b65f0 100644
> >>> --- a/lib/librte_ether/rte_ethdev.c
> >>> +++ b/lib/librte_ether/rte_ethdev.c
> >>> @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
> >>>    }
> >>>    
> >>>    int
> >>> +_rte_eth_link_update(struct rte_eth_dev *dev,
> >>> +		    const struct rte_eth_link *link)
> >>> +{
> >>> +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> >>> +	struct rte_eth_link old;
> >>> +
> >>> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> >>> +
> >>> +	old = *dev_link;
> >>> +
> >>> +	/* Only reason we use cmpset rather than set is
> >>> +	 * that on some architecture may use sign bit as a flag value.  
> >> May I ask to provide more details here.  
> >
> > rte_atomic64_set() takes an int64 argument.
> > This code (taken from ixgbe, virtio and other drivers) uses cmpset
> > to allow using uint64_t.
> >
> > My assumption is that some architecture in the past was using the
> > sign bit a a lock value or something. On 64 bit no special support
> > for 64bit atomic assignment is necessary. Not sure how this code
> > got inherited that way.  
> 
> Many thanks. May be it would be useful in the comment as well.

Maybe one of the original developers could clarify.
It would be cleaner just to do rte_atomcic64_set(), it might just
be a leftover semantic from Linux/BSD/??? where the original developer
was looking.

> 
> >>> +	 */
> >>> +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> >>> +				    *(volatile uint64_t *)dev_link,
> >>> +				   *(const uint64_t *)link) == 0)  
> >> Shouldn't it be:
> >> do {
> >>         old = *dev_link;
> >> } while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> >> *(uint64_t *)&old, *(const uint64_t *)link) == 0);
> >>
> >> At least it has some sense to guarantee transition from old to new
> >> talking below comparison into account.  
> > Since dev_link is volatile, the compiler is required to refetch
> > the pointer every time it evaluates the expression. Maybe clearer
> > to alias devlink to a volatile uint64_t ptr.  
> 
> I meant that dev_link value may change after old value saved in original 
> patch,
> but before cmpset which actually replaces dev_link value here. As the result
> two _rte_eth_link_update() run in parallel changing to the same value 
> may return
> "changes done", but actually only one did the job.
> I'm not sure if it is really important here, since requirements are not 
> clear.

Since there is no locking here. There can not be a guarantee of ordering possible.
The only guarantee is that the set of values (duplex, speed, flags) is consistent.
I.e one caller wins, the streams don't get crossed.

Re: [RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Stephen Hemminger]

On Mon, 17 Jul 2017 19:14:16 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> > On Sun, 16 Jul 2017 15:33:26 +0300
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >  
> >>> +	link.link_autoneg = ETH_LINK_SPEED_FIXED;  
> >> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
> >> It looks like it has wrong comment:
> >>           uint16_t link_autoneg : 1;  /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
> >>
> >> since
> >> #define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds) */
> >> #define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed speed) */
> >>
> >> whereas
> >> #define ETH_LINK_FIXED          0 /**< No autonegotiation. */
> >> #define ETH_LINK_AUTONEG        1 /**< Autonegotiated. */
> >>
> >> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
> >> defines with opposite values.  
> > Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
> > and others were not. Turns out it makes no difference
> > since FIXED == 0, the old code and new code have same effect.  
> 
> May be I miss something, but  ETH_LINK_SPEED_FIXED==1, but 
> ETH_LINK_FIXED==0.
> So, initially it was 0 (fixed speed), but now it is 1 (autoneg).

My understanding is that ETH_SPEED_XXX and ETH_LINK_FIXED are for rte_eth_link
structure; and ETH_LINK_SPEED_XXX values are for dev_info->speed_capa

Would be good to use some kind of typedef for this, maybe introduce a bitfield
for speed_capa and get  rid of the ETH_LINK_SPEED_XXX.

Re: [RFC 01/14] ethdev: add link status read/write functions

[Andrew Rybchenko]

On 07/17/2017 07:21 PM, Stephen Hemminger wrote:
> On Mon, 17 Jul 2017 19:12:01 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 07/17/2017 06:58 PM, Stephen Hemminger wrote:
>>> On Sun, 16 Jul 2017 16:26:06 +0300
>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>   
>>>> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:
>>>>> Many drivers are all doing copy/paste of the same code to atomicly
>>>>> update the link status. Reduce duplication, and allow for future
>>>>> changes by having common function for this.
>>>>>
>>>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>>>> ---
>>>>>     lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>     lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
>>>>>     2 files changed, 64 insertions(+)
>>>>>
>>>>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>>>>> index a1b744704f3a..7532fc6b65f0 100644
>>>>> --- a/lib/librte_ether/rte_ethdev.c
>>>>> +++ b/lib/librte_ether/rte_ethdev.c
>>>>> @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
>>>>>     }
>>>>>     
>>>>>     int
>>>>> +_rte_eth_link_update(struct rte_eth_dev *dev,
>>>>> +		    const struct rte_eth_link *link)
>>>>> +{
>>>>> +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
>>>>> +	struct rte_eth_link old;
>>>>> +
>>>>> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>>>>> +
>>>>> +	old = *dev_link;
>>>>> +
>>>>> +	/* Only reason we use cmpset rather than set is
>>>>> +	 * that on some architecture may use sign bit as a flag value.
>>>> May I ask to provide more details here.
>>> rte_atomic64_set() takes an int64 argument.
>>> This code (taken from ixgbe, virtio and other drivers) uses cmpset
>>> to allow using uint64_t.
>>>
>>> My assumption is that some architecture in the past was using the
>>> sign bit a a lock value or something. On 64 bit no special support
>>> for 64bit atomic assignment is necessary. Not sure how this code
>>> got inherited that way.
>> Many thanks. May be it would be useful in the comment as well.
> Maybe one of the original developers could clarify.
> It would be cleaner just to do rte_atomcic64_set(), it might just
> be a leftover semantic from Linux/BSD/??? where the original developer
> was looking.

Agree.

>>>>> +	 */
>>>>> +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>>>>> +				    *(volatile uint64_t *)dev_link,
>>>>> +				   *(const uint64_t *)link) == 0)
>>>> Shouldn't it be:
>>>> do {
>>>>          old = *dev_link;
>>>> } while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
>>>> *(uint64_t *)&old, *(const uint64_t *)link) == 0);
>>>>
>>>> At least it has some sense to guarantee transition from old to new
>>>> talking below comparison into account.
>>> Since dev_link is volatile, the compiler is required to refetch
>>> the pointer every time it evaluates the expression. Maybe clearer
>>> to alias devlink to a volatile uint64_t ptr.
>> I meant that dev_link value may change after old value saved in original
>> patch,
>> but before cmpset which actually replaces dev_link value here. As the result
>> two _rte_eth_link_update() run in parallel changing to the same value
>> may return
>> "changes done", but actually only one did the job.
>> I'm not sure if it is really important here, since requirements are not
>> clear.
> Since there is no locking here. There can not be a guarantee of ordering possible.
> The only guarantee is that the set of values (duplex, speed, flags) is consistent.
> I.e one caller wins, the streams don't get crossed.

Results of the update operation is used by some driver to log link up/down
change. So, it could result in duplicate up/down logs. Not a big deal, but
could be confusing.

I guess many are very busy right now with 17.08 release. So, I hope
we'll see more feedback when 17.08 release is done.

Re: [RFC 11/14] liquidio: use _rte_eth_link_update

[Shijith Thotton]

On Fri, Jul 14, 2017 at 11:30:24AM -0700, Stephen Hemminger wrote:
> Use the new link update API, and cleanup the logic in the the
> link update routine.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  drivers/net/liquidio/lio_ethdev.c | 76 ++++++++++-----------------------------
>  1 file changed, 19 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/net/liquidio/lio_ethdev.c b/drivers/net/liquidio/lio_ethdev.c
> index 479936a52ff9..95dc7232601e 100644
> --- a/drivers/net/liquidio/lio_ethdev.c
> +++ b/drivers/net/liquidio/lio_ethdev.c
> @@ -888,32 +888,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)
>  {
> @@ -933,45 +907,33 @@ 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;
> +	struct rte_eth_link link;
>  
>  	/* Initialize */
> -	link.link_status = ETH_LINK_DOWN;
> -	link.link_speed = ETH_SPEED_NUM_NONE;
> -	link.link_duplex = ETH_LINK_HALF_DUPLEX;
> -	memset(&old, 0, sizeof(old));
> -
> +	memset(&link, 0, sizeof(link));
>  	/* 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;
> -	}
> -
> -	link.link_status = ETH_LINK_UP; /* Interface is up */
> -	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> -	switch (lio_dev->linfo.link.s.speed) {
> -	case LIO_LINK_SPEED_10000:
> -		link.link_speed = ETH_SPEED_NUM_10G;
> -		break;
> -	case LIO_LINK_SPEED_25000:
> -		link.link_speed = ETH_SPEED_NUM_25G;
> -		break;
> -	default:
> +		link.link_status = ETH_LINK_DOWN;
>  		link.link_speed = ETH_SPEED_NUM_NONE;
>  		link.link_duplex = ETH_LINK_HALF_DUPLEX;
> -	}
> -
> -	if (lio_dev_atomic_write_link_status(eth_dev, &link))
> -		return -1;
> +	} else {
> +		link.link_status = ETH_LINK_UP; /* Interface is up */
> +		link.link_duplex = ETH_LINK_FULL_DUPLEX;
> +		switch (lio_dev->linfo.link.s.speed) {
> +		case LIO_LINK_SPEED_10000:
> +			link.link_speed = ETH_SPEED_NUM_10G;
> +			break;
> +		case LIO_LINK_SPEED_25000:
> +			link.link_speed = ETH_SPEED_NUM_25G;
> +			break;
> +		default:
> +			link.link_speed = ETH_SPEED_NUM_NONE;
> +			link.link_duplex = ETH_LINK_HALF_DUPLEX;
> +		}
>  
> -	if (link.link_status == old.link_status)
> -		return -1;
> +	}
>  
> -	return 0;
> +	return _rte_eth_link_update(eth_dev, &link);
>  }
>  
>  /**
> -- 
> 2.11.0
> 

Tested-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>

Thanks,
Shijith

Re: [RFC 01/14] ethdev: add link status read/write functions

[Yang, Qiming]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Saturday, July 15, 2017 2:30 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> <sthemmin@microsoft.com>
> Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> 
> Many drivers are all doing copy/paste of the same code to atomicly update the
> link status. Reduce duplication, and allow for future changes by having common
> function for this.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
>  lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index
> a1b744704f3a..7532fc6b65f0 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct
> rte_eth_link *eth_link)  }
> 
>  int
> +_rte_eth_link_update(struct rte_eth_dev *dev,
> +		    const struct rte_eth_link *link)
> +{
> +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> +	struct rte_eth_link old;
> +
> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> +	old = *dev_link;
> +
> +	/* Only reason we use cmpset rather than set is
> +	 * that on some architecture may use sign bit as a flag value.
> +	 */
> +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> +				    *(volatile uint64_t *)dev_link,
> +				   *(const uint64_t *)link) == 0)
> +		continue;
> +
> +	return (old.link_status == link->link_status) ? -1 : 0; }
> +
> +void _rte_eth_link_read(struct rte_eth_dev *dev,
> +			struct rte_eth_link *link)
> +{
> +	const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> +	volatile uint64_t *dst = (uint64_t *)link;
> +
> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> +	/* Note: this should never fail since both destination and expected
> +	 * values are the same and are a pointer from caller.
> +	 */
> +	rte_atomic64_cmpset(dst, *dst, *src);
> +}
> +
> +int
>  rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)  {
>  	struct rte_eth_dev *dev;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> f6837278521c..974657933f23 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> rte_eth_link *link);
>   */
>  void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> 
> +
> +/**
> + * @internal
> + * Atomically write the link status for the specific device.
> + * It is for use by DPDK device driver use only.
> + * User applications should not call it
> + *
> + * @param dev
> + *  Pointer to struct rte_eth_dev.
> + * @param link
> + *  New link status value.
> + * @return
> + *  -1 if link state has changed, 0 if the same.
> + */
> +int _rte_eth_link_update(struct rte_eth_dev *dev,
> +			 const struct rte_eth_link *link);
> +
This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
Use name link_update makes me confused, and mix up it with dev_ops link_update.
> +/**
> + * @internal
> + * Atomically read the link speed and status.
> + * @param dev
> + *  Pointer to struct rte_eth_dev.
> + * @param link
> + *  link status value.
> + */
> +void _rte_eth_link_read(struct rte_eth_dev *dev,
> +			struct rte_eth_link *link);
> +
This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
>  /**
>   * Retrieve the general I/O statistics of an Ethernet device.
>   *
> --
> 2.11.0

Re: [RFC 01/14] ethdev: add link status read/write functions

[Stephen Hemminger]

On Wed, 11 Oct 2017 08:32:12 +0000
"Yang, Qiming" <qiming.yang@intel.com> wrote:

> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Saturday, July 15, 2017 2:30 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> > <sthemmin@microsoft.com>
> > Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> > 
> > Many drivers are all doing copy/paste of the same code to atomicly update the
> > link status. Reduce duplication, and allow for future changes by having common
> > function for this.
> > 
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> >  lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> > 
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index
> > a1b744704f3a..7532fc6b65f0 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct
> > rte_eth_link *eth_link)  }
> > 
> >  int
> > +_rte_eth_link_update(struct rte_eth_dev *dev,
> > +		    const struct rte_eth_link *link)
> > +{
> > +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> > +	struct rte_eth_link old;
> > +
> > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > +
> > +	old = *dev_link;
> > +
> > +	/* Only reason we use cmpset rather than set is
> > +	 * that on some architecture may use sign bit as a flag value.
> > +	 */
> > +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > +				    *(volatile uint64_t *)dev_link,
> > +				   *(const uint64_t *)link) == 0)
> > +		continue;
> > +
> > +	return (old.link_status == link->link_status) ? -1 : 0; }
> > +
> > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > +			struct rte_eth_link *link)
> > +{
> > +	const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> > +	volatile uint64_t *dst = (uint64_t *)link;
> > +
> > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > +
> > +	/* Note: this should never fail since both destination and expected
> > +	 * values are the same and are a pointer from caller.
> > +	 */
> > +	rte_atomic64_cmpset(dst, *dst, *src);
> > +}
> > +
> > +int
> >  rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)  {
> >  	struct rte_eth_dev *dev;
> > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> > f6837278521c..974657933f23 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> > rte_eth_link *link);
> >   */
> >  void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> > 
> > +
> > +/**
> > + * @internal
> > + * Atomically write the link status for the specific device.
> > + * It is for use by DPDK device driver use only.
> > + * User applications should not call it
> > + *
> > + * @param dev
> > + *  Pointer to struct rte_eth_dev.
> > + * @param link
> > + *  New link status value.
> > + * @return
> > + *  -1 if link state has changed, 0 if the same.
> > + */
> > +int _rte_eth_link_update(struct rte_eth_dev *dev,
> > +			 const struct rte_eth_link *link);
> > +  
> This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
> Use name link_update makes me confused, and mix up it with dev_ops link_update.
> > +/**
> > + * @internal
> > + * Atomically read the link speed and status.
> > + * @param dev
> > + *  Pointer to struct rte_eth_dev.
> > + * @param link
> > + *  link status value.
> > + */
> > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > +			struct rte_eth_link *link);
> > +  
> This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
> >  /**
> >   * Retrieve the general I/O statistics of an Ethernet device.
> >   *
> > --
> > 2.11.0  
> 

The first set of  patches was just  trying to combine multiple copies of same code.
Every place  was doing same thing for atomic update.

Re: [RFC 01/14] ethdev: add link status read/write functions

[Thomas Monjalon]

Stephen,
Qiming was suggesting a name change for the functions.
What do you think?

13/10/2017 17:12, Stephen Hemminger:
> On Wed, 11 Oct 2017 08:32:12 +0000
> "Yang, Qiming" <qiming.yang@intel.com> wrote:
> 
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Saturday, July 15, 2017 2:30 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> > > <sthemmin@microsoft.com>
> > > Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> > > 
> > > Many drivers are all doing copy/paste of the same code to atomicly update the
> > > link status. Reduce duplication, and allow for future changes by having common
> > > function for this.
> > > 
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> > >  2 files changed, 64 insertions(+)
> > > 
> > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index
> > > a1b744704f3a..7532fc6b65f0 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct
> > > rte_eth_link *eth_link)  }
> > > 
> > >  int
> > > +_rte_eth_link_update(struct rte_eth_dev *dev,
> > > +		    const struct rte_eth_link *link)
> > > +{
> > > +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> > > +	struct rte_eth_link old;
> > > +
> > > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > > +
> > > +	old = *dev_link;
> > > +
> > > +	/* Only reason we use cmpset rather than set is
> > > +	 * that on some architecture may use sign bit as a flag value.
> > > +	 */
> > > +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > > +				    *(volatile uint64_t *)dev_link,
> > > +				   *(const uint64_t *)link) == 0)
> > > +		continue;
> > > +
> > > +	return (old.link_status == link->link_status) ? -1 : 0; }
> > > +
> > > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > > +			struct rte_eth_link *link)
> > > +{
> > > +	const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> > > +	volatile uint64_t *dst = (uint64_t *)link;
> > > +
> > > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > > +
> > > +	/* Note: this should never fail since both destination and expected
> > > +	 * values are the same and are a pointer from caller.
> > > +	 */
> > > +	rte_atomic64_cmpset(dst, *dst, *src);
> > > +}
> > > +
> > > +int
> > >  rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)  {
> > >  	struct rte_eth_dev *dev;
> > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> > > f6837278521c..974657933f23 100644
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> > > rte_eth_link *link);
> > >   */
> > >  void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> > > 
> > > +
> > > +/**
> > > + * @internal
> > > + * Atomically write the link status for the specific device.
> > > + * It is for use by DPDK device driver use only.
> > > + * User applications should not call it
> > > + *
> > > + * @param dev
> > > + *  Pointer to struct rte_eth_dev.
> > > + * @param link
> > > + *  New link status value.
> > > + * @return
> > > + *  -1 if link state has changed, 0 if the same.
> > > + */
> > > +int _rte_eth_link_update(struct rte_eth_dev *dev,
> > > +			 const struct rte_eth_link *link);
> > > +  
> > This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
> > Use name link_update makes me confused, and mix up it with dev_ops link_update.
> > > +/**
> > > + * @internal
> > > + * Atomically read the link speed and status.
> > > + * @param dev
> > > + *  Pointer to struct rte_eth_dev.
> > > + * @param link
> > > + *  link status value.
> > > + */
> > > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > > +			struct rte_eth_link *link);
> > > +  
> > This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
> > >  /**
> > >   * Retrieve the general I/O statistics of an Ethernet device.
> > >   *
> > > --
> > > 2.11.0  
> > 
> 
> The first set of  patches was just  trying to combine multiple copies of same code.
> Every place  was doing same thing for atomic update.

Re: [RFC 00/14] link status API improvement and bugfixes

[Thomas Monjalon]

14/07/2017 20:30, Stephen Hemminger:
> While writing new driver, I 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 several drivers have
> bugs in this are because they don't return the correct value.
> Also, some drivers were not fully initializing all the space (including
> padding).

Good initiative.
Please, could you spin a new version not RFC?

> This is compile test only at this point, I don't have any of the hardware
> available (except virtio) to test.

I think we should integrate it in 18.02-rc1 so everybody will be forced
to test it.

Re: [RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Thomas Monjalon]

17/07/2017 18:28, Stephen Hemminger:
> On Mon, 17 Jul 2017 19:14:16 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> > On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> > > On Sun, 16 Jul 2017 15:33:26 +0300
> > > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> > >  
> > >>> +	link.link_autoneg = ETH_LINK_SPEED_FIXED;  
> > >> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. 0/false - no autoneg, 1/true - autoneg.
> > >> It looks like it has wrong comment:
> > >>           uint16_t link_autoneg : 1;  /**< ETH_LINK_SPEED_[AUTONEG/FIXED] */
> > >>
> > >> since
> > >> #define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds) */
> > >> #define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed speed) */
> > >>
> > >> whereas
> > >> #define ETH_LINK_FIXED          0 /**< No autonegotiation. */
> > >> #define ETH_LINK_AUTONEG        1 /**< Autonegotiated. */
> > >>
> > >> In general this attempt to introduce bug is the result of wrong comment which is caused by very similar
> > >> defines with opposite values.  
> > > Orignal observation was because some drivers (vmxnet3) were setting autoneg = fixed
> > > and others were not. Turns out it makes no difference
> > > since FIXED == 0, the old code and new code have same effect.  
> > 
> > May be I miss something, but  ETH_LINK_SPEED_FIXED==1, but 
> > ETH_LINK_FIXED==0.
> > So, initially it was 0 (fixed speed), but now it is 1 (autoneg).
> 
> My understanding is that ETH_SPEED_XXX and ETH_LINK_FIXED are for rte_eth_link
> structure; and ETH_LINK_SPEED_XXX values are for dev_info->speed_capa

Right.
The comment is wrong.
rte_eth_link.link_autoneg must uses ETH_LINK_[FIXED/AUTONEG]
ETH_LINK_SPEED_[AUTONEG/FIXED] is for rte_eth_conf.link_speeds
and for rte_eth_dev_info.speed_capa.

So in this patch, you should set link.link_autoneg = ETH_LINK_FIXED.

I will send a patch to fix the comment in ethdev.

Re: [RFC 01/14] ethdev: add link status read/write functions

[Stephen Hemminger]

On Fri, 05 Jan 2018 15:24:48 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> Stephen,
> Qiming was suggesting a name change for the functions.
> What do you think?
> 
> 13/10/2017 17:12, Stephen Hemminger:
> > On Wed, 11 Oct 2017 08:32:12 +0000
> > "Yang, Qiming" <qiming.yang@intel.com> wrote:
> >   
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > > Sent: Saturday, July 15, 2017 2:30 AM
> > > > To: dev@dpdk.org
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> > > > <sthemmin@microsoft.com>
> > > > Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> > > > 
> > > > Many drivers are all doing copy/paste of the same code to atomicly update the
> > > > link status. Reduce duplication, and allow for future changes by having common
> > > > function for this.
> > > > 
> > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > > ---
> > > >  lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> > > >  lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> > > >  2 files changed, 64 insertions(+)
> > > > 
> > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index
> > > > a1b744704f3a..7532fc6b65f0 100644
> > > > --- a/lib/librte_ether/rte_ethdev.c
> > > > +++ b/lib/librte_ether/rte_ethdev.c
> > > > @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct
> > > > rte_eth_link *eth_link)  }
> > > > 
> > > >  int
> > > > +_rte_eth_link_update(struct rte_eth_dev *dev,
> > > > +		    const struct rte_eth_link *link)
> > > > +{
> > > > +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> > > > +	struct rte_eth_link old;
> > > > +
> > > > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > > > +
> > > > +	old = *dev_link;
> > > > +
> > > > +	/* Only reason we use cmpset rather than set is
> > > > +	 * that on some architecture may use sign bit as a flag value.
> > > > +	 */
> > > > +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > > > +				    *(volatile uint64_t *)dev_link,
> > > > +				   *(const uint64_t *)link) == 0)
> > > > +		continue;
> > > > +
> > > > +	return (old.link_status == link->link_status) ? -1 : 0; }
> > > > +
> > > > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > > > +			struct rte_eth_link *link)
> > > > +{
> > > > +	const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> > > > +	volatile uint64_t *dst = (uint64_t *)link;
> > > > +
> > > > +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > > > +
> > > > +	/* Note: this should never fail since both destination and expected
> > > > +	 * values are the same and are a pointer from caller.
> > > > +	 */
> > > > +	rte_atomic64_cmpset(dst, *dst, *src);
> > > > +}
> > > > +
> > > > +int
> > > >  rte_eth_stats_get(uint8_t port_id, struct rte_eth_stats *stats)  {
> > > >  	struct rte_eth_dev *dev;
> > > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> > > > f6837278521c..974657933f23 100644
> > > > --- a/lib/librte_ether/rte_ethdev.h
> > > > +++ b/lib/librte_ether/rte_ethdev.h
> > > > @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> > > > rte_eth_link *link);
> > > >   */
> > > >  void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> > > > 
> > > > +
> > > > +/**
> > > > + * @internal
> > > > + * Atomically write the link status for the specific device.
> > > > + * It is for use by DPDK device driver use only.
> > > > + * User applications should not call it
> > > > + *
> > > > + * @param dev
> > > > + *  Pointer to struct rte_eth_dev.
> > > > + * @param link
> > > > + *  New link status value.
> > > > + * @return
> > > > + *  -1 if link state has changed, 0 if the same.
> > > > + */
> > > > +int _rte_eth_link_update(struct rte_eth_dev *dev,
> > > > +			 const struct rte_eth_link *link);
> > > > +    
> > > This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
> > > Use name link_update makes me confused, and mix up it with dev_ops link_update.  
> > > > +/**
> > > > + * @internal
> > > > + * Atomically read the link speed and status.
> > > > + * @param dev
> > > > + *  Pointer to struct rte_eth_dev.
> > > > + * @param link
> > > > + *  link status value.
> > > > + */
> > > > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > > > +			struct rte_eth_link *link);
> > > > +    
> > > This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.  
> > > >  /**
> > > >   * Retrieve the general I/O statistics of an Ethernet device.
> > > >   *
> > > > --
> > > > 2.11.0    
> > >   
> > 
> > The first set of  patches was just  trying to combine multiple copies of same code.
> > Every place  was doing same thing for atomic update.  

I would just change the name to linkstatsus update.
Also since writes of unsigned long are guaranteed atomic, the code could
be optimized on 64bit platforms.

Re: [RFC 00/14] link status API improvement and bugfixes

[Stephen Hemminger]

On Fri, 05 Jan 2018 15:29:09 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 14/07/2017 20:30, Stephen Hemminger:
> > While writing new driver, I 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 several drivers have
> > bugs in this are because they don't return the correct value.
> > Also, some drivers were not fully initializing all the space (including
> > padding).  
> 
> Good initiative.
> Please, could you spin a new version not RFC?
> 
> > This is compile test only at this point, I don't have any of the hardware
> > available (except virtio) to test.  
> 
> I think we should integrate it in 18.02-rc1 so everybody will be forced
> to test it.
> 

Let's change the name then to.
  rte_eth_linkstatus_set
  rte_eth_linkstatus_get