diff mbox

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

Message ID 20160719183047.GC85399@google.com
State Superseded
Headers show

Commit Message

Brian Norris July 19, 2016, 6:30 p.m. UTC 
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.smirnov@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 <computersforpeace@gmail.com>

Comments

Andrey Smirnov July 19, 2016, 6:48 p.m. UTC | #1 
On Tue, Jul 19, 2016 at 11:30 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> 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.smirnov@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?

Sure, although, if you don't mind, I'd rather you used:

if (ops->mode != MTD_OPS_PLACE_OOB &&
    ops->mode != MTD_OPS_AUTO_OOB &&
    ops->mode != MTD_OPS_RAW)
       return -ENOTSUPP;

instead of the switch statement, IMHO, this way it is more obvious
that this codepath is just arguments correctness checking.

Andrey
Boris Brezillon July 19, 2016, 6:55 p.m. UTC | #2 
On Tue, 19 Jul 2016 11:48:04 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> On Tue, Jul 19, 2016 at 11:30 AM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > 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.smirnov@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?  
> 
> Sure, although, if you don't mind, I'd rather you used:
> 
> if (ops->mode != MTD_OPS_PLACE_OOB &&
>     ops->mode != MTD_OPS_AUTO_OOB &&
>     ops->mode != MTD_OPS_RAW)
>        return -ENOTSUPP;

Or just

	if (ops->mode < MTD_OPS_PLACE_OOB || ops->mode > MTD_OPS_RAW)
		return -ENOTSUPP;

Anyway, I'm fine with all different versions as long as you don't take
the nand lock if the mode is incorrect, so I'll let Brian decide.

> 
> instead of the switch statement, IMHO, this way it is more obvious
> that this codepath is just arguments correctness checking.
> 
> Andrey
Brian Norris July 19, 2016, 7:43 p.m. UTC | #3 
Hi,

On Tue, Jul 19, 2016 at 08:55:21PM +0200, Boris Brezillon wrote:
> On Tue, 19 Jul 2016 11:48:04 -0700
> Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> 
> > On Tue, Jul 19, 2016 at 11:30 AM, Brian Norris
> > <computersforpeace@gmail.com> wrote:
> > > 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.smirnov@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?  
> > 
> > Sure, although, if you don't mind, I'd rather you used:
> > 
> > if (ops->mode != MTD_OPS_PLACE_OOB &&
> >     ops->mode != MTD_OPS_AUTO_OOB &&
> >     ops->mode != MTD_OPS_RAW)
> >        return -ENOTSUPP;
> 
> Or just
> 
> 	if (ops->mode < MTD_OPS_PLACE_OOB || ops->mode > MTD_OPS_RAW)

ops->mode is unsigned. And this seems a little more fragile, assuming
the enum layout.

> 		return -ENOTSUPP;
> 
> Anyway, I'm fine with all different versions as long as you don't take
> the nand lock if the mode is incorrect, so I'll let Brian decide.

Whatever Andrey prefers is his choice to send, and I don't have much
more preference than the above comment. I'd take either mine or Andrey's
second solution above.

Brian

> > 
> > instead of the switch statement, IMHO, this way it is more obvious
> > that this codepath is just arguments correctness checking.
> > 
> > Andrey
>
diff mbox

Patch

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