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 <gws...@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 <gws...@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
>>
>
