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