mtd: cfi_util: mark expected switch fall-throughs
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
@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cficase 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
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.
Signed-off-by Gustavo A. R. Silva <[email protected]>--- drivers/mtd/chips/cfi_util.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/mtd/chips/cfi_util.c +6
-0@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cficase 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 : ; }
Richard Weinberger
maintainerHi!
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
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
Richard Weinberger
maintainerAm 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
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
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
--
GustavoQuoted 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.
Signed-off-by Gustavo A. R. Silva <[email protected]>--- drivers/mtd/chips/cfi_util.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/mtd/chips/cfi_util.c +6
-0@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cficase 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 : ; }
Miquel Raynal
maintainerHi 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
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
Miquel Raynal
maintainerHi 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
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/
Hi all,
Thanks a lot for this, Richard:
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
--
GustavoReferences:
[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/
Richard Weinberger
maintainer----- 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-throughsHi all,
Thanks a lot for this, Richard:
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 */
>
> caseONENAND_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_ALLis 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 inONENAND_DEVICE_DENSITY_4Gbonly ifONENAND_IS_DDP()
- it is unset inONENAND_DEVICE_DENSITY_2Gbonly 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
Did we miss this patch? AFAICT it is in -next too.
Thanks,
//richard
Quoted 38 lines · Richard Weinberger
Quoted 38 lines
Hi all,
Thanks a lot for this, Richard:
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 */
>
> caseONENAND_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_ALLis 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 inONENAND_DEVICE_DENSITY_4Gbonly ifONENAND_IS_DDP()
- it is unset inONENAND_DEVICE_DENSITY_2Gbonly 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
--
GustavoDid 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
Miquel Raynal
maintainerHi 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:
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 */
>
> caseONENAND_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_ALLis 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 inONENAND_DEVICE_DENSITY_4Gbonly ifONENAND_IS_DDP()
- it is unset inONENAND_DEVICE_DENSITY_2Gbonly 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
--
GustavoDid 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
caseONENAND_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
caseONENAND_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
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.
Signed-off-by Gustavo A. R. Silva <[email protected]>
-Kees
Quoted 39 lines
--- drivers/mtd/chips/cfi_util.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/mtd/chips/cfi_util.c +6
-1@@ -109,10 +109,13 @@ map_word cfi_build_cmd(u_long cmd, struct map_info *map, struct cfi_private *cficase 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