Linux-mtd Archive on lore.kernel.org
 help / Atom feed
* [PATCH] mtd: cfi_util: mark expected switch fall-throughs
@ 2019-02-08 18:02 Gustavo A. R. Silva
  2019-03-20 20:20 ` Gustavo A. R. Silva
  2019-04-10 21:46 ` Kees Cook
  0 siblings, 2 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-02-08 18:02 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
	Richard Weinberger
  Cc: linux-mtd, linux-kernel, 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 <gustavo@embeddedor.com>
---
 drivers/mtd/chips/cfi_util.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
index 6f16552cd59f..e3b266ee06af 100644
--- a/drivers/mtd/chips/cfi_util.c
+++ b/drivers/mtd/chips/cfi_util.c
@@ -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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-02-08 18:02 [PATCH] mtd: cfi_util: mark expected switch fall-throughs Gustavo A. R. Silva
@ 2019-03-20 20:20 ` Gustavo A. R. Silva
  2019-03-20 21:05   ` Richard Weinberger
  2019-04-10 21:16   ` Gustavo A. R. Silva
  2019-04-10 21:46 ` Kees Cook
  1 sibling, 2 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-03-20 20:20 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
	Richard Weinberger
  Cc: linux-mtd, linux-kernel, Kees Cook

Hi all,

Friendly ping:

Who can take this?

Thanks
--
Gustavo

On 2/8/19 12:02 PM, Gustavo A. R. Silva wrote:
> 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 <gustavo@embeddedor.com>
> ---
>  drivers/mtd/chips/cfi_util.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
> index 6f16552cd59f..e3b266ee06af 100644
> --- a/drivers/mtd/chips/cfi_util.c
> +++ b/drivers/mtd/chips/cfi_util.c
> @@ -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:
>  		;
>  	}
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-03-20 20:20 ` Gustavo A. R. Silva
@ 2019-03-20 21:05   ` Richard Weinberger
  2019-03-20 21:23     ` Gustavo A. R. Silva
  2019-04-10 21:16   ` Gustavo A. R. Silva
  1 sibling, 1 reply; 16+ messages in thread
From: Richard Weinberger @ 2019-03-20 21:05 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Kees Cook, Boris Brezillon, linux-kernel, Marek Vasut, linux-mtd,
	Brian Norris, David Woodhouse

Hi!

Am Mittwoch, 20. März 2019, 21:20:51 CET schrieb Gustavo A. R. Silva:
> 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





______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-03-20 21:05   ` Richard Weinberger
@ 2019-03-20 21:23     ` Gustavo A. R. Silva
  2019-03-20 23:25       ` Richard Weinberger
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-03-20 21:23 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Kees Cook, Boris Brezillon, linux-kernel, Marek Vasut, linux-mtd,
	Brian Norris, David Woodhouse



On 3/20/19 4:05 PM, Richard Weinberger wrote:
> Hi!
> 
> Am Mittwoch, 20. März 2019, 21:20:51 CET schrieb Gustavo A. R. Silva:
>> 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.
> 

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-03-20 21:23     ` Gustavo A. R. Silva
@ 2019-03-20 23:25       ` Richard Weinberger
  2019-03-20 23:36         ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Weinberger @ 2019-03-20 23:25 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Kees Cook, Boris Brezillon, linux-kernel, Marek Vasut, linux-mtd,
	Brian Norris, David Woodhouse

Am Mittwoch, 20. März 2019, 22:23:21 CET schrieb Gustavo A. R. Silva:
> 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



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-03-20 23:25       ` Richard Weinberger
@ 2019-03-20 23:36         ` Gustavo A. R. Silva
  0 siblings, 0 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-03-20 23:36 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Kees Cook, Boris Brezillon, linux-kernel, Marek Vasut, linux-mtd,
	Brian Norris, David Woodhouse



On 3/20/19 6:25 PM, Richard Weinberger wrote:
> Am Mittwoch, 20. März 2019, 22:23:21 CET schrieb Gustavo A. R. Silva:
>> 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.

> 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-03-20 20:20 ` Gustavo A. R. Silva
  2019-03-20 21:05   ` Richard Weinberger
@ 2019-04-10 21:16   ` Gustavo A. R. Silva
  2019-04-15  8:44     ` Miquel Raynal
  1 sibling, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-10 21:16 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
	Richard Weinberger
  Cc: linux-mtd, linux-kernel, Kees Cook

Hi all,

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

Thanks
--
Gustavo

On 3/20/19 3:20 PM, Gustavo A. R. Silva wrote:
> Hi all,
> 
> Friendly ping:
> 
> Who can take this?
> 
> Thanks
> --
> Gustavo
> 
> On 2/8/19 12:02 PM, Gustavo A. R. Silva wrote:
>> 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 <gustavo@embeddedor.com>
>> ---
>>  drivers/mtd/chips/cfi_util.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
>> index 6f16552cd59f..e3b266ee06af 100644
>> --- a/drivers/mtd/chips/cfi_util.c
>> +++ b/drivers/mtd/chips/cfi_util.c
>> @@ -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:
>>  		;
>>  	}
>>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-02-08 18:02 [PATCH] mtd: cfi_util: mark expected switch fall-throughs Gustavo A. R. Silva
  2019-03-20 20:20 ` Gustavo A. R. Silva
@ 2019-04-10 21:46 ` Kees Cook
  1 sibling, 0 replies; 16+ messages in thread
From: Kees Cook @ 2019-04-10 21:46 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Boris Brezillon, Richard Weinberger, LKML, Marek Vasut,
	Linux mtd, Brian Norris, David Woodhouse

On Fri, Feb 8, 2019 at 10:02 AM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
> 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 <gustavo@embeddedor.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  drivers/mtd/chips/cfi_util.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
> index 6f16552cd59f..e3b266ee06af 100644
> --- a/drivers/mtd/chips/cfi_util.c
> +++ b/drivers/mtd/chips/cfi_util.c
> @@ -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
>


-- 
Kees Cook

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-04-10 21:16   ` Gustavo A. R. Silva
@ 2019-04-15  8:44     ` Miquel Raynal
  2019-04-15 12:57       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Miquel Raynal @ 2019-04-15  8:44 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Kees Cook, Boris Brezillon, Richard Weinberger, linux-kernel,
	Marek Vasut, linux-mtd, Brian Norris, David Woodhouse

Hi Gustavo,

"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Wed, 10 Apr
2019 16:16:51 -0500:

> 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-04-15  8:44     ` Miquel Raynal
@ 2019-04-15 12:57       ` Gustavo A. R. Silva
  2019-04-16 17:24         ` Miquel Raynal
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-15 12:57 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Kees Cook, Boris Brezillon, Richard Weinberger, linux-kernel,
	Marek Vasut, linux-mtd, Brian Norris, David Woodhouse

Hi Miquel,

On 4/15/19 3:44 AM, Miquel Raynal wrote:
> Hi Gustavo,
> 
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Wed, 10 Apr
> 2019 16:16:51 -0500:
> 
>> 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

> 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




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-04-15 12:57       ` Gustavo A. R. Silva
@ 2019-04-16 17:24         ` Miquel Raynal
  2019-04-16 20:49           ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Miquel Raynal @ 2019-04-16 17:24 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Kees Cook, Boris Brezillon, Richard Weinberger, linux-kernel,
	Marek Vasut, linux-mtd, Brian Norris, David Woodhouse

Hi Gustavo,

"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Mon, 15 Apr
2019 07:57:11 -0500:

> Hi Miquel,
> 
> On 4/15/19 3:44 AM, Miquel Raynal wrote:
> > Hi Gustavo,
> > 
> > "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Wed, 10 Apr
> > 2019 16:16:51 -0500:
> >   
> >> 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
> 
> > 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/
> 

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.

> 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-04-16 17:24         ` Miquel Raynal
@ 2019-04-16 20:49           ` Gustavo A. R. Silva
  2019-05-07 14:54             ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-16 20:49 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Kees Cook, Boris Brezillon, Richard Weinberger, linux-kernel,
	Marek Vasut, linux-mtd, Brian Norris, David Woodhouse

Hi Miquel,

On 4/16/19 12:24 PM, Miquel Raynal wrote:
> Hi Gustavo,
> 
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Mon, 15 Apr
> 2019 07:57:11 -0500:
> 
>> Hi Miquel,
>>
>> On 4/15/19 3:44 AM, Miquel Raynal wrote:
>>> Hi Gustavo,
>>>
>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Wed, 10 Apr
>>> 2019 16:16:51 -0500:
>>>   
>>>> 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
>>
>>> 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/
>>
> 
> 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.

> 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.

>> 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.
> 

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:

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




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-04-16 20:49           ` Gustavo A. R. Silva
@ 2019-05-07 14:54             ` Gustavo A. R. Silva
  2019-05-07 15:49               ` Richard Weinberger
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-05-07 14:54 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Kees Cook, Boris Brezillon, Richard Weinberger, linux-kernel,
	Marek Vasut, linux-mtd, Brian Norris, David Woodhouse

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/

On 4/16/19 3:49 PM, Gustavo A. R. Silva wrote:
> Hi Miquel,
> 
> On 4/16/19 12:24 PM, Miquel Raynal wrote:
>> Hi Gustavo,
>>
>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Mon, 15 Apr
>> 2019 07:57:11 -0500:
>>
>>> Hi Miquel,
>>>
>>> On 4/15/19 3:44 AM, Miquel Raynal wrote:
>>>> Hi Gustavo,
>>>>
>>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Wed, 10 Apr
>>>> 2019 16:16:51 -0500:
>>>>   
>>>>> 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
>>>
>>>> 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/
>>>
>>
>> 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.
> 
>> 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.
> 
>>> 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.
>>
> 
> 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:
> 
>> 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/
> 
> 
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-05-07 14:54             ` Gustavo A. R. Silva
@ 2019-05-07 15:49               ` Richard Weinberger
  2019-05-07 15:59                 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Weinberger @ 2019-05-07 15:49 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Kees Cook, Boris Brezillon, linux-kernel, Marek Vasut, linux-mtd,
	Miquel Raynal, Brian Norris, David Woodhouse

----- Ursprüngliche Mail -----
> Von: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
> An: "Miquel Raynal" <miquel.raynal@bootlin.com>
> CC: "David Woodhouse" <dwmw2@infradead.org>, "Brian Norris" <computersforpeace@gmail.com>, "Boris Brezillon"
> <bbrezillon@kernel.org>, "Marek Vasut" <marek.vasut@gmail.com>, "richard" <richard@nod.at>, "linux-mtd"
> <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "Kees Cook" <keescook@chromium.org>
> 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.
> 
> 
> Thanks
> --
> Gustavo
> 
> [1] https://lore.kernel.org/patchwork/patch/1036251/

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

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-05-07 15:49               ` Richard Weinberger
@ 2019-05-07 15:59                 ` Gustavo A. R. Silva
  2019-05-09  6:53                   ` Miquel Raynal
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2019-05-07 15:59 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: Kees Cook, Boris Brezillon, linux-kernel, Marek Vasut, linux-mtd,
	Miquel Raynal, Brian Norris, David Woodhouse



On 5/7/19 10:49 AM, Richard Weinberger wrote:

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs
  2019-05-07 15:59                 ` Gustavo A. R. Silva
@ 2019-05-09  6:53                   ` Miquel Raynal
  0 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2019-05-09  6:53 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Kees Cook, Boris Brezillon, Richard Weinberger, linux-kernel,
	Marek Vasut, linux-mtd, Brian Norris, David Woodhouse

Hi Gustavo,

"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Tue, 7 May 2019
10:59:38 -0500:

> On 5/7/19 10:49 AM, Richard Weinberger wrote:
> 
> >> 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/  
> > 
> > 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.
> 

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08 18:02 [PATCH] mtd: cfi_util: mark expected switch fall-throughs Gustavo A. R. Silva
2019-03-20 20:20 ` Gustavo A. R. Silva
2019-03-20 21:05   ` Richard Weinberger
2019-03-20 21:23     ` Gustavo A. R. Silva
2019-03-20 23:25       ` Richard Weinberger
2019-03-20 23:36         ` Gustavo A. R. Silva
2019-04-10 21:16   ` Gustavo A. R. Silva
2019-04-15  8:44     ` Miquel Raynal
2019-04-15 12:57       ` Gustavo A. R. Silva
2019-04-16 17:24         ` Miquel Raynal
2019-04-16 20:49           ` Gustavo A. R. Silva
2019-05-07 14:54             ` Gustavo A. R. Silva
2019-05-07 15:49               ` Richard Weinberger
2019-05-07 15:59                 ` Gustavo A. R. Silva
2019-05-09  6:53                   ` Miquel Raynal
2019-04-10 21:46 ` Kees Cook

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org linux-mtd@archiver.kernel.org
	public-inbox-index linux-mtd


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/ public-inbox