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

[Brian Norris]

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@xxxxxxxxx> wrote:
> 
> > On Tue, Jul 19, 2016 at 11:30 AM, Brian Norris
> > <computersforpeace@xxxxxxxxx> 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@xxxxxxxxx>
> > >> ---
> > >>  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
>