mtd: onenand_base: Avoid fall-through warnings

PATCH Superseded Gustavo A. R. Silva 3 participants 7 messages May 22, 2019

Subsystems (3)

Memory technology devices (MTD) NAND flash Onenand flash driver

lore

Share & tools

lore

Gustavo A. R. Silva

author

2019-05-22 18:05 UTC #0 lore raw

PATCH 1/1 mtd: onenand_base: Avoid fall-through warnings

NOTICE THAT:

"...we don't know whether we need fallthroughs or breaks there and this is just a change to avoid having new warnings when switching to -Wimplicit-fallthrough but this change might be entirely wrong."[1]

See the original thread of discussion here:

https://lore.kernel.org/patchwork/patch/1036251/

So, in preparation to enabling -Wimplicit-fallthrough, this patch silences the following warnings:

drivers/mtd/nand/onenand/onenand_base.c: In function ‘onenand_check_features’:
drivers/mtd/nand/onenand/onenand_base.c:3264:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
if (ONENAND_IS_DDP(this))

drivers/mtd/nand/onenand/onenand_base.c:3284:2: note: here
case ONENAND_DEVICE_DENSITY_2Gb:
^~~~
drivers/mtd/nand/onenand/onenand_base.c:3288:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
this->options |= ONENAND_HAS_UNLOCK_ALL;
drivers/mtd/nand/onenand/onenand_base.c:3290:2: note: here
case ONENAND_DEVICE_DENSITY_1Gb:
^~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.

[1] https://lore.kernel.org/lkml/20190509085318.34a9d4be@xps13/

Cc Miquel Raynal <[email protected]>

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/mtd/nand/onenand/onenand_base.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/mtd/nand/onenand/onenand_base.c +2 -0

index f41d76248550..6cf4df9f8c01 100644
@@ -3280,12 +3280,14 @@ static void onenand_check_features(struct mtd_info *mtd)
 			if ((this->version_id & 0xf) == 0xe)
 				this->options |= ONENAND_HAS_NOP_1;
 		}
+		/* Fall through - ? */
 
 	case ONENAND_DEVICE_DENSITY_2Gb:
 		/* 2Gb DDP does not have 2 plane */
 		if (!ONENAND_IS_DDP(this))
 			this->options |= ONENAND_HAS_2PLANE;
 		this->options |= ONENAND_HAS_UNLOCK_ALL;
+		/* Fall through - ? */
 
 	case ONENAND_DEVICE_DENSITY_1Gb:
 		/* A-Die has all block unlock */

Signature · 1 line

2.21.0

Kees Cook

2019-05-22 21:30 UTC #1 lore raw

Sorry for being late to speaking up on this. I missed something in the code the first time I read the thread, that now stood out to me. Notes below...

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd)

Quoted 5 lines · Gustavo A. R. Silva

drivers/mtd/nand/onenand/onenand_base.c +0 -0

index f41d76248550..6cf4df9f8c01 100644
@@ -3280,12 +3280,14 @@ static void onenand_check_features(struct mtd_info *mtd)

Reverse-order review, second hunk first:

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3293

Quoted 9 lines

drivers/mtd/nand/onenand/onenand_base.c +1 -0

 	case ONENAND_DEVICE_DENSITY_2Gb:
 		/* 2Gb DDP does not have 2 plane */
 		if (!ONENAND_IS_DDP(this))
 			this->options |= ONENAND_HAS_2PLANE;
 		this->options |= ONENAND_HAS_UNLOCK_ALL;
+		/* Fall through - ? */
 
 	case ONENAND_DEVICE_DENSITY_1Gb:
 		/* A-Die has all block unlock */

So, I think the ONENAND_DEVICE_DENSITY_2Gb should be a "break". Though, actually, it doesn't matter:

        case ONENAND_DEVICE_DENSITY_2Gb:
                /* 2Gb DDP does not have 2 plane */
                if (!ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;
                this->options |= ONENAND_HAS_UNLOCK_ALL;

        case ONENAND_DEVICE_DENSITY_1Gb:
                /* A-Die has all block unlock */
                if (process)
                        this->options |= ONENAND_HAS_UNLOCK_ALL;
                break;

Falling through from ONENAND_DEVICE_DENSITY_2Gb to ONENAND_DEVICE_DENSITY_1Gb will actually have no side-effects:
ONENAND_HAS_UNLOCK_ALL was unconditionally set in ..._2Gb, so there is no reason to fall through to ..._1Gb. (But falling through is harmless.)

Now the first hunk:

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3284

Quoted 5 lines

drivers/mtd/nand/onenand/onenand_base.c +1 -0

 			if ((this->version_id & 0xf) == 0xe)
 				this->options |= ONENAND_HAS_NOP_1;
 		}
+		/* Fall through - ? */

        case ONENAND_DEVICE_DENSITY_4Gb:
                if (ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;
                else if (numbufs == 1) {
                        this->options |= ONENAND_HAS_4KB_PAGE;
                        this->options |= ONENAND_HAS_CACHE_PROGRAM;
                        /*
                         * There are two different 4KiB pagesize chips
                         * and no way to detect it by H/W config values.
                         *
                         * To detect the correct NOP for each chips,
                         * It should check the version ID as workaround.
                         *
                         * Now it has as following
                         * KFM4G16Q4M has NOP 4 with version ID 0x0131
                         * KFM4G16Q5M has NOP 1 with versoin ID 0x013e
                         */
                        if ((this->version_id & 0xf) == 0xe)
                                this->options |= ONENAND_HAS_NOP_1;
                }

Falling through from ONENAND_DEVICE_DENSITY_4Gb to ONENAND_DEVICE_DENSITY_2Gb looks like it would mean that ONENAND_HAS_2PLANE would be unconditionally set for ...4Gb, which seems very strange to expect:

                if (ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;
...
                if (!ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;

However! This happens later:

        if (ONENAND_IS_4KB_PAGE(this))
                this->options &= ~ONENAND_HAS_2PLANE;

i.e. falling through to ...2Gb (which sets ONENAND_HAS_2PLANE) has no effect because when ONENAND_HAS_2PLANE isn't set (numbufs == 1), it gets _cleared_ by the above code due to ONENAND_HAS_4KB_PAGE getting set:

#define ONENAND_IS_4KB_PAGE(this) \
        (this->options & ONENAND_HAS_4KB_PAGE)

Unfortunately, though, it's less clear about ONENAND_HAS_UNLOCK_ALL, which is getting set unconditionally for ...4Gb currently (due to the fallthrough to ...2Gb). However, this happens later:

        if (FLEXONENAND(this)) {
                this->options &= ~ONENAND_HAS_CONT_LOCK;
                this->options |= ONENAND_HAS_UNLOCK_ALL;
        }
...
#define FLEXONENAND(this) \
        (this->device_id & DEVICE_IS_FLEXONENAND)

So it's possible this fall through has no effect (are all 4Gb density devices also FLEXONENAND devices?)

Setting a "break" after 4Gb may remove ONENAND_HAS_UNLOCK_ALL in the !FLEXONENAND(this) case. Does anyone have real hardware to test with?

Thoughts?

Signature · 1 line

Kees Cook

Boris Brezillon

2019-05-22 21:57 UTC #2 lore raw

Quoted 77 lines · On Wed, 22 May 2019 14:30:11 -0700 Kees Cook <[email protected]> wrote:

Sorry for being late to speaking up on this. I missed something in the code the first time I read the thread, that now stood out to me. Notes below...
drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd)
Quoted 5 lines · Gustavo A. R. Silva
drivers/mtd/nand/onenand/onenand_base.c +0 -0
index f41d76248550..6cf4df9f8c01 100644
@@ -3280,12 +3280,14 @@ static void onenand_check_features(struct mtd_info *mtd)  
Reverse-order review, second hunk first:
drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3293
Quoted 9 lines
drivers/mtd/nand/onenand/onenand_base.c +1 -0
 	case ONENAND_DEVICE_DENSITY_2Gb:
 		/* 2Gb DDP does not have 2 plane */
 		if (!ONENAND_IS_DDP(this))
 			this->options |= ONENAND_HAS_2PLANE;
 		this->options |= ONENAND_HAS_UNLOCK_ALL;
+		/* Fall through - ? */
 
 	case ONENAND_DEVICE_DENSITY_1Gb:
 		/* A-Die has all block unlock */  
So, I think the ONENAND_DEVICE_DENSITY_2Gb should be a "break". Though, actually, it doesn't matter:
        case ONENAND_DEVICE_DENSITY_2Gb:
                /* 2Gb DDP does not have 2 plane */
                if (!ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;
                this->options |= ONENAND_HAS_UNLOCK_ALL;

        case ONENAND_DEVICE_DENSITY_1Gb:
                /* A-Die has all block unlock */
                if (process)
                        this->options |= ONENAND_HAS_UNLOCK_ALL;
                break;
Falling through from ONENAND_DEVICE_DENSITY_2Gb to ONENAND_DEVICE_DENSITY_1Gb will actually have no side-effects:
ONENAND_HAS_UNLOCK_ALL was unconditionally set in ..._2Gb, so there is no reason to fall through to ..._1Gb. (But falling through is harmless.)
Now the first hunk:
drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3284
Quoted 5 lines
drivers/mtd/nand/onenand/onenand_base.c +1 -0
 			if ((this->version_id & 0xf) == 0xe)
 				this->options |= ONENAND_HAS_NOP_1;
 		}
+		/* Fall through - ? */
 
        case ONENAND_DEVICE_DENSITY_4Gb:
                if (ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;
                else if (numbufs == 1) {
                        this->options |= ONENAND_HAS_4KB_PAGE;
                        this->options |= ONENAND_HAS_CACHE_PROGRAM;
                        /*
                         * There are two different 4KiB pagesize chips
                         * and no way to detect it by H/W config values.
                         *
                         * To detect the correct NOP for each chips,
                         * It should check the version ID as workaround.
                         *
                         * Now it has as following
                         * KFM4G16Q4M has NOP 4 with version ID 0x0131
                         * KFM4G16Q5M has NOP 1 with versoin ID 0x013e
                         */
                        if ((this->version_id & 0xf) == 0xe)
                                this->options |= ONENAND_HAS_NOP_1;
                }
Falling through from ONENAND_DEVICE_DENSITY_4Gb to ONENAND_DEVICE_DENSITY_2Gb looks like it would mean that ONENAND_HAS_2PLANE would be unconditionally set for ...4Gb, which seems very strange to expect:

Quoted 5 lines

                if (ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;
...
                if (!ONENAND_IS_DDP(this))
                        this->options |= ONENAND_HAS_2PLANE;

Oops, didn't notice the ! on the second test.

Quoted 9 lines

However! This happens later:
        if (ONENAND_IS_4KB_PAGE(this))
                this->options &= ~ONENAND_HAS_2PLANE;
i.e. falling through to ...2Gb (which sets ONENAND_HAS_2PLANE) has no effect because when ONENAND_HAS_2PLANE isn't set (numbufs == 1), it gets _cleared_ by the above code due to ONENAND_HAS_4KB_PAGE getting set:

Are you sure !DDP implies num_bufs == 1?

Quoted 17 lines

#define ONENAND_IS_4KB_PAGE(this) \
        (this->options & ONENAND_HAS_4KB_PAGE)
Unfortunately, though, it's less clear about ONENAND_HAS_UNLOCK_ALL, which is getting set unconditionally for ...4Gb currently (due to the fallthrough to ...2Gb). However, this happens later:
        if (FLEXONENAND(this)) {
                this->options &= ~ONENAND_HAS_CONT_LOCK;
                this->options |= ONENAND_HAS_UNLOCK_ALL;
        }
...
#define FLEXONENAND(this) \
        (this->device_id & DEVICE_IS_FLEXONENAND)

Quoted 3 lines

So it's possible this fall through has no effect (are all 4Gb density devices also FLEXONENAND devices?)

All this look suspicious, and even if the fall through logic has no side effects in practice (which I'm still not sure is the case), I think it'd be better to explicitly set the flags that have to be set in each case statement and add breaks.

Kees Cook

2019-05-22 22:20 UTC #3 lore raw

Quoted 4 lines · Boris Brezillon

All this look suspicious, and even if the fall through logic has no side effects in practice (which I'm still not sure is the case), I think it'd be better to explicitly set the flags that have to be set in each case statement and add breaks.

Yeah, totally agreed. :)

Signature · 1 line

Kees Cook

Gustavo A. R. Silva

author

2019-05-22 22:30 UTC #4 lore raw

Quoted 8 lines · Kees Cook

Quoted 4 lines · Boris Brezillon
All this look suspicious, and even if the fall through logic has no side effects in practice (which I'm still not sure is the case), I think it'd be better to explicitly set the flags that have to be set in each case statement and add breaks.
Yeah, totally agreed. :)

Thank you, Kees and Boris for the feedback. Some external opinions were certainly much needed here.

I'll just wait for any comments from the MTD guys.

Thanks
--
Gustavo

Boris Brezillon

2019-05-22 21:37 UTC #5 lore raw

In reply to Gustavo A. R. Silva

Quoted 44 lines · On Wed, 22 May 2019 13:04:46 -0500 "Gustavo A. R. Silva"…

NOTICE THAT:
"...we don't know whether we need fallthroughs or breaks there and this is just a change to avoid having new warnings when switching to -Wimplicit-fallthrough but this change might be entirely wrong."[1]
See the original thread of discussion here:
https://lore.kernel.org/patchwork/patch/1036251/
So, in preparation to enabling -Wimplicit-fallthrough, this patch silences the following warnings:
drivers/mtd/nand/onenand/onenand_base.c: In function ‘onenand_check_features’:
drivers/mtd/nand/onenand/onenand_base.c:3264:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
if (ONENAND_IS_DDP(this))
      ^
drivers/mtd/nand/onenand/onenand_base.c:3284:2: note: here
case ONENAND_DEVICE_DENSITY_2Gb:
^~~~
drivers/mtd/nand/onenand/onenand_base.c:3288:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
this->options |= ONENAND_HAS_UNLOCK_ALL;
drivers/mtd/nand/onenand/onenand_base.c:3290:2: note: here
case ONENAND_DEVICE_DENSITY_1Gb:
^~~~
Warning level 3 was used: -Wimplicit-fallthrough=3
This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.
[1] https://lore.kernel.org/lkml/20190509085318.34a9d4be@xps13/
Cc Miquel Raynal <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/mtd/nand/onenand/onenand_base.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/mtd/nand/onenand/onenand_base.c +0 -0
index f41d76248550..6cf4df9f8c01 100644

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3283

Quoted 4 lines

drivers/mtd/nand/onenand/onenand_base.c +1 -0

@@ -3280,12 +3280,14 @@ static void onenand_check_features(struct mtd_info *mtd)
 			if ((this->version_id & 0xf) == 0xe)
 				this->options |= ONENAND_HAS_NOP_1;
 		}
+		/* Fall through - ? */

So, the only thing that you'll re-use by falling through the next case is the '->options |= ONENAND_HAS_UNLOCK_ALL' operation. I find it easier to follow with an explicit copy of this line + a break.

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3290

Quoted 7 lines

drivers/mtd/nand/onenand/onenand_base.c +1 -0

 	case ONENAND_DEVICE_DENSITY_2Gb:
 		/* 2Gb DDP does not have 2 plane */
 		if (!ONENAND_IS_DDP(this))
 			this->options |= ONENAND_HAS_2PLANE;
 		this->options |= ONENAND_HAS_UNLOCK_ALL;
+		/* Fall through - ? */

This fall through certainly doesn't make sense, as the only thing that might be done in the 1Gb case is conditionally adding the HAS_UNLOCK_ALL flag, and this flag is already unconditionally set. Please add a break here.

Quoted 3 lines

drivers/mtd/nand/onenand/onenand_base.c +0 -0
 	case ONENAND_DEVICE_DENSITY_1Gb:
 		/* A-Die has all block unlock */

Kees Cook

2019-05-22 21:45 UTC #6 lore raw

Quoted 11 lines · Boris Brezillon

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3283
Quoted 5 lines
drivers/mtd/nand/onenand/onenand_base.c +1 -0
@@ -3280,12 +3280,14 @@ static void onenand_check_features(struct mtd_info *mtd)
 			if ((this->version_id & 0xf) == 0xe)
 				this->options |= ONENAND_HAS_NOP_1;
 		}
+		/* Fall through - ? */
So, the only thing that you'll re-use by falling through the next case is the '->options |= ONENAND_HAS_UNLOCK_ALL' operation. I find it easier to follow with an explicit copy of this line + a break.
Quoted 1 line

Quoted 16 lines

drivers/mtd/nand/onenand/onenand_base.c static void onenand_check_features(struct mtd_info *mtd) · line 3290
Quoted 6 lines
drivers/mtd/nand/onenand/onenand_base.c +1 -0
 	case ONENAND_DEVICE_DENSITY_2Gb:
 		/* 2Gb DDP does not have 2 plane */
 		if (!ONENAND_IS_DDP(this))
 			this->options |= ONENAND_HAS_2PLANE;
 		this->options |= ONENAND_HAS_UNLOCK_ALL;
+		/* Fall through - ? */
This fall through certainly doesn't make sense, as the only thing that might be done in the 1Gb case is conditionally adding the HAS_UNLOCK_ALL flag, and this flag is already unconditionally set. Please add a break here.
Quoted 3 lines
drivers/mtd/nand/onenand/onenand_base.c +0 -0
 	case ONENAND_DEVICE_DENSITY_1Gb:
 		/* A-Die has all block unlock */

Your reply was much more to-the-point than mine. :) I'd agree: retain existing behavior (ONENAND_HAS_UNLOCK_ALL) and add breaks.

Signature · 1 line

Kees Cook