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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

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
>>
>