Message ID | 20160719183047.GC85399@google.com |
---|---|
State | Superseded |
Headers | show |
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
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
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 --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; }