Skip to contentSkip to conversation

mtd: cfi_util: mark expected switch fall-throughs

PATCH In review Gustavo A. R. Silva 4 participants 16 messages Feb 8, 2019 – May 9, 2019 Memory technology devices (MTD)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mtd: cfi_util: mark expected switch fall-throughs

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This patch fixes the following warnings:

drivers/mtd/chips/cfi_util.c: In function ‘cfi_build_cmd’:
drivers/mtd/chips/cfi_util.c:110:10: warning: this statement may fall through [-Wimplicit-fallthrough=]
onecmd |= (onecmd << (chip_mode * 32));
~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mtd/chips/cfi_util.c:112:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:113:10: warning: this statement may fall through [-Wimplicit-fallthrough=]
onecmd |= (onecmd << (chip_mode * 16));
~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mtd/chips/cfi_util.c:114:2: note: here
case 2:
^~~~
drivers/mtd/chips/cfi_util.c: In function ‘cfi_merge_status’:
drivers/mtd/chips/cfi_util.c:163:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
res |= (onestat >> (chip_mode * 32));
~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mtd/chips/cfi_util.c:165:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:166:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
res |= (onestat >> (chip_mode * 16));
~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mtd/chips/cfi_util.c:167:2: note: here
case 2:
^~~~

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

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

---
 drivers/mtd/chips/cfi_util.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/chips/cfi_util.c +6 -0
index 6f16552cd59f..e3b266ee06af 100644
@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cfi
 	case 8:
 		onecmd |= (onecmd << (chip_mode * 32));
 #endif
+		/* fall through */
 	case 4:
 		onecmd |= (onecmd << (chip_mode * 16));
+		/* fall through */
 	case 2:
 		onecmd |= (onecmd << (chip_mode * 8));
+		/* fall through */
 	case 1:
 		;
 	}
@@ -162,10 +165,13 @@ unsigned long cfi_merge_status(map_word val, struct map_info *map,
 	case 8:
 		res |= (onestat >> (chip_mode * 32));
 #endif
+		/* fall through */
 	case 4:
 		res |= (onestat >> (chip_mode * 16));
+		/* fall through */
 	case 2:
 		res |= (onestat >> (chip_mode * 8));
+		/* fall through */
 	case 1:
 		;
 	}
Signature · 1 line
2.20.1
#1 lore raw

Hi all,

Friendly ping:

Who can take this?

Thanks
--
Gustavo

Quoted 75 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This patch fixes the following warnings:

drivers/mtd/chips/cfi_util.c: In function ‘cfi_build_cmd’:
drivers/mtd/chips/cfi_util.c:110:10: warning: this statement may fall through [-Wimplicit-fallthrough=]

   onecmd |= (onecmd << (chip_mode * 32));
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:112:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:113:10: warning: this statement may fall through [-Wimplicit-fallthrough=]

   onecmd |= (onecmd << (chip_mode * 16));
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:114:2: note: here
case 2:
^~~~
drivers/mtd/chips/cfi_util.c: In function ‘cfi_merge_status’:
drivers/mtd/chips/cfi_util.c:163:7: warning: this statement may fall through [-Wimplicit-fallthrough=]

   res |= (onestat >> (chip_mode * 32));
   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:165:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:166:7: warning: this statement may fall through [-Wimplicit-fallthrough=]

   res |= (onestat >> (chip_mode * 16));
   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:167:2: note: here
case 2:
^~~~

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

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

---
 drivers/mtd/chips/cfi_util.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/chips/cfi_util.c +6 -0
index 6f16552cd59f..e3b266ee06af 100644
@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cfi
 	case 8:
 		onecmd |= (onecmd << (chip_mode * 32));
 #endif
+		/* fall through */
 	case 4:
 		onecmd |= (onecmd << (chip_mode * 16));
+		/* fall through */
 	case 2:
 		onecmd |= (onecmd << (chip_mode * 8));
+		/* fall through */
 	case 1:
 		;
 	}
@@ -162,10 +165,13 @@ unsigned long cfi_merge_status(map_word val, struct map_info *map,
 	case 8:
 		res |= (onestat >> (chip_mode * 32));
 #endif
+		/* fall through */
 	case 4:
 		res |= (onestat >> (chip_mode * 16));
+		/* fall through */
 	case 2:
 		res |= (onestat >> (chip_mode * 8));
+		/* fall through */
 	case 1:
 		;
 	}
#2 lore raw

Hi!

Am Mittwoch, 20. März 2019, 21:20:51 CET schrieb Gustavo A. R. Silva:

Quoted 5 lines

Hi all,

Friendly ping:

Who can take this?

Hmmm, for MTD I think we can schedule these patches for the next merge window. But I'm not sure whether these comments are a good solution.
I much more prefer the compiler attribute solution.
Also a tree-wide (sane) coccinelle script would be better IMHO, and not zillions of individual patches via different trees.

Thanks,
//richard

#3 lore raw
Quoted 7 lines · Richard Weinberger

Hi!

Am Mittwoch, 20. März 2019, 21:20:51 CET schrieb Gustavo A. R. Silva:

Quoted 4 lines

Hi all,

Friendly ping:

Quoted 8 lines
Quoted 1 line

Who can take this?

Hmmm, for MTD I think we can schedule these patches for the next merge window. But I'm not sure whether these comments are a good solution.
I much more prefer the compiler attribute solution.
Also a tree-wide (sane) coccinelle script would be better IMHO, and not zillions of individual patches via different trees.

If that script is sophisticated enough to spot both false positives and actual bugs, then yeah, I'd love that solution. But it's actually not that simple.

The thing about the compiler attributes is not a big deal, once we finally enable -Wimplicit-fallthrough, for which we first need to address each one of these cases. But we are getting close, there are less than 100 of these issues in linux-next. :)

So, if this is too much of a burden for people, I can add all these MTD patches to my own tree for 5.2.

Just, please, let me know.

Thanks
--
Gustavo

#4 lore raw

Am Mittwoch, 20. März 2019, 22:23:21 CET schrieb Gustavo A. R. Silva:

Quoted 2 lines

So, if this is too much of a burden for people, I can add all these MTD patches to my own tree for 5.2.

Taking the patches via MTD is not a big deal, let's go that path. For you it is more work to send many patches and getting them into the individual trees. :)

Thanks,
//richard

#5 lore raw
Quoted 5 lines · Richard Weinberger

Am Mittwoch, 20. März 2019, 22:23:21 CET schrieb Gustavo A. R. Silva:

Quoted 2 lines

So, if this is too much of a burden for people, I can add all these MTD patches to my own tree for 5.2.

Taking the patches via MTD is not a big deal, let's go that path.

Cool.

Quoted 3 lines

For you it is more work to send many patches and getting them into the individual trees. :)

Here applies this wise phrase: It is what it is. :)

Thanks
--
Gustavo

#6 lore raw
In reply to Gustavo A. R. Silva

Hi all,

If no one cares I'll add this to my tree for 5.2.

Thanks
--
Gustavo

Quoted 86 lines · Gustavo A. R. Silva

Hi all,

Friendly ping:

Who can take this?

Thanks
--
Gustavo

Quoted 75 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This patch fixes the following warnings:

drivers/mtd/chips/cfi_util.c: In function ‘cfi_build_cmd’:
drivers/mtd/chips/cfi_util.c:110:10: warning: this statement may fall through [-Wimplicit-fallthrough=]

   onecmd |= (onecmd << (chip_mode * 32));
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:112:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:113:10: warning: this statement may fall through [-Wimplicit-fallthrough=]

   onecmd |= (onecmd << (chip_mode * 16));
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:114:2: note: here
case 2:
^~~~
drivers/mtd/chips/cfi_util.c: In function ‘cfi_merge_status’:
drivers/mtd/chips/cfi_util.c:163:7: warning: this statement may fall through [-Wimplicit-fallthrough=]

   res |= (onestat >> (chip_mode * 32));
   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:165:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:166:7: warning: this statement may fall through [-Wimplicit-fallthrough=]

   res |= (onestat >> (chip_mode * 16));
   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:167:2: note: here
case 2:
^~~~

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

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

---
 drivers/mtd/chips/cfi_util.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/chips/cfi_util.c +6 -0
index 6f16552cd59f..e3b266ee06af 100644
@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cfi
 	case 8:
 		onecmd |= (onecmd << (chip_mode * 32));
 #endif
+		/* fall through */
 	case 4:
 		onecmd |= (onecmd << (chip_mode * 16));
+		/* fall through */
 	case 2:
 		onecmd |= (onecmd << (chip_mode * 8));
+		/* fall through */
 	case 1:
 		;
 	}
@@ -162,10 +165,13 @@ unsigned long cfi_merge_status(map_word val, struct map_info *map,
 	case 8:
 		res |= (onestat >> (chip_mode * 32));
 #endif
+		/* fall through */
 	case 4:
 		res |= (onestat >> (chip_mode * 16));
+		/* fall through */
 	case 2:
 		res |= (onestat >> (chip_mode * 8));
+		/* fall through */
 	case 1:
 		;
 	}
maintainer
#7 lore raw

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Wed, 10 Apr 2019 16:16:51 -0500:

Quoted 3 lines

Hi all,

If no one cares I'll add this to my tree for 5.2.

Which tree are you talking about?

Please let the MTD maintainers take patches through their tree. We might be late but this is definitely not a good reason to bypass us.

Thanks,
Miquèl

#8 lore raw

Hi Miquel,

Quoted 11 lines · Miquel Raynal

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Wed, 10 Apr 2019 16:16:51 -0500:

Quoted 3 lines

Hi all,

If no one cares I'll add this to my tree for 5.2.

Which tree are you talking about?

This one:

https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp

Quoted 3 lines

Please let the MTD maintainers take patches through their tree. We might be late but this is definitely not a good reason to bypass us.

It's a bit confusing when patches are being ignored for more than two months:

https://lore.kernel.org/patchwork/patch/1040099/
https://lore.kernel.org/patchwork/patch/1040100/
https://lore.kernel.org/patchwork/patch/1040098/

Certainly, Richard Weinberger replied to this one. But I couldn't find a tree to which this patch was applied, in case it actually was.

It's a common practice for maintainers to reply saying that a patch has been finally applied, and in most cases they also explicitly mention the tree and branch to which it was applied. All this info is really helpful for people working all over the tree.

I'm only taking this type of patches in my tree when they are being ignored for a considerable amount of time and even after pinging the maintainers.

Thanks
--
Gustavo

maintainer
#9 lore raw

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 15 Apr 2019 07:57:11 -0500:

Quoted 22 lines

Hi Miquel,

Quoted 11 lines · Miquel Raynal

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Wed, 10 Apr 2019 16:16:51 -0500:

Quoted 3 lines

Hi all,

If no one cares I'll add this to my tree for 5.2.

Which tree are you talking about?

This one:

https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp

Quoted 3 lines

Please let the MTD maintainers take patches through their tree. We might be late but this is definitely not a good reason to bypass us.

Quoted 7 lines

It's a bit confusing when patches are being ignored for more than two months:

https://lore.kernel.org/patchwork/patch/1040099/
https://lore.kernel.org/patchwork/patch/1040100/
https://lore.kernel.org/patchwork/patch/1040098/

Patches posted at -rc6 right before the last release? Come on! Gustavo, we always spend more time for you than for other contributors because we do not trust your changes. We could apply them blindly but we don't do that for other (worthy) contributions, so why shall we do it for you?

I think you could at least flag these changes as "automatic and unverified" in the commit log so that when git blaming, people could know that the additional explicit /* fallthrough */ comment might be wrong and was just added in order to limit the number of warnings when enabling the extra GCC warning.

Quoted 8 lines

Certainly, Richard Weinberger replied to this one. But I couldn't find a tree to which this patch was applied, in case it actually was.

It's a common practice for maintainers to reply saying that a patch has been finally applied, and in most cases they also explicitly mention the tree and branch to which it was applied. All this info is really helpful for people working all over the tree.

It is common practice for contributors to understand what they are doing before submitting a change and this is something that you clearly don't try to do.

Thanks,
Miquèl

#10 lore raw

Hi Miquel,

Quoted 35 lines · Miquel Raynal

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 15 Apr 2019 07:57:11 -0500:

Quoted 22 lines

Hi Miquel,

Quoted 11 lines · Miquel Raynal

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Wed, 10 Apr 2019 16:16:51 -0500:

Quoted 3 lines

Hi all,

If no one cares I'll add this to my tree for 5.2.

Which tree are you talking about?

This one:

https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp

Quoted 3 lines

Please let the MTD maintainers take patches through their tree. We might be late but this is definitely not a good reason to bypass us.

Quoted 7 lines

It's a bit confusing when patches are being ignored for more than two months:

https://lore.kernel.org/patchwork/patch/1040099/
https://lore.kernel.org/patchwork/patch/1040100/
https://lore.kernel.org/patchwork/patch/1040098/

Quoted 5 lines

Patches posted at -rc6 right before the last release? Come on! Gustavo, we always spend more time for you than for other contributors because we do not trust your changes. We could apply them blindly but we don't do that for other (worthy) contributions, so why shall we do it for you?

Oh, I didn't know about that. You don't have to blindly trust me. I sincerely think people should always double check any changes, regardless of their level of trust in a particular person/entity.

Anyway, I really appreciate your sincerity because now I think we can come up with a good strategy to collaborate with each other smoothly.

It seems to me that the root cause for this lack of trust, and, maybe even despite towards these type of patches, is basically misunderstanding of what I'm trying to accomplish and, more importantly, how I do it.

Quoted 6 lines

I think you could at least flag these changes as "automatic and unverified" in the commit log so that when git blaming, people could know that the additional explicit /* fallthrough */ comment might be wrong and was just added in order to limit the number of warnings when enabling the extra GCC warning.

I don't do that because that's not how I'm tackling this task.

I'm not sending these patches with the intention of merely accumulate contributions --and I'm not saying you say so, this is just for clarification-- or because of a lack of more technically interesting things to do in the kernel --this is certainly not the only thing I'm working on. What I'm trying to accomplish is to be able to add -Wimplicit-fallthrough to the build so that the kernel will stay entirely free of this class of bug going forward; is that simple. Now, why is that? because sometimes people forget to place a break/return and a bug is introduced, and it could take up to 7 years to fix it [1].

Now, I really try to determine if I'm dealing with a false positive or an actual bug every time. I read the code and try to understand the context around which each warning is reported. You can tell it's not the most sexy and glamorous thing. And a static analyzer is clearly not sophisticated enough to spot actual bugs in this situation, not even the Coccinelle tool.

I had a similar conversation with a wireless maintainer a while ago. He claimed I was not even looking at the code and that I was blindly using a transformation tool [2]. Please, take a look at it, so you can better understand my workflow.

I have gone through this process of reading code all over the tree and trying to understand it hundreds of times; there were more than 2000 of these warnings at the time I started working on this, and there are are around 50 left in linux-next. Of course, the vast majority of cases have resulted to be obvious false positives, but it's me who have determined that, by auditing each case, so I haven't blindly placed any fall-through comment.

Now, have I made any mistake? Of course! but I have also amended it immediately [3][4]. And the number of bugs I have fixed while working on this task is much bigger. A clear example of how hard this can be is documented in this thread, in which you, being an MTD maintainer, cannot clearly determine if this is a false positive or an actual bug [5]. It can be troublesome for you for a number of reasons --I'm not judging that. I'm trying to illustrate the magnitude of the task as a whole.

So, this patches together with the related bugfixes are part of that whole. And, although sometimes painful for everyone, that whole is what's important, and worth it.

Quoted 4 lines
Quoted 4 lines

Certainly, Richard Weinberger replied to this one. But I couldn't find a tree to which this patch was applied, in case it actually was.

Quoted 9 lines
Quoted 4 lines

It's a common practice for maintainers to reply saying that a patch has been finally applied, and in most cases they also explicitly mention the tree and branch to which it was applied. All this info is really helpful for people working all over the tree.

It is common practice for contributors to understand what they are doing before submitting a change and this is something that you clearly don't try to do.

This is too much to say, and sadly, it's not uncommon for even the most senior people to assume others don't even make an effort to think through their work, before at least asking. But I have already explained myself above.

Regarding this:

Quoted 1 line

Patches posted at -rc6 right before the last release? Come on! Gustavo,

I don't expect people to send an urgent pull-request to merge this patches into mainline as soon as they arrive, and I have never requested such thing.

Lastly, what I really want we *all* get out of this conversation is a better way to collaborate with each other. For me, and I guess for most contributors, it's good enough to have a confirmation that the accepted patch has been applied to a certain branch in a certain tree. I understand this may sound like an special request, in particular because, currently, the number of people working all over the tree is not that big, so it is not that critical for maintainers to adopt certain practices that benefits this small group of contributors, but thanks to recent initiatives as The Linux Kernel Mentorship project I think this is going to change and it will force us all to evolve in the right direction.

By the way, notice that these are the last patches for MTD. :)

Thank you
--
Gustavo

References:

[1] https://lore.kernel.org/patchwork/patch/1042976/
[2] https://lore.kernel.org/patchwork/patch/1002568/
[3] https://lore.kernel.org/patchwork/patch/970617/
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.1-rc5&id=ad0eaee6195db1db1749dd46b9e6f4466793d178
[5] https://lore.kernel.org/patchwork/patch/1036251/

#11 lore raw

Hi all,

Thanks a lot for this, Richard:

https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=mtd%2Fnext&qt=grep&q=fall-through

There are only two of these warnings left to be addressed in MTD[1]:

> @@ -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 */

This looks strange.

In ONENAND_DEVICE_DENSITY_2Gb:
ONENAND_HAS_UNLOCK_ALL is set unconditionally.

But then, under ONENAND_DEVICE_DENSITY_1Gb, the same option is set only if process is evaluated to true.

Same problem with ONENAND_HAS_2PLANE:
- it is set in ONENAND_DEVICE_DENSITY_4Gb only if ONENAND_IS_DDP()
- it is unset in ONENAND_DEVICE_DENSITY_2Gb only if !ONENAND_IS_DDP()

Maybe this portion should be reworked because I am unsure if this is a missing fall through or a bug.

Thanks
--
Gustavo

[1] https://lore.kernel.org/patchwork/patch/1036251/

Quoted 160 lines · Gustavo A. R. Silva

Hi Miquel,

Quoted 35 lines · Miquel Raynal

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Mon, 15 Apr 2019 07:57:11 -0500:

Quoted 22 lines

Hi Miquel,

Quoted 11 lines · Miquel Raynal

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Wed, 10 Apr 2019 16:16:51 -0500:

Quoted 3 lines

Hi all,

If no one cares I'll add this to my tree for 5.2.

Which tree are you talking about?

This one:

https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp

Quoted 3 lines

Please let the MTD maintainers take patches through their tree. We might be late but this is definitely not a good reason to bypass us.

Quoted 7 lines

It's a bit confusing when patches are being ignored for more than two months:

https://lore.kernel.org/patchwork/patch/1040099/
https://lore.kernel.org/patchwork/patch/1040100/
https://lore.kernel.org/patchwork/patch/1040098/

Quoted 5 lines

Patches posted at -rc6 right before the last release? Come on! Gustavo, we always spend more time for you than for other contributors because we do not trust your changes. We could apply them blindly but we don't do that for other (worthy) contributions, so why shall we do it for you?

Oh, I didn't know about that. You don't have to blindly trust me. I sincerely think people should always double check any changes, regardless of their level of trust in a particular person/entity.

Anyway, I really appreciate your sincerity because now I think we can come up with a good strategy to collaborate with each other smoothly.

It seems to me that the root cause for this lack of trust, and, maybe even despite towards these type of patches, is basically misunderstanding of what I'm trying to accomplish and, more importantly, how I do it.

Quoted 6 lines

I think you could at least flag these changes as "automatic and unverified" in the commit log so that when git blaming, people could know that the additional explicit /* fallthrough */ comment might be wrong and was just added in order to limit the number of warnings when enabling the extra GCC warning.

I don't do that because that's not how I'm tackling this task.

I'm not sending these patches with the intention of merely accumulate contributions --and I'm not saying you say so, this is just for clarification-- or because of a lack of more technically interesting things to do in the kernel --this is certainly not the only thing I'm working on. What I'm trying to accomplish is to be able to add -Wimplicit-fallthrough to the build so that the kernel will stay entirely free of this class of bug going forward; is that simple. Now, why is that? because sometimes people forget to place a break/return and a bug is introduced, and it could take up to 7 years to fix it [1].

Now, I really try to determine if I'm dealing with a false positive or an actual bug every time. I read the code and try to understand the context around which each warning is reported. You can tell it's not the most sexy and glamorous thing. And a static analyzer is clearly not sophisticated enough to spot actual bugs in this situation, not even the Coccinelle tool.

I had a similar conversation with a wireless maintainer a while ago. He claimed I was not even looking at the code and that I was blindly using a transformation tool [2]. Please, take a look at it, so you can better understand my workflow.

I have gone through this process of reading code all over the tree and trying to understand it hundreds of times; there were more than 2000 of these warnings at the time I started working on this, and there are are around 50 left in linux-next. Of course, the vast majority of cases have resulted to be obvious false positives, but it's me who have determined that, by auditing each case, so I haven't blindly placed any fall-through comment.

Now, have I made any mistake? Of course! but I have also amended it immediately [3][4]. And the number of bugs I have fixed while working on this task is much bigger. A clear example of how hard this can be is documented in this thread, in which you, being an MTD maintainer, cannot clearly determine if this is a false positive or an actual bug [5]. It can be troublesome for you for a number of reasons --I'm not judging that. I'm trying to illustrate the magnitude of the task as a whole.

So, this patches together with the related bugfixes are part of that whole. And, although sometimes painful for everyone, that whole is what's important, and worth it.

Quoted 4 lines
Quoted 4 lines

Certainly, Richard Weinberger replied to this one. But I couldn't find a tree to which this patch was applied, in case it actually was.

Quoted 9 lines
Quoted 4 lines

It's a common practice for maintainers to reply saying that a patch has been finally applied, and in most cases they also explicitly mention the tree and branch to which it was applied. All this info is really helpful for people working all over the tree.

It is common practice for contributors to understand what they are doing before submitting a change and this is something that you clearly don't try to do.

This is too much to say, and sadly, it's not uncommon for even the most senior people to assume others don't even make an effort to think through their work, before at least asking. But I have already explained myself above.

Regarding this:

Quoted 1 line

Patches posted at -rc6 right before the last release? Come on! Gustavo,

I don't expect people to send an urgent pull-request to merge this patches into mainline as soon as they arrive, and I have never requested such thing.

Lastly, what I really want we *all* get out of this conversation is a better way to collaborate with each other. For me, and I guess for most contributors, it's good enough to have a confirmation that the accepted patch has been applied to a certain branch in a certain tree. I understand this may sound like an special request, in particular because, currently, the number of people working all over the tree is not that big, so it is not that critical for maintainers to adopt certain practices that benefits this small group of contributors, but thanks to recent initiatives as The Linux Kernel Mentorship project I think this is going to change and it will force us all to evolve in the right direction.

By the way, notice that these are the last patches for MTD. :)

Thank you
--
Gustavo

References:

[1] https://lore.kernel.org/patchwork/patch/1042976/
[2] https://lore.kernel.org/patchwork/patch/1002568/
[3] https://lore.kernel.org/patchwork/patch/970617/
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.1-rc5&id=ad0eaee6195db1db1749dd46b9e6f4466793d178
[5] https://lore.kernel.org/patchwork/patch/1036251/

#12 lore raw

----- Ursprüngliche Mail -----

Quoted 46 lines

Von: "Gustavo A. R. Silva" <[email protected]>
An: "Miquel Raynal" <[email protected]>
CC: "David Woodhouse" <[email protected]>, "Brian Norris" <[email protected]>, "Boris Brezillon" <[email protected]>, "Marek Vasut" <[email protected]>, "richard" <[email protected]>, "linux-mtd" <[email protected]>, "linux-kernel" <[email protected]>, "Kees Cook" <[email protected]>
Gesendet: Dienstag, 7. Mai 2019 16:54:12
Betreff: Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs

Hi all,

Thanks a lot for this, Richard:

https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=mtd%2Fnext&qt=grep&q=fall-through

There are only two of these warnings left to be addressed in MTD[1]:

> @@ -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 */

This looks strange.

In ONENAND_DEVICE_DENSITY_2Gb:
ONENAND_HAS_UNLOCK_ALL is set unconditionally.

But then, under ONENAND_DEVICE_DENSITY_1Gb, the same option is set only if process is evaluated to true.

Same problem with ONENAND_HAS_2PLANE:
- it is set in ONENAND_DEVICE_DENSITY_4Gb only if ONENAND_IS_DDP()
- it is unset in ONENAND_DEVICE_DENSITY_2Gb only if !ONENAND_IS_DDP()

Maybe this portion should be reworked because I am unsure if this is a missing fall through or a bug.

Quoted 5 lines

Thanks
--
Gustavo

[1] https://lore.kernel.org/patchwork/patch/1036251/

Did we miss this patch? AFAICT it is in -next too.

Thanks,
//richard

#13 lore raw
Quoted 38 lines · Richard Weinberger
Quoted 38 lines

Hi all,

Thanks a lot for this, Richard:

https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=mtd%2Fnext&qt=grep&q=fall-through

There are only two of these warnings left to be addressed in MTD[1]:

> @@ -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 */

This looks strange.

In ONENAND_DEVICE_DENSITY_2Gb:
ONENAND_HAS_UNLOCK_ALL is set unconditionally.

But then, under ONENAND_DEVICE_DENSITY_1Gb, the same option is set only if process is evaluated to true.

Same problem with ONENAND_HAS_2PLANE:
- it is set in ONENAND_DEVICE_DENSITY_4Gb only if ONENAND_IS_DDP()
- it is unset in ONENAND_DEVICE_DENSITY_2Gb only if !ONENAND_IS_DDP()

Maybe this portion should be reworked because I am unsure if this is a missing fall through or a bug.

Quoted 8 lines
Quoted 5 lines

Thanks
--
Gustavo

[1] https://lore.kernel.org/patchwork/patch/1036251/

Did we miss this patch? AFAICT it is in -next too.

What is pending to be resolved are these 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:
^~~~

The final version of the patch in -next does not address them.

Thanks
--
Gustavo

maintainer
#14 lore raw

Hi Gustavo,

"Gustavo A. R. Silva" <[email protected]> wrote on Tue, 7 May 2019 10:59:38 -0500:

Quoted 64 lines · Richard Weinberger
Quoted 39 lines
Quoted 39 lines

Hi all,

Thanks a lot for this, Richard:

https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/log/?h=mtd%2Fnext&qt=grep&q=fall-through

There are only two of these warnings left to be addressed in MTD[1]:

> @@ -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 */

This looks strange.

In ONENAND_DEVICE_DENSITY_2Gb:
ONENAND_HAS_UNLOCK_ALL is set unconditionally.

But then, under ONENAND_DEVICE_DENSITY_1Gb, the same option is set only if process is evaluated to true.

Same problem with ONENAND_HAS_2PLANE:
- it is set in ONENAND_DEVICE_DENSITY_4Gb only if ONENAND_IS_DDP()
- it is unset in ONENAND_DEVICE_DENSITY_2Gb only if !ONENAND_IS_DDP()

Maybe this portion should be reworked because I am unsure if this is a missing fall through or a bug.

Quoted 8 lines
Quoted 5 lines

Thanks
--
Gustavo

[1] https://lore.kernel.org/patchwork/patch/1036251/

Did we miss this patch? AFAICT it is in -next too.

What is pending to be resolved are these 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:
^~~~

Quoted 2 lines

The final version of the patch in -next does not address them.

Send a commit for these two warnings stating very clearly close to the top of the commit log that we don't know whether we need fallthroughs or breaks there and that this is just a change to avoid having new warnings when switching to -Wimplicit-fallthrough but this change might be entirely wrong.

Thanks,
Miquèl

Reviewed-by on v1 1/1
#15 lore raw
In reply to Gustavo A. R. Silva
Quoted 33 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This patch fixes the following warnings:

drivers/mtd/chips/cfi_util.c: In function ‘cfi_build_cmd’:
drivers/mtd/chips/cfi_util.c:110:10: warning: this statement may fall through [-Wimplicit-fallthrough=]

   onecmd |= (onecmd << (chip_mode * 32));
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:112:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:113:10: warning: this statement may fall through [-Wimplicit-fallthrough=]

   onecmd |= (onecmd << (chip_mode * 16));
   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:114:2: note: here
case 2:
^~~~
drivers/mtd/chips/cfi_util.c: In function ‘cfi_merge_status’:
drivers/mtd/chips/cfi_util.c:163:7: warning: this statement may fall through [-Wimplicit-fallthrough=]

   res |= (onestat >> (chip_mode * 32));
   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:165:2: note: here
case 4:
^~~~
drivers/mtd/chips/cfi_util.c:166:7: warning: this statement may fall through [-Wimplicit-fallthrough=]

   res |= (onestat >> (chip_mode * 16));
   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

drivers/mtd/chips/cfi_util.c:167:2: note: here
case 2:
^~~~

Quoted 6 lines

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

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

-Kees

Quoted 39 lines
---
 drivers/mtd/chips/cfi_util.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/chips/cfi_util.c +6 -1
index 6f16552cd59f..e3b266ee06af 100644
@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cfi
        case 8:
                onecmd |= (onecmd << (chip_mode * 32));
 #endif
+               /* fall through */
        case 4:
                onecmd |= (onecmd << (chip_mode * 16));
+               /* fall through */
        case 2:
                onecmd |= (onecmd << (chip_mode * 8));
+               /* fall through */
        case 1:
                ;
        }
@@ -162,10 +165,13 @@ unsigned long cfi_merge_status(map_word val, struct map_info *map,
        case 8:
                res |= (onestat >> (chip_mode * 32));
 #endif
+               /* fall through */
        case 4:
                res |= (onestat >> (chip_mode * 16));
+               /* fall through */
        case 2:
                res |= (onestat >> (chip_mode * 8));
+               /* fall through */
        case 1:
                ;
        }
--

2.20.1

Signature · 1 line
Kees Cook