Re: [PATCH 2/2] mtd: nand: Get rid of needless 'goto'

[Brian Norris]

On Tue, Jul 19, 2016 at 08:41:44AM -0700, Andrey Smirnov wrote:
> Using "goto" in that "switch" statement only makes it harder to follow
> control flow and doesn't bring any advantages. Rewrite the code to avoid
> using "goto".
> 
> Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com>
> ---
>  drivers/mtd/nand/nand_base.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 57043a6..8fa5536 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2139,18 +2139,15 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t 
> from,
>       case MTD_OPS_PLACE_OOB:
>       case MTD_OPS_AUTO_OOB:
>       case MTD_OPS_RAW:
> +             if (!ops->datbuf)
> +                     ret = nand_do_read_oob(mtd, from, ops);
> +             else
> +                     ret = nand_do_read_ops(mtd, from, ops);
>               break;
> -
>       default:
> -             goto out;
> +             break;
>       }
>  
> -     if (!ops->datbuf)
> -             ret = nand_do_read_oob(mtd, from, ops);
> -     else
> -             ret = nand_do_read_ops(mtd, from, ops);
> -
> -out:
>       nand_release_device(mtd);
>       return ret;
>  }

The default case is really just a catch-all error case. We don't
actually even need the nand_get_device() call for that... can we just
do this instead?

static int nand_read_oob(struct mtd_info *mtd, loff_t from,
                         struct mtd_oob_ops *ops)
{
        int ret;
         
        ops->retlen = 0; 

        /* Do not allow reads past end of device */
        if (ops->datbuf && (from + ops->len) > mtd->size) {
                pr_debug("%s: attempt to read beyond end of device\n",
                                __func__);
                return -EINVAL;
        }

        switch (ops->mode) {
        case MTD_OPS_PLACE_OOB:
        case MTD_OPS_AUTO_OOB:
        case MTD_OPS_RAW:
                break;

        default:
                return -ENOTSUPP;
        }

        nand_get_device(mtd, FL_READING);

        if (!ops->datbuf)
                ret = nand_do_read_oob(mtd, from, ops);
        else
                ret = nand_do_read_ops(mtd, from, ops);

        nand_release_device(mtd);
        return ret;
}

i.e., this diff:

Signed-off-by: Brian Norris <computersforpe...@gmail.com>

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 77533f7f2429..881dbd495466 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2162,7 +2162,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t 
from,
 static int nand_read_oob(struct mtd_info *mtd, loff_t from,
                         struct mtd_oob_ops *ops)
 {
-       int ret = -ENOTSUPP;
+       int ret;
 
        ops->retlen = 0;
 
@@ -2173,8 +2173,6 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t 
from,
                return -EINVAL;
        }
 
-       nand_get_device(mtd, FL_READING);
-
        switch (ops->mode) {
        case MTD_OPS_PLACE_OOB:
        case MTD_OPS_AUTO_OOB:
@@ -2182,15 +2180,16 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t 
from,
                break;
 
        default:
-               goto out;
+               return -ENOTSUPP;
        }
 
+       nand_get_device(mtd, FL_READING);
+
        if (!ops->datbuf)
                ret = nand_do_read_oob(mtd, from, ops);
        else
                ret = nand_do_read_ops(mtd, from, ops);
 
-out:
        nand_release_device(mtd);
        return ret;
 }