netdev - Re: [PATCH net 2/5] net/ncsi: Split out logic for ncsi_dev_state_suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20161014102832.GA6278@gwshan>
Date:   Fri, 14 Oct 2016 21:28:32 +1100
From:   Gavin Shan <gwshan@...ux.vnet.ibm.com>
To:     Joel Stanley <joel@....id.au>
Cc:     Gavin Shan <gwshan@...ux.vnet.ibm.com>, netdev@...r.kernel.org,
        davem@...emloft.net
Subject: Re: [PATCH net 2/5] net/ncsi: Split out logic for
 ncsi_dev_state_suspend_select

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