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]

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

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?

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