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

[Andrew Rybchenko]

On 01/09/2018 07:27 PM, Stephen Hemminger wrote:

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

On 01/08/2018 08:45 PM, Stephen Hemminger wrote:

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

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

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 2f5f86f84877..e0a12b32b1a3 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -238,22 +238,12 @@ static int
   sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
   {
        struct sfc_adapter *sa = dev->data->dev_private;
-       struct rte_eth_link *dev_link = &dev->data->dev_link;
-       struct rte_eth_link old_link;
        struct rte_eth_link current_link;

sfc_log_init(sa, "entry"); -retry:

-       EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));
-       *(int64_t *)&old_link = rte_atomic64_read((rte_atomic64_t *)dev_link);
-
        if (sa->state != SFC_ADAPTER_STARTED) {
                sfc_port_link_mode_to_info(EFX_LINK_UNKNOWN, &current_link);
-               if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,
-                                        *(uint64_t *)&old_link,
-                                        *(uint64_t *)&current_link))
-                       goto retry;
        } else if (wait_to_complete) {
                efx_link_mode_t link_mode;

@@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)

                        link_mode = EFX_LINK_UNKNOWN;
                sfc_port_link_mode_to_info(link_mode, &current_link);

- if (!rte_atomic64_cmpset((volatile uint64_t *)dev_link,

-                                        *(uint64_t *)&old_link,
-                                        *(uint64_t *)&current_link))
-                       goto retry;
        } else {
                sfc_ev_mgmt_qpoll(sa);
-               *(int64_t *)&current_link =
-                       rte_atomic64_read((rte_atomic64_t *)dev_link);
+               _rte_eth_linkstatus_get(dev, &current_link);
        }

- if (old_link.link_status != current_link.link_status)

-               sfc_info(sa, "Link status is %s",
-                        current_link.link_status ? "UP" : "DOWN");
+       if (_rte_eth_linkstatus_set(dev, &current_link) == 0)
+               return 0;
+
+       sfc_info(sa, "Link status is %s",
+                current_link.link_status ? "UP" : "DOWN");

- return old_link.link_status == current_link.link_status ? 0 : -1;

+       return -1;
   }

static void

diff --git a/drivers/net/sfc/sfc_ev.c b/drivers/net/sfc/sfc_ev.c
index a16dc27b380e..3e96536a9d60 100644
--- a/drivers/net/sfc/sfc_ev.c
+++ b/drivers/net/sfc/sfc_ev.c
@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode)
   {
        struct sfc_evq *evq = arg;
        struct sfc_adapter *sa = evq->sa;
-       struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;
        struct rte_eth_link new_link;
-       uint64_t new_link_u64;
-       uint64_t old_link_u64;
-
-       EFX_STATIC_ASSERT(sizeof(*dev_link) == sizeof(rte_atomic64_t));

sfc_port_link_mode_to_info(link_mode, &new_link);

+       if (_rte_eth_linkstatus_set(sa->eth_dev, &new_link) == 0)
+               return B_FALSE;

- new_link_u64 = *(uint64_t *)&new_link;

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

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

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

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