[PATCH net 0/5] net/ncsi: More bug fixes

[PATCH net 0/5] net/ncsi: More bug fixes

[Gavin Shan]

This series fixes 2 issues that were found during NCSI's availability
testing on BCM5718 and improves HNCDSC AEN handler:

   * PATCH[1] and PATCH[2] refactors the code so that minimal code
     change is put to PATCH[3].
   * PATCH[3] fixes the NCSI channel's stale link state before doing
     failover.
   * PATCH[4] chooses the hot channel, which was ever chosen as active
     channel, when the available channels are all in link-down state.
   * PATCH[5] improves Host Network Controller Driver Status Change
     (HNCDSC) AEN handler

Gavin Shan (5):
  net/ncsi: Not fetch active package and channel again
  net/ncsi: Split out logic for ncsi_dev_state_suspend_select
  net/ncsi: Fix stale link state of inactive channels on failover
  net/ncsi: Choose hot channel as active one if necessary
  net/ncsi: Improve HNCDSC AEN handler

 net/ncsi/internal.h    |  2 ++
 net/ncsi/ncsi-aen.c    | 18 ++++++++++--
 net/ncsi/ncsi-manage.c | 80 ++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 79 insertions(+), 21 deletions(-)

-- 
2.1.0

[PATCH net 1/5] net/ncsi: Not fetch active package and channel again

[Gavin Shan]

In ncsi_suspend_channel(), we fetch the active package and
channel to local variables for twice. It's unnecessary. This
drops one of them. No functional changes introduced.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-manage.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 5e509e5..1bc96dc 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -545,8 +545,6 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
 	case ncsi_dev_state_suspend_deselect:
 		ndp->pending_req_num = 1;
 
-		np = ndp->active_package;
-		nc = ndp->active_channel;
 		nca.package = np->id;
 		if (nd->state == ncsi_dev_state_suspend_select) {
 			nca.type = NCSI_PKT_CMD_SP;
-- 
2.1.0

[PATCH net 2/5] net/ncsi: Split out logic for ncsi_dev_state_suspend_select

[Gavin Shan]

This splits out the code that handles ncsi_dev_state_suspend_select
so that we can add more code to the handler in subsequent patch.
Apart from adding a error tag to reuse the code in error path,
no logical changes introduced.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-manage.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 1bc96dc..5758a26 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -540,21 +540,30 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
 		nd->state = ncsi_dev_state_suspend_select;
 		/* Fall through */
 	case ncsi_dev_state_suspend_select:
+		ndp->pending_req_num = 1;
+
+		nca.type = NCSI_PKT_CMD_SP;
+		nca.package = np->id;
+		nca.channel = NCSI_RESERVED_CHANNEL;
+		if (ndp->flags & NCSI_DEV_HWA)
+			nca.bytes[0] = 0;
+		else
+			nca.bytes[0] = 1;
+
+		nd->state = ncsi_dev_state_suspend_dcnt;
+
+		ret = ncsi_xmit_cmd(&nca);
+		if (ret)
+			goto error;
+
+		break;
 	case ncsi_dev_state_suspend_dcnt:
 	case ncsi_dev_state_suspend_dc:
 	case ncsi_dev_state_suspend_deselect:
 		ndp->pending_req_num = 1;
 
 		nca.package = np->id;
-		if (nd->state == ncsi_dev_state_suspend_select) {
-			nca.type = NCSI_PKT_CMD_SP;
-			nca.channel = NCSI_RESERVED_CHANNEL;
-			if (ndp->flags & NCSI_DEV_HWA)
-				nca.bytes[0] = 0;
-			else
-				nca.bytes[0] = 1;
-			nd->state = ncsi_dev_state_suspend_dcnt;
-		} else if (nd->state == ncsi_dev_state_suspend_dcnt) {
+		if (nd->state == ncsi_dev_state_suspend_dcnt) {
 			nca.type = NCSI_PKT_CMD_DCNT;
 			nca.channel = nc->id;
 			nd->state = ncsi_dev_state_suspend_dc;
@@ -570,10 +579,8 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
 		}
 
 		ret = ncsi_xmit_cmd(&nca);
-		if (ret) {
-			nd->state = ncsi_dev_state_functional;
-			return;
-		}
+		if (ret)
+			goto error;
 
 		break;
 	case ncsi_dev_state_suspend_done:
@@ -587,6 +594,11 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
 		netdev_warn(nd->dev, "Wrong NCSI state 0x%x in suspend\n",
 			    nd->state);
 	}
+
+	return;
+
+error:
+	nd->state = ncsi_dev_state_functional;
 }
 
 static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
-- 
2.1.0

[PATCH net 3/5] net/ncsi: Fix stale link state of inactive channels on failover

[Gavin Shan]

The issue was found on BCM5718 which has two NCSI channels in one
package: C0 and C1. Both of them are connected to different LANs,
means they are in link-up state and C0 is chosen  as the active
one until resetting BCM5718 happens as below.

Resetting BCM5718 results in LSC (Link State Change) AEN packet
received on C0, meaning LSC AEN is missed on C1. When LSC AEN packet
received on C0 to report link-down, it fails over to C1 because C1
is in link-up state as software can see. However, C1 is in link-down
state in hardware. It means the link state is out of synchronization
between hardware and software, resulting in inappropriate channel (C1)
selected as active one.

This resolves the issue by sending separate GLS (Get Link Status)
commands to all channels in the package before trying to do failover.
The last link state on all channels in the package is retrieved. With
it, C0 is selected as active one as expected.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/internal.h    |  1 +
 net/ncsi/ncsi-manage.c | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 13290a7..eac4858 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -246,6 +246,7 @@ enum {
 	ncsi_dev_state_config_gls,
 	ncsi_dev_state_config_done,
 	ncsi_dev_state_suspend_select	= 0x0401,
+	ncsi_dev_state_suspend_gls,
 	ncsi_dev_state_suspend_dcnt,
 	ncsi_dev_state_suspend_dc,
 	ncsi_dev_state_suspend_deselect,
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 5758a26..e959979 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -550,13 +550,31 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
 		else
 			nca.bytes[0] = 1;
 
-		nd->state = ncsi_dev_state_suspend_dcnt;
+		if (ndp->flags & NCSI_DEV_RESHUFFLE)
+			nd->state = ncsi_dev_state_suspend_gls;
+		else
+			nd->state = ncsi_dev_state_suspend_dcnt;
 
 		ret = ncsi_xmit_cmd(&nca);
 		if (ret)
 			goto error;
 
 		break;
+	case ncsi_dev_state_suspend_gls:
+		ndp->pending_req_num = np->channel_num;
+
+		nca.type = NCSI_PKT_CMD_GLS;
+		nca.package = np->id;
+		nd->state = ncsi_dev_state_suspend_dcnt;
+
+		NCSI_FOR_EACH_CHANNEL(np, nc) {
+			nca.channel = nc->id;
+			ret = ncsi_xmit_cmd(&nca);
+			if (ret)
+				goto error;
+		}
+
+		break;
 	case ncsi_dev_state_suspend_dcnt:
 	case ncsi_dev_state_suspend_dc:
 	case ncsi_dev_state_suspend_deselect:
-- 
2.1.0

[PATCH net 4/5] net/ncsi: Choose hot channel as active one if necessary

[Gavin Shan]

The issue was found on BCM5718 which has two NCSI channels in one
package: C0 and C1. C0 is in link-up state while C1 is in link-down
state. C0 is chosen as active channel until unplugging and plugging
C0's cable:  On unplugging C0's cable, LSC (Link State Change) AEN
packet received on C0 to report link-down event. After that, C1 is
chosen as active channel. LSC AEN for link-up event is lost on C0
when plugging C0's cable back. We lose the network even C0 is usable.

This resolves the issue by recording the (hot) channel that was ever
chosen as active one. The hot channel is chosen to be active one
if none of available channels in link-up state. With this, C0 is still
the active one after unplugging C0's cable. LSC AEN packet received
on C0 when plugging its cable back.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/internal.h    |  1 +
 net/ncsi/ncsi-manage.c | 22 +++++++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index eac4858..1308a56 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -265,6 +265,7 @@ struct ncsi_dev_priv {
 #endif
 	unsigned int        package_num;     /* Number of packages         */
 	struct list_head    packages;        /* List of packages           */
+	struct ncsi_channel *hot_channel;    /* Channel was ever active    */
 	struct ncsi_request requests[256];   /* Request table              */
 	unsigned int        request_id;      /* Last used request ID       */
 #define NCSI_REQ_START_IDX	1
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index e959979..cccedcf 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -625,6 +625,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 	struct net_device *dev = nd->dev;
 	struct ncsi_package *np = ndp->active_package;
 	struct ncsi_channel *nc = ndp->active_channel;
+	struct ncsi_channel *hot_nc = NULL;
 	struct ncsi_cmd_arg nca;
 	unsigned char index;
 	unsigned long flags;
@@ -730,12 +731,20 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 		break;
 	case ncsi_dev_state_config_done:
 		spin_lock_irqsave(&nc->lock, flags);
-		if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1)
+		if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1) {
+			hot_nc = nc;
 			nc->state = NCSI_CHANNEL_ACTIVE;
-		else
+		} else {
+			hot_nc = NULL;
 			nc->state = NCSI_CHANNEL_INACTIVE;
+		}
 		spin_unlock_irqrestore(&nc->lock, flags);
 
+		/* Update the hot channel */
+		spin_lock_irqsave(&ndp->lock, flags);
+		ndp->hot_channel = hot_nc;
+		spin_unlock_irqrestore(&ndp->lock, flags);
+
 		ncsi_start_channel_monitor(nc);
 		ncsi_process_next_channel(ndp);
 		break;
@@ -753,10 +762,14 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
 {
 	struct ncsi_package *np;
-	struct ncsi_channel *nc, *found;
+	struct ncsi_channel *nc, *found, *hot_nc;
 	struct ncsi_channel_mode *ncm;
 	unsigned long flags;
 
+	spin_lock_irqsave(&ndp->lock, flags);
+	hot_nc = ndp->hot_channel;
+	spin_unlock_irqrestore(&ndp->lock, flags);
+
 	/* The search is done once an inactive channel with up
 	 * link is found.
 	 */
@@ -774,6 +787,9 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
 			if (!found)
 				found = nc;
 
+			if (nc == hot_nc)
+				found = nc;
+
 			ncm = &nc->modes[NCSI_MODE_LINK];
 			if (ncm->data[2] & 0x1) {
 				spin_unlock_irqrestore(&nc->lock, flags);
-- 
2.1.0

[PATCH net 5/5] net/ncsi: Improve HNCDSC AEN handler

[Gavin Shan]

This improves AEN handler for Host Network Controller Driver Status
Change (HNCDSC):

   * The channel's lock should be hold when accessing its state.
   * Do failover when host driver isn't ready.
   * Configure channel when host driver becomes ready.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-aen.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index b41a661..6898e72 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -141,23 +141,35 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
 		return -ENODEV;
 
 	/* If the channel is active one, we need reconfigure it */
+	spin_lock_irqsave(&nc->lock, flags);
 	ncm = &nc->modes[NCSI_MODE_LINK];
 	hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
 	ncm->data[3] = ntohl(hncdsc->status);
 	if (!list_empty(&nc->link) ||
-	    nc->state != NCSI_CHANNEL_ACTIVE ||
-	    (ncm->data[3] & 0x1))
+	    nc->state != NCSI_CHANNEL_ACTIVE) {
+		spin_unlock_irqrestore(&nc->lock, flags);
 		return 0;
+	}
 
-	if (ndp->flags & NCSI_DEV_HWA)
+	spin_unlock_irqrestore(&nc->lock, flags);
+	if (!(ndp->flags & NCSI_DEV_HWA) && !(ncm->data[3] & 0x1))
 		ndp->flags |= NCSI_DEV_RESHUFFLE;
 
 	/* If this channel is the active one and the link doesn't
 	 * work, we have to choose another channel to be active one.
 	 * The logic here is exactly similar to what we do when link
 	 * is down on the active channel.
+	 *
+	 * On the other hand, we need configure it when host driver
+	 * state on the active channel becomes ready.
 	 */
 	ncsi_stop_channel_monitor(nc);
+
+	spin_lock_irqsave(&nc->lock, flags);
+	nc->state = (ncm->data[3] & 0x1) ? NCSI_CHANNEL_INACTIVE :
+					   NCSI_CHANNEL_ACTIVE;
+	spin_unlock_irqrestore(&nc->lock, flags);
+
 	spin_lock_irqsave(&ndp->lock, flags);
 	list_add_tail_rcu(&nc->link, &ndp->channel_queue);
 	spin_unlock_irqrestore(&ndp->lock, flags);
-- 
2.1.0

Re: [PATCH net 2/5] net/ncsi: Split out logic for ncsi_dev_state_suspend_select

[Joel Stanley]

Hi Gavin,

On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> This splits out the code that handles ncsi_dev_state_suspend_select
> so that we can add more code to the handler in subsequent patch.
> Apart from adding a error tag to reuse the code in error path,
> no logical changes introduced.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  net/ncsi/ncsi-manage.c | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 1bc96dc..5758a26 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -540,21 +540,30 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>                 nd->state = ncsi_dev_state_suspend_select;
>                 /* Fall through */
>         case ncsi_dev_state_suspend_select:
> +               ndp->pending_req_num = 1;
> +
> +               nca.type = NCSI_PKT_CMD_SP;
> +               nca.package = np->id;
> +               nca.channel = NCSI_RESERVED_CHANNEL;
> +               if (ndp->flags & NCSI_DEV_HWA)
> +                       nca.bytes[0] = 0;
> +               else
> +                       nca.bytes[0] = 1;
> +
> +               nd->state = ncsi_dev_state_suspend_dcnt;
> +
> +               ret = ncsi_xmit_cmd(&nca);
> +               if (ret)
> +                       goto error;
> +
> +               break;
>         case ncsi_dev_state_suspend_dcnt:
>         case ncsi_dev_state_suspend_dc:
>         case ncsi_dev_state_suspend_deselect:
>                 ndp->pending_req_num = 1;
>
>                 nca.package = np->id;
> -               if (nd->state == ncsi_dev_state_suspend_select) {
> -                       nca.type = NCSI_PKT_CMD_SP;
> -                       nca.channel = NCSI_RESERVED_CHANNEL;
> -                       if (ndp->flags & NCSI_DEV_HWA)
> -                               nca.bytes[0] = 0;
> -                       else
> -                               nca.bytes[0] = 1;
> -                       nd->state = ncsi_dev_state_suspend_dcnt;
> -               } else if (nd->state == ncsi_dev_state_suspend_dcnt) {
> +               if (nd->state == ncsi_dev_state_suspend_dcnt) {
>                         nca.type = NCSI_PKT_CMD_DCNT;
>                         nca.channel = nc->id;
>                         nd->state = ncsi_dev_state_suspend_dc;

This is a messy switch statement. How about break out out all of the
states as you've done with suspend_select, instead of grouping them
and then doing if ... else if .. else if. I realise there might be one
or two lines duplicated for each state, but I think that's okay at the
expense of readability.

Also, patch 1 could also be merged into this when making this cleanup.

What do you think?

> @@ -570,10 +579,8 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>                 }
>
>                 ret = ncsi_xmit_cmd(&nca);
> -               if (ret) {
> -                       nd->state = ncsi_dev_state_functional;
> -                       return;
> -               }
> +               if (ret)
> +                       goto error;
>
>                 break;
>         case ncsi_dev_state_suspend_done:
> @@ -587,6 +594,11 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>                 netdev_warn(nd->dev, "Wrong NCSI state 0x%x in suspend\n",
>                             nd->state);
>         }
> +
> +       return;
> +
> +error:
> +       nd->state = ncsi_dev_state_functional;
>  }
>
>  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> --
> 2.1.0
>

Re: [PATCH net 3/5] net/ncsi: Fix stale link state of inactive channels on failover

[Joel Stanley]

On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> The issue was found on BCM5718 which has two NCSI channels in one
> package: C0 and C1. Both of them are connected to different LANs,
> means they are in link-up state and C0 is chosen  as the active
> one until resetting BCM5718 happens as below.
>
> Resetting BCM5718 results in LSC (Link State Change) AEN packet
> received on C0, meaning LSC AEN is missed on C1. When LSC AEN packet
> received on C0 to report link-down, it fails over to C1 because C1
> is in link-up state as software can see. However, C1 is in link-down
> state in hardware. It means the link state is out of synchronization
> between hardware and software, resulting in inappropriate channel (C1)
> selected as active one.
>
> This resolves the issue by sending separate GLS (Get Link Status)
> commands to all channels in the package before trying to do failover.
> The last link state on all channels in the package is retrieved. With
> it, C0 is selected as active one as expected.

I follow this, and can see that happening in the
ncsi_dev_state_suspend_gls state. However, what is

> -               nd->state = ncsi_dev_state_suspend_dcnt;
> +               if (ndp->flags & NCSI_DEV_RESHUFFLE)
> +                       nd->state = ncsi_dev_state_suspend_gls;
> +               else
> +                       nd->state = ncsi_dev_state_suspend_dcnt;

However, what is this doing? I'm not quite sure what
NCSI_DEV_RESHUFFLE is and why we enable it?

>
>                 ret = ncsi_xmit_cmd(&nca);
>                 if (ret)
>                         goto error;
>
>                 break;
> +       case ncsi_dev_state_suspend_gls:
> +               ndp->pending_req_num = np->channel_num;
> +
> +               nca.type = NCSI_PKT_CMD_GLS;
> +               nca.package = np->id;
> +               nd->state = ncsi_dev_state_suspend_dcnt;
> +
> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
> +                       nca.channel = nc->id;
> +                       ret = ncsi_xmit_cmd(&nca);
> +                       if (ret)
> +                               goto error;
> +               }
> +
> +               break;
>         case ncsi_dev_state_suspend_dcnt:
>         case ncsi_dev_state_suspend_dc:
>         case ncsi_dev_state_suspend_deselect:
> --
> 2.1.0
>

Re: [PATCH net 4/5] net/ncsi: Choose hot channel as active one if necessary

[Joel Stanley]

On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
> The issue was found on BCM5718 which has two NCSI channels in one
> package: C0 and C1. C0 is in link-up state while C1 is in link-down
> state. C0 is chosen as active channel until unplugging and plugging
> C0's cable:  On unplugging C0's cable, LSC (Link State Change) AEN
> packet received on C0 to report link-down event. After that, C1 is
> chosen as active channel. LSC AEN for link-up event is lost on C0
> when plugging C0's cable back. We lose the network even C0 is usable.

Why do we lose the LCS AEN packet?

Is this a bug in the BCM5718? If so, we shouldn't put it in the common
ncsi code without adding a quirk for that hardware.

>
> This resolves the issue by recording the (hot) channel that was ever
> chosen as active one. The hot channel is chosen to be active one
> if none of available channels in link-up state. With this, C0 is still
> the active one after unplugging C0's cable. LSC AEN packet received
> on C0 when plugging its cable back.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  net/ncsi/internal.h    |  1 +
>  net/ncsi/ncsi-manage.c | 22 +++++++++++++++++++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index eac4858..1308a56 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -265,6 +265,7 @@ struct ncsi_dev_priv {
>  #endif
>         unsigned int        package_num;     /* Number of packages         */
>         struct list_head    packages;        /* List of packages           */
> +       struct ncsi_channel *hot_channel;    /* Channel was ever active    */
>         struct ncsi_request requests[256];   /* Request table              */
>         unsigned int        request_id;      /* Last used request ID       */
>  #define NCSI_REQ_START_IDX     1
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index e959979..cccedcf 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -625,6 +625,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>         struct net_device *dev = nd->dev;
>         struct ncsi_package *np = ndp->active_package;
>         struct ncsi_channel *nc = ndp->active_channel;
> +       struct ncsi_channel *hot_nc = NULL;
>         struct ncsi_cmd_arg nca;
>         unsigned char index;
>         unsigned long flags;
> @@ -730,12 +731,20 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>                 break;
>         case ncsi_dev_state_config_done:
>                 spin_lock_irqsave(&nc->lock, flags);
> -               if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1)
> +               if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1) {
> +                       hot_nc = nc;
>                         nc->state = NCSI_CHANNEL_ACTIVE;
> -               else
> +               } else {
> +                       hot_nc = NULL;
>                         nc->state = NCSI_CHANNEL_INACTIVE;
> +               }
>                 spin_unlock_irqrestore(&nc->lock, flags);
>
> +               /* Update the hot channel */
> +               spin_lock_irqsave(&ndp->lock, flags);
> +               ndp->hot_channel = hot_nc;
> +               spin_unlock_irqrestore(&ndp->lock, flags);
> +
>                 ncsi_start_channel_monitor(nc);
>                 ncsi_process_next_channel(ndp);
>                 break;
> @@ -753,10 +762,14 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>  static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>  {
>         struct ncsi_package *np;
> -       struct ncsi_channel *nc, *found;
> +       struct ncsi_channel *nc, *found, *hot_nc;
>         struct ncsi_channel_mode *ncm;
>         unsigned long flags;
>
> +       spin_lock_irqsave(&ndp->lock, flags);
> +       hot_nc = ndp->hot_channel;
> +       spin_unlock_irqrestore(&ndp->lock, flags);
> +
>         /* The search is done once an inactive channel with up
>          * link is found.
>          */
> @@ -774,6 +787,9 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>                         if (!found)
>                                 found = nc;
>
> +                       if (nc == hot_nc)
> +                               found = nc;
> +
>                         ncm = &nc->modes[NCSI_MODE_LINK];
>                         if (ncm->data[2] & 0x1) {
>                                 spin_unlock_irqrestore(&nc->lock, flags);
> --
> 2.1.0
>

Re: [PATCH net 2/5] net/ncsi: Split out logic for ncsi_dev_state_suspend_select

[Gavin Shan]

On Fri, Oct 14, 2016 at 04:32:22PM +1030, Joel Stanley wrote:
>Hi Gavin,
>
>On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> This splits out the code that handles ncsi_dev_state_suspend_select
>> so that we can add more code to the handler in subsequent patch.
>> Apart from adding a error tag to reuse the code in error path,
>> no logical changes introduced.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  net/ncsi/ncsi-manage.c | 38 +++++++++++++++++++++++++-------------
>>  1 file changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>> index 1bc96dc..5758a26 100644
>> --- a/net/ncsi/ncsi-manage.c
>> +++ b/net/ncsi/ncsi-manage.c
>> @@ -540,21 +540,30 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>>                 nd->state = ncsi_dev_state_suspend_select;
>>                 /* Fall through */
>>         case ncsi_dev_state_suspend_select:
>> +               ndp->pending_req_num = 1;
>> +
>> +               nca.type = NCSI_PKT_CMD_SP;
>> +               nca.package = np->id;
>> +               nca.channel = NCSI_RESERVED_CHANNEL;
>> +               if (ndp->flags & NCSI_DEV_HWA)
>> +                       nca.bytes[0] = 0;
>> +               else
>> +                       nca.bytes[0] = 1;
>> +
>> +               nd->state = ncsi_dev_state_suspend_dcnt;
>> +
>> +               ret = ncsi_xmit_cmd(&nca);
>> +               if (ret)
>> +                       goto error;
>> +
>> +               break;
>>         case ncsi_dev_state_suspend_dcnt:
>>         case ncsi_dev_state_suspend_dc:
>>         case ncsi_dev_state_suspend_deselect:
>>                 ndp->pending_req_num = 1;
>>
>>                 nca.package = np->id;
>> -               if (nd->state == ncsi_dev_state_suspend_select) {
>> -                       nca.type = NCSI_PKT_CMD_SP;
>> -                       nca.channel = NCSI_RESERVED_CHANNEL;
>> -                       if (ndp->flags & NCSI_DEV_HWA)
>> -                               nca.bytes[0] = 0;
>> -                       else
>> -                               nca.bytes[0] = 1;
>> -                       nd->state = ncsi_dev_state_suspend_dcnt;
>> -               } else if (nd->state == ncsi_dev_state_suspend_dcnt) {
>> +               if (nd->state == ncsi_dev_state_suspend_dcnt) {
>>                         nca.type = NCSI_PKT_CMD_DCNT;
>>                         nca.channel = nc->id;
>>                         nd->state = ncsi_dev_state_suspend_dc;
>
>This is a messy switch statement. How about break out out all of the
>states as you've done with suspend_select, instead of grouping them
>and then doing if ... else if .. else if. I realise there might be one
>or two lines duplicated for each state, but I think that's okay at the
>expense of readability.
>
>Also, patch 1 could also be merged into this when making this cleanup.
>
>What do you think?
>

Thanks, Joel. I agree with you that code readability is important than
duplicated code. I will do in next revision.

Thanks,
Gavin

>> @@ -570,10 +579,8 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>>                 }
>>
>>                 ret = ncsi_xmit_cmd(&nca);
>> -               if (ret) {
>> -                       nd->state = ncsi_dev_state_functional;
>> -                       return;
>> -               }
>> +               if (ret)
>> +                       goto error;
>>
>>                 break;
>>         case ncsi_dev_state_suspend_done:
>> @@ -587,6 +594,11 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
>>                 netdev_warn(nd->dev, "Wrong NCSI state 0x%x in suspend\n",
>>                             nd->state);
>>         }
>> +
>> +       return;
>> +
>> +error:
>> +       nd->state = ncsi_dev_state_functional;
>>  }
>>
>>  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>> --
>> 2.1.0
>>
>

Re: [PATCH net 3/5] net/ncsi: Fix stale link state of inactive channels on failover

[Gavin Shan]

On Fri, Oct 14, 2016 at 04:32:28PM +1030, Joel Stanley wrote:
>On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> The issue was found on BCM5718 which has two NCSI channels in one
>> package: C0 and C1. Both of them are connected to different LANs,
>> means they are in link-up state and C0 is chosen  as the active
>> one until resetting BCM5718 happens as below.
>>
>> Resetting BCM5718 results in LSC (Link State Change) AEN packet
>> received on C0, meaning LSC AEN is missed on C1. When LSC AEN packet
>> received on C0 to report link-down, it fails over to C1 because C1
>> is in link-up state as software can see. However, C1 is in link-down
>> state in hardware. It means the link state is out of synchronization
>> between hardware and software, resulting in inappropriate channel (C1)
>> selected as active one.
>>
>> This resolves the issue by sending separate GLS (Get Link Status)
>> commands to all channels in the package before trying to do failover.
>> The last link state on all channels in the package is retrieved. With
>> it, C0 is selected as active one as expected.
>
>I follow this, and can see that happening in the
>ncsi_dev_state_suspend_gls state. However, what is
>
>> -               nd->state = ncsi_dev_state_suspend_dcnt;
>> +               if (ndp->flags & NCSI_DEV_RESHUFFLE)
>> +                       nd->state = ncsi_dev_state_suspend_gls;
>> +               else
>> +                       nd->state = ncsi_dev_state_suspend_dcnt;
>
>However, what is this doing? I'm not quite sure what
>NCSI_DEV_RESHUFFLE is and why we enable it?
>

NCSI_DEV_RESHUFFLE is set when we need failover, which happens on
resetting NIC or unplugging the cable connected to the active channel
(port) or other events. The first step for failover is to suspend
currently active channel and then choose the best one (channel's link
state is important factor) to be active. ncsi_dev_state_suspend_gls
ensures we will get updated link state of available channels before
choosing and enabling next active channel. If there are no failover
happening, we needn't get the update link state on the available
channels and the state ncsi_dev_state_suspend_gls will be skipped.

I think I need put comments here to explain the change in next revision.

Thanks,
Gavin

>>
>>                 ret = ncsi_xmit_cmd(&nca);
>>                 if (ret)
>>                         goto error;
>>
>>                 break;
>> +       case ncsi_dev_state_suspend_gls:
>> +               ndp->pending_req_num = np->channel_num;
>> +
>> +               nca.type = NCSI_PKT_CMD_GLS;
>> +               nca.package = np->id;
>> +               nd->state = ncsi_dev_state_suspend_dcnt;
>> +
>> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
>> +                       nca.channel = nc->id;
>> +                       ret = ncsi_xmit_cmd(&nca);
>> +                       if (ret)
>> +                               goto error;
>> +               }
>> +
>> +               break;
>>         case ncsi_dev_state_suspend_dcnt:
>>         case ncsi_dev_state_suspend_dc:
>>         case ncsi_dev_state_suspend_deselect:
>> --
>> 2.1.0
>>
>

Re: [PATCH net 4/5] net/ncsi: Choose hot channel as active one if necessary

[Gavin Shan]

On Fri, Oct 14, 2016 at 04:32:36PM +1030, Joel Stanley wrote:
>On Fri, Oct 14, 2016 at 1:23 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> The issue was found on BCM5718 which has two NCSI channels in one
>> package: C0 and C1. C0 is in link-up state while C1 is in link-down
>> state. C0 is chosen as active channel until unplugging and plugging
>> C0's cable:  On unplugging C0's cable, LSC (Link State Change) AEN
>> packet received on C0 to report link-down event. After that, C1 is
>> chosen as active channel. LSC AEN for link-up event is lost on C0
>> when plugging C0's cable back. We lose the network even C0 is usable.
>
>Why do we lose the LCS AEN packet?
>
>Is this a bug in the BCM5718? If so, we shouldn't put it in the common
>ncsi code without adding a quirk for that hardware.
>

It's not a BCM5718 bug. LSC AEN is only received on the active channel.
After the failover (C0 -> C1), C0 becomes inactive and LSC AEN packet
won't be received on it as expected.

Thanks,
Gavin

>>
>> This resolves the issue by recording the (hot) channel that was ever
>> chosen as active one. The hot channel is chosen to be active one
>> if none of available channels in link-up state. With this, C0 is still
>> the active one after unplugging C0's cable. LSC AEN packet received
>> on C0 when plugging its cable back.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  net/ncsi/internal.h    |  1 +
>>  net/ncsi/ncsi-manage.c | 22 +++++++++++++++++++---
>>  2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
>> index eac4858..1308a56 100644
>> --- a/net/ncsi/internal.h
>> +++ b/net/ncsi/internal.h
>> @@ -265,6 +265,7 @@ struct ncsi_dev_priv {
>>  #endif
>>         unsigned int        package_num;     /* Number of packages         */
>>         struct list_head    packages;        /* List of packages           */
>> +       struct ncsi_channel *hot_channel;    /* Channel was ever active    */
>>         struct ncsi_request requests[256];   /* Request table              */
>>         unsigned int        request_id;      /* Last used request ID       */
>>  #define NCSI_REQ_START_IDX     1
>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>> index e959979..cccedcf 100644
>> --- a/net/ncsi/ncsi-manage.c
>> +++ b/net/ncsi/ncsi-manage.c
>> @@ -625,6 +625,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>>         struct net_device *dev = nd->dev;
>>         struct ncsi_package *np = ndp->active_package;
>>         struct ncsi_channel *nc = ndp->active_channel;
>> +       struct ncsi_channel *hot_nc = NULL;
>>         struct ncsi_cmd_arg nca;
>>         unsigned char index;
>>         unsigned long flags;
>> @@ -730,12 +731,20 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>>                 break;
>>         case ncsi_dev_state_config_done:
>>                 spin_lock_irqsave(&nc->lock, flags);
>> -               if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1)
>> +               if (nc->modes[NCSI_MODE_LINK].data[2] & 0x1) {
>> +                       hot_nc = nc;
>>                         nc->state = NCSI_CHANNEL_ACTIVE;
>> -               else
>> +               } else {
>> +                       hot_nc = NULL;
>>                         nc->state = NCSI_CHANNEL_INACTIVE;
>> +               }
>>                 spin_unlock_irqrestore(&nc->lock, flags);
>>
>> +               /* Update the hot channel */
>> +               spin_lock_irqsave(&ndp->lock, flags);
>> +               ndp->hot_channel = hot_nc;
>> +               spin_unlock_irqrestore(&ndp->lock, flags);
>> +
>>                 ncsi_start_channel_monitor(nc);
>>                 ncsi_process_next_channel(ndp);
>>                 break;
>> @@ -753,10 +762,14 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>>  static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>>  {
>>         struct ncsi_package *np;
>> -       struct ncsi_channel *nc, *found;
>> +       struct ncsi_channel *nc, *found, *hot_nc;
>>         struct ncsi_channel_mode *ncm;
>>         unsigned long flags;
>>
>> +       spin_lock_irqsave(&ndp->lock, flags);
>> +       hot_nc = ndp->hot_channel;
>> +       spin_unlock_irqrestore(&ndp->lock, flags);
>> +
>>         /* The search is done once an inactive channel with up
>>          * link is found.
>>          */
>> @@ -774,6 +787,9 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>>                         if (!found)
>>                                 found = nc;
>>
>> +                       if (nc == hot_nc)
>> +                               found = nc;
>> +
>>                         ncm = &nc->modes[NCSI_MODE_LINK];
>>                         if (ncm->data[2] & 0x1) {
>>                                 spin_unlock_irqrestore(&nc->lock, flags);
>> --
>> 2.1.0
>>
>

Re: [PATCH net 0/5] net/ncsi: More bug fixes

[Gavin Shan]

On Fri, Oct 14, 2016 at 01:53:29PM +1100, Gavin Shan wrote:
>This series fixes 2 issues that were found during NCSI's availability
>testing on BCM5718 and improves HNCDSC AEN handler:
>
>   * PATCH[1] and PATCH[2] refactors the code so that minimal code
>     change is put to PATCH[3].
>   * PATCH[3] fixes the NCSI channel's stale link state before doing
>     failover.
>   * PATCH[4] chooses the hot channel, which was ever chosen as active
>     channel, when the available channels are all in link-down state.
>   * PATCH[5] improves Host Network Controller Driver Status Change
>     (HNCDSC) AEN handler
>

v2 addressing comments received from Joel has been posted. Joel, thanks
a lot for your time and comments :)

Thanks,
Gavin

>Gavin Shan (5):
>  net/ncsi: Not fetch active package and channel again
>  net/ncsi: Split out logic for ncsi_dev_state_suspend_select
>  net/ncsi: Fix stale link state of inactive channels on failover
>  net/ncsi: Choose hot channel as active one if necessary
>  net/ncsi: Improve HNCDSC AEN handler
>
> net/ncsi/internal.h    |  2 ++
> net/ncsi/ncsi-aen.c    | 18 ++++++++++--
> net/ncsi/ncsi-manage.c | 80 ++++++++++++++++++++++++++++++++++++++------------
> 3 files changed, 79 insertions(+), 21 deletions(-)
>
>-- 
>2.1.0
>