| Message ID | 20180710132902.GA13541@embeddedor.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: nuc900_nand: mark expected switch fall-through | expand |
Hi Gustavo, Prefix should be "mtd: rawnand: nuc900:" "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Tue, 10 Jul 2018 08:29:02 -0500: > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Addresses-Coverity-ID: 1471717 ("Missing break in switch") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/mtd/nand/raw/nuc900_nand.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/nuc900_nand.c b/drivers/mtd/nand/raw/nuc900_nand.c > index af5b32c9..53a9f6c 100644 > --- a/drivers/mtd/nand/raw/nuc900_nand.c > +++ b/drivers/mtd/nand/raw/nuc900_nand.c > @@ -191,8 +191,9 @@ static void nuc900_nand_command_lp(struct mtd_info *mtd, unsigned int command, > return; > > case NAND_CMD_READ0: > - > write_cmd_reg(nand, NAND_CMD_READSTART); > + /* fall through */ Have you checked this is actually the right thing to do? Thanks, Miquèl
Hi Miquel, On 07/18/2018 03:03 AM, Miquel Raynal wrote: > Hi Gustavo, > > Prefix should be "mtd: rawnand: nuc900:" > Oh OK. I'll fix it. > "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Tue, 10 Jul > 2018 08:29:02 -0500: > >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >> where we are expecting to fall through. >> >> Addresses-Coverity-ID: 1471717 ("Missing break in switch") >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >> --- >> drivers/mtd/nand/raw/nuc900_nand.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/nand/raw/nuc900_nand.c b/drivers/mtd/nand/raw/nuc900_nand.c >> index af5b32c9..53a9f6c 100644 >> --- a/drivers/mtd/nand/raw/nuc900_nand.c >> +++ b/drivers/mtd/nand/raw/nuc900_nand.c >> @@ -191,8 +191,9 @@ static void nuc900_nand_command_lp(struct mtd_info *mtd, unsigned int command, >> return; >> >> case NAND_CMD_READ0: >> - >> write_cmd_reg(nand, NAND_CMD_READSTART); >> + /* fall through */ > > Have you checked this is actually the right thing to do? > Actually, no. My first impression was that due to the time this code has been there, this might be a missing-break false positive. But, now that I'm double checking, it may well be that this is an actual missing-break bug. I can send a patch to fix this, but as I'm not familiar with the code, it'd be of great help if someone could help me to verify this. Thanks for the feedback. -- Gustavo
Hi Miquel, On 07/18/2018 03:03 AM, Miquel Raynal wrote: > Hi Gustavo, > > Prefix should be "mtd: rawnand: nuc900:" > Oh OK. I'll fix it. Thanks. > "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Tue, 10 Jul > 2018 08:29:02 -0500: > >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >> where we are expecting to fall through. >> >> Addresses-Coverity-ID: 1471717 ("Missing break in switch") >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >> --- >> drivers/mtd/nand/raw/nuc900_nand.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/nand/raw/nuc900_nand.c b/drivers/mtd/nand/raw/nuc900_nand.c >> index af5b32c9..53a9f6c 100644 >> --- a/drivers/mtd/nand/raw/nuc900_nand.c >> +++ b/drivers/mtd/nand/raw/nuc900_nand.c >> @@ -191,8 +191,9 @@ static void nuc900_nand_command_lp(struct mtd_info *mtd, unsigned int command, >> return; >> >> case NAND_CMD_READ0: >> - >> write_cmd_reg(nand, NAND_CMD_READSTART); >> + /* fall through */ > > Have you checked this is actually the right thing to do? > Actually, no. My first impression was that due to the long time this code has been there, this might be a missing-break-in-switch false positive. But, due to your comments and a double check at the code, it may well be that this is an actual bug and that a return statement should be added after calling write_cmd_reg. Similar to the cases above. I can send a patch to fix this, but it would be great if someone could help me to verify this first. :) Thanks for the feedback! -- Gustavo
diff --git a/drivers/mtd/nand/raw/nuc900_nand.c b/drivers/mtd/nand/raw/nuc900_nand.c index af5b32c9..53a9f6c 100644 --- a/drivers/mtd/nand/raw/nuc900_nand.c +++ b/drivers/mtd/nand/raw/nuc900_nand.c @@ -191,8 +191,9 @@ static void nuc900_nand_command_lp(struct mtd_info *mtd, unsigned int command, return; case NAND_CMD_READ0: - write_cmd_reg(nand, NAND_CMD_READSTART); + /* fall through */ + default: if (!chip->dev_ready) {
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 1471717 ("Missing break in switch") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/mtd/nand/raw/nuc900_nand.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)