| Message ID | 1476413614-24586-4-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 >
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 >> >
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:
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(-)