* [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-11 18:15 ` Gustavo A. R. Silva
0 siblings, 0 replies; 20+ messages in thread
From: Gustavo A. R. Silva @ 2019-02-11 18:15 UTC (permalink / raw)
To: Kukjin Kim, Krzysztof Kozlowski, Sylwester Nawrocki, Tomasz Figa,
Chanwoo Choi, Michael Turquette, Stephen Boyd
Cc: linux-samsung-soc, Kees Cook, Gustavo A. R. Silva, linux-kernel,
linux-clk, linux-arm-kernel
In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.
This patch fixes the following warnings:
drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
samsung_clk_register_alias(ctx, s3c2450_aliases,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ARRAY_SIZE(s3c2450_aliases));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
case S3C2416:
^~~~
Warning level 3 was used: -Wimplicit-fallthrough=3
Notice that, in this particular case, the code comment is modified
in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/clk/samsung/clk-s3c2443.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/samsung/clk-s3c2443.c b/drivers/clk/samsung/clk-s3c2443.c
index 884067e4f1a1..f38f0e24e3b6 100644
--- a/drivers/clk/samsung/clk-s3c2443.c
+++ b/drivers/clk/samsung/clk-s3c2443.c
@@ -389,7 +389,7 @@ void __init s3c2443_common_clk_init(struct device_node *np, unsigned long xti_f,
ARRAY_SIZE(s3c2450_gates));
samsung_clk_register_alias(ctx, s3c2450_aliases,
ARRAY_SIZE(s3c2450_aliases));
- /* fall through, as s3c2450 extends the s3c2416 clocks */
+ /* fall through - as s3c2450 extends the s3c2416 clocks */
case S3C2416:
samsung_clk_register_div(ctx, s3c2416_dividers,
ARRAY_SIZE(s3c2416_dividers));
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-11 18:15 ` Gustavo A. R. Silva
@ 2019-02-12 0:37 ` Chanwoo Choi
-1 siblings, 0 replies; 20+ messages in thread
From: Chanwoo Choi @ 2019-02-12 0:37 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kukjin Kim, Krzysztof Kozlowski,
Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd
Cc: linux-arm-kernel, linux-samsung-soc, linux-clk, linux-kernel,
Kees Cook
Hi Gustavo,
On 19. 2. 12. 오전 3:15, 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/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
Except for level 5 of -Wimplicit-fallthrough,
level 4 is more strict to show the warnings.
Why don't you support level 4 for -Wimplicit-fallthrough?
I think that you want to fix for -Wimplicit-fallthrough warning,
you better to support level 4. What do you think?
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/clk/samsung/clk-s3c2443.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/samsung/clk-s3c2443.c b/drivers/clk/samsung/clk-s3c2443.c
> index 884067e4f1a1..f38f0e24e3b6 100644
> --- a/drivers/clk/samsung/clk-s3c2443.c
> +++ b/drivers/clk/samsung/clk-s3c2443.c
> @@ -389,7 +389,7 @@ void __init s3c2443_common_clk_init(struct device_node *np, unsigned long xti_f,
> ARRAY_SIZE(s3c2450_gates));
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ARRAY_SIZE(s3c2450_aliases));
> - /* fall through, as s3c2450 extends the s3c2416 clocks */
> + /* fall through - as s3c2450 extends the s3c2416 clocks */
> case S3C2416:
> samsung_clk_register_div(ctx, s3c2416_dividers,
> ARRAY_SIZE(s3c2416_dividers));
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-12 0:37 ` Chanwoo Choi
0 siblings, 0 replies; 20+ messages in thread
From: Chanwoo Choi @ 2019-02-12 0:37 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kukjin Kim, Krzysztof Kozlowski,
Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd
Cc: Kees Cook, linux-samsung-soc, linux-clk, linux-arm-kernel,
linux-kernel
Hi Gustavo,
On 19. 2. 12. 오전 3:15, 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/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
Except for level 5 of -Wimplicit-fallthrough,
level 4 is more strict to show the warnings.
Why don't you support level 4 for -Wimplicit-fallthrough?
I think that you want to fix for -Wimplicit-fallthrough warning,
you better to support level 4. What do you think?
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/clk/samsung/clk-s3c2443.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/samsung/clk-s3c2443.c b/drivers/clk/samsung/clk-s3c2443.c
> index 884067e4f1a1..f38f0e24e3b6 100644
> --- a/drivers/clk/samsung/clk-s3c2443.c
> +++ b/drivers/clk/samsung/clk-s3c2443.c
> @@ -389,7 +389,7 @@ void __init s3c2443_common_clk_init(struct device_node *np, unsigned long xti_f,
> ARRAY_SIZE(s3c2450_gates));
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ARRAY_SIZE(s3c2450_aliases));
> - /* fall through, as s3c2450 extends the s3c2416 clocks */
> + /* fall through - as s3c2450 extends the s3c2416 clocks */
> case S3C2416:
> samsung_clk_register_div(ctx, s3c2416_dividers,
> ARRAY_SIZE(s3c2416_dividers));
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-11 18:15 ` Gustavo A. R. Silva
@ 2019-02-12 7:40 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2019-02-12 7:40 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Kukjin Kim, Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi,
Michael Turquette, Stephen Boyd, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, linux-kernel,
Kees Cook
On Mon, 11 Feb 2019 at 19:40, 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/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
I saw this in multiple places already and I think fix is wrong. The
point is that the code is correct - the fall through is marked
properly.
It is just the GCC which has to be fixed not the code. You want to
adjust the code for specific version of GCC and what if GCC changes
its warning? For example GCC might require "fall through: "... or any
other syntax. Another point - what about clang's syntax?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-12 7:40 ` Krzysztof Kozlowski
0 siblings, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2019-02-12 7:40 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: linux-samsung-soc@vger.kernel.org, Kees Cook, Stephen Boyd,
Michael Turquette, Tomasz Figa, linux-kernel, Chanwoo Choi,
Kukjin Kim, Sylwester Nawrocki, linux-clk, linux-arm-kernel
On Mon, 11 Feb 2019 at 19:40, 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/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
I saw this in multiple places already and I think fix is wrong. The
point is that the code is correct - the fall through is marked
properly.
It is just the GCC which has to be fixed not the code. You want to
adjust the code for specific version of GCC and what if GCC changes
its warning? For example GCC might require "fall through: "... or any
other syntax. Another point - what about clang's syntax?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-12 7:40 ` Krzysztof Kozlowski
@ 2019-02-12 18:57 ` Kees Cook
-1 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2019-02-12 18:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Gustavo A. R. Silva, Kukjin Kim, Sylwester Nawrocki, Tomasz Figa,
Chanwoo Choi, Michael Turquette, Stephen Boyd, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, LKML
On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, 11 Feb 2019 at 19:40, 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/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> > drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> > samsung_clk_register_alias(ctx, s3c2450_aliases,
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ARRAY_SIZE(s3c2450_aliases));
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> > case S3C2416:
> > ^~~~
> >
> > Warning level 3 was used: -Wimplicit-fallthrough=3
> >
> > Notice that, in this particular case, the code comment is modified
> > in accordance with what GCC is expecting to find.
> >
> > This patch is part of the ongoing efforts to enable
> > -Wimplicit-fallthrough.
>
> I saw this in multiple places already and I think fix is wrong. The
> point is that the code is correct - the fall through is marked
> properly.
>
> It is just the GCC which has to be fixed not the code. You want to
> adjust the code for specific version of GCC and what if GCC changes
> its warning? For example GCC might require "fall through: "... or any
> other syntax. Another point - what about clang's syntax?
-Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
choice. GCC's levels were chosen based on the existing linters, static
analyzers, etc. The patterns are unlikely to change (see the gcc
man-page).
Clang doesn't recognize anything in C mode (hopefully this will be
fixed in the future[1]).
As long as one of the compilers is able to check this, we'll avoid the
bugs associated with this mis-pattern. Gustavo's efforts here have
found kind of a lot of bugs, so I think it's worth a little churn to
add these (and make minor adjustments to existing comments).
[1] https://bugs.llvm.org/show_bug.cgi?id=37135
--
Kees Cook
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-12 18:57 ` Kees Cook
0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2019-02-12 18:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-samsung-soc@vger.kernel.org, Gustavo A. R. Silva,
Stephen Boyd, Michael Turquette, Tomasz Figa, LKML, Chanwoo Choi,
Kukjin Kim, Sylwester Nawrocki, linux-clk, linux-arm-kernel
On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, 11 Feb 2019 at 19:40, 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/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> > drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> > samsung_clk_register_alias(ctx, s3c2450_aliases,
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ARRAY_SIZE(s3c2450_aliases));
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> > case S3C2416:
> > ^~~~
> >
> > Warning level 3 was used: -Wimplicit-fallthrough=3
> >
> > Notice that, in this particular case, the code comment is modified
> > in accordance with what GCC is expecting to find.
> >
> > This patch is part of the ongoing efforts to enable
> > -Wimplicit-fallthrough.
>
> I saw this in multiple places already and I think fix is wrong. The
> point is that the code is correct - the fall through is marked
> properly.
>
> It is just the GCC which has to be fixed not the code. You want to
> adjust the code for specific version of GCC and what if GCC changes
> its warning? For example GCC might require "fall through: "... or any
> other syntax. Another point - what about clang's syntax?
-Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
choice. GCC's levels were chosen based on the existing linters, static
analyzers, etc. The patterns are unlikely to change (see the gcc
man-page).
Clang doesn't recognize anything in C mode (hopefully this will be
fixed in the future[1]).
As long as one of the compilers is able to check this, we'll avoid the
bugs associated with this mis-pattern. Gustavo's efforts here have
found kind of a lot of bugs, so I think it's worth a little churn to
add these (and make minor adjustments to existing comments).
[1] https://bugs.llvm.org/show_bug.cgi?id=37135
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-12 18:57 ` Kees Cook
(?)
@ 2019-02-16 0:34 ` Stephen Boyd
-1 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-16 0:34 UTC (permalink / raw)
To: Kees Cook, Krzysztof Kozlowski
Cc: Gustavo A. R. Silva, Kukjin Kim, Sylwester Nawrocki, Tomasz Figa,
Chanwoo Choi, Michael Turquette, linux-arm-kernel,
linux-samsung-soc@vger.kernel.org, linux-clk, LKML
Quoting Kees Cook (2019-02-12 10:57:05)
> On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > It is just the GCC which has to be fixed not the code. You want to
> > adjust the code for specific version of GCC and what if GCC changes
> > its warning? For example GCC might require "fall through: "... or any
> > other syntax. Another point - what about clang's syntax?
>
> -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> choice. GCC's levels were chosen based on the existing linters, static
> analyzers, etc. The patterns are unlikely to change (see the gcc
> man-page).
>
> Clang doesn't recognize anything in C mode (hopefully this will be
> fixed in the future[1]).
>
> As long as one of the compilers is able to check this, we'll avoid the
> bugs associated with this mis-pattern. Gustavo's efforts here have
> found kind of a lot of bugs, so I think it's worth a little churn to
> add these (and make minor adjustments to existing comments).
Just curious, what compilation phase does this check run in? Could we
gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
whatever the compiler wants to see and then there would only be "one
way" to do this? It would alleviate the above concerns, but maybe I'm
rehashing something that's already been proposed and rejected.
Of course, I'm happy to merge any of these patches that tweak things so
no worries either way.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-16 0:34 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-16 0:34 UTC (permalink / raw)
To: Kees Cook, Krzysztof Kozlowski
Cc: linux-samsung-soc@vger.kernel.org, Gustavo A. R. Silva,
Michael Turquette, Tomasz Figa, LKML, Chanwoo Choi, Kukjin Kim,
Sylwester Nawrocki, linux-clk, linux-arm-kernel
Quoting Kees Cook (2019-02-12 10:57:05)
> On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > It is just the GCC which has to be fixed not the code. You want to
> > adjust the code for specific version of GCC and what if GCC changes
> > its warning? For example GCC might require "fall through: "... or any
> > other syntax. Another point - what about clang's syntax?
>
> -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> choice. GCC's levels were chosen based on the existing linters, static
> analyzers, etc. The patterns are unlikely to change (see the gcc
> man-page).
>
> Clang doesn't recognize anything in C mode (hopefully this will be
> fixed in the future[1]).
>
> As long as one of the compilers is able to check this, we'll avoid the
> bugs associated with this mis-pattern. Gustavo's efforts here have
> found kind of a lot of bugs, so I think it's worth a little churn to
> add these (and make minor adjustments to existing comments).
Just curious, what compilation phase does this check run in? Could we
gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
whatever the compiler wants to see and then there would only be "one
way" to do this? It would alleviate the above concerns, but maybe I'm
rehashing something that's already been proposed and rejected.
Of course, I'm happy to merge any of these patches that tweak things so
no worries either way.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-16 0:34 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-16 0:34 UTC (permalink / raw)
To: Kees Cook, Krzysztof Kozlowski
Cc: linux-samsung-soc@vger.kernel.org, Gustavo A. R. Silva,
Michael Turquette, Tomasz Figa, LKML, Chanwoo Choi, Kukjin Kim,
Sylwester Nawrocki, linux-clk, linux-arm-kernel
Quoting Kees Cook (2019-02-12 10:57:05)
> On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > It is just the GCC which has to be fixed not the code. You want to
> > adjust the code for specific version of GCC and what if GCC changes
> > its warning? For example GCC might require "fall through: "... or any
> > other syntax. Another point - what about clang's syntax?
>
> -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> choice. GCC's levels were chosen based on the existing linters, static
> analyzers, etc. The patterns are unlikely to change (see the gcc
> man-page).
>
> Clang doesn't recognize anything in C mode (hopefully this will be
> fixed in the future[1]).
>
> As long as one of the compilers is able to check this, we'll avoid the
> bugs associated with this mis-pattern. Gustavo's efforts here have
> found kind of a lot of bugs, so I think it's worth a little churn to
> add these (and make minor adjustments to existing comments).
Just curious, what compilation phase does this check run in? Could we
gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
whatever the compiler wants to see and then there would only be "one
way" to do this? It would alleviate the above concerns, but maybe I'm
rehashing something that's already been proposed and rejected.
Of course, I'm happy to merge any of these patches that tweak things so
no worries either way.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-16 0:34 ` Stephen Boyd
@ 2019-02-20 22:26 ` Kees Cook
-1 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2019-02-20 22:26 UTC (permalink / raw)
To: Stephen Boyd
Cc: Krzysztof Kozlowski, Gustavo A. R. Silva, Kukjin Kim,
Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
linux-arm-kernel, linux-samsung-soc@vger.kernel.org, linux-clk,
LKML
On Fri, Feb 15, 2019 at 4:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Kees Cook (2019-02-12 10:57:05)
> > On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > It is just the GCC which has to be fixed not the code. You want to
> > > adjust the code for specific version of GCC and what if GCC changes
> > > its warning? For example GCC might require "fall through: "... or any
> > > other syntax. Another point - what about clang's syntax?
> >
> > -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> > choice. GCC's levels were chosen based on the existing linters, static
> > analyzers, etc. The patterns are unlikely to change (see the gcc
> > man-page).
> >
> > Clang doesn't recognize anything in C mode (hopefully this will be
> > fixed in the future[1]).
> >
> > As long as one of the compilers is able to check this, we'll avoid the
> > bugs associated with this mis-pattern. Gustavo's efforts here have
> > found kind of a lot of bugs, so I think it's worth a little churn to
> > add these (and make minor adjustments to existing comments).
>
> Just curious, what compilation phase does this check run in? Could we
> gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
> whatever the compiler wants to see and then there would only be "one
> way" to do this? It would alleviate the above concerns, but maybe I'm
> rehashing something that's already been proposed and rejected.
When this got discussed a while back, the thinking was that since
we're also dealing with static analyzers (e.g. Coverity) and IDEs that
literally parse comments in the code, it was most sensible (at least
for now, prior to there being a formal C "fall through" statement --
there is for C++ but not yet for C), we'd stick to explicit comments.
In theory, we will be able to do a tree-wide change to add the C
statement once it exists.
> Of course, I'm happy to merge any of these patches that tweak things so
> no worries either way.
Thanks! :)
--
Kees Cook
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-20 22:26 ` Kees Cook
0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2019-02-20 22:26 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-samsung-soc@vger.kernel.org, Gustavo A. R. Silva,
Michael Turquette, Tomasz Figa, Krzysztof Kozlowski, LKML,
Chanwoo Choi, Kukjin Kim, Sylwester Nawrocki, linux-clk,
linux-arm-kernel
On Fri, Feb 15, 2019 at 4:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Kees Cook (2019-02-12 10:57:05)
> > On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > It is just the GCC which has to be fixed not the code. You want to
> > > adjust the code for specific version of GCC and what if GCC changes
> > > its warning? For example GCC might require "fall through: "... or any
> > > other syntax. Another point - what about clang's syntax?
> >
> > -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> > choice. GCC's levels were chosen based on the existing linters, static
> > analyzers, etc. The patterns are unlikely to change (see the gcc
> > man-page).
> >
> > Clang doesn't recognize anything in C mode (hopefully this will be
> > fixed in the future[1]).
> >
> > As long as one of the compilers is able to check this, we'll avoid the
> > bugs associated with this mis-pattern. Gustavo's efforts here have
> > found kind of a lot of bugs, so I think it's worth a little churn to
> > add these (and make minor adjustments to existing comments).
>
> Just curious, what compilation phase does this check run in? Could we
> gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
> whatever the compiler wants to see and then there would only be "one
> way" to do this? It would alleviate the above concerns, but maybe I'm
> rehashing something that's already been proposed and rejected.
When this got discussed a while back, the thinking was that since
we're also dealing with static analyzers (e.g. Coverity) and IDEs that
literally parse comments in the code, it was most sensible (at least
for now, prior to there being a formal C "fall through" statement --
there is for C++ but not yet for C), we'd stick to explicit comments.
In theory, we will be able to do a tree-wide change to add the C
statement once it exists.
> Of course, I'm happy to merge any of these patches that tweak things so
> no worries either way.
Thanks! :)
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-20 22:26 ` Kees Cook
@ 2019-02-21 21:43 ` Stephen Boyd
-1 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-21 21:43 UTC (permalink / raw)
To: Kees Cook
Cc: Krzysztof Kozlowski, Gustavo A. R. Silva, Kukjin Kim,
Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
linux-arm-kernel, linux-samsung-soc@vger.kernel.org, linux-clk,
LKML
Quoting Kees Cook (2019-02-20 14:26:06)
> On Fri, Feb 15, 2019 at 4:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Kees Cook (2019-02-12 10:57:05)
> > > On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > It is just the GCC which has to be fixed not the code. You want to
> > > > adjust the code for specific version of GCC and what if GCC changes
> > > > its warning? For example GCC might require "fall through: "... or any
> > > > other syntax. Another point - what about clang's syntax?
> > >
> > > -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> > > choice. GCC's levels were chosen based on the existing linters, static
> > > analyzers, etc. The patterns are unlikely to change (see the gcc
> > > man-page).
> > >
> > > Clang doesn't recognize anything in C mode (hopefully this will be
> > > fixed in the future[1]).
> > >
> > > As long as one of the compilers is able to check this, we'll avoid the
> > > bugs associated with this mis-pattern. Gustavo's efforts here have
> > > found kind of a lot of bugs, so I think it's worth a little churn to
> > > add these (and make minor adjustments to existing comments).
> >
> > Just curious, what compilation phase does this check run in? Could we
> > gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
> > whatever the compiler wants to see and then there would only be "one
> > way" to do this? It would alleviate the above concerns, but maybe I'm
> > rehashing something that's already been proposed and rejected.
>
> When this got discussed a while back, the thinking was that since
> we're also dealing with static analyzers (e.g. Coverity) and IDEs that
> literally parse comments in the code, it was most sensible (at least
> for now, prior to there being a formal C "fall through" statement --
> there is for C++ but not yet for C), we'd stick to explicit comments.
> In theory, we will be able to do a tree-wide change to add the C
> statement once it exists.
Ok, thanks for the background. Looks like the perf tool already
introduced the #define __fallthrough that they use for this purpose.
Maybe they're hoping that it will be formalized.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-21 21:43 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-21 21:43 UTC (permalink / raw)
To: Kees Cook
Cc: linux-samsung-soc@vger.kernel.org, Gustavo A. R. Silva,
Michael Turquette, Tomasz Figa, Krzysztof Kozlowski, LKML,
Chanwoo Choi, Kukjin Kim, Sylwester Nawrocki, linux-clk,
linux-arm-kernel
Quoting Kees Cook (2019-02-20 14:26:06)
> On Fri, Feb 15, 2019 at 4:34 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Kees Cook (2019-02-12 10:57:05)
> > > On Mon, Feb 11, 2019 at 11:41 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > It is just the GCC which has to be fixed not the code. You want to
> > > > adjust the code for specific version of GCC and what if GCC changes
> > > > its warning? For example GCC might require "fall through: "... or any
> > > > other syntax. Another point - what about clang's syntax?
> > >
> > > -Wimplicit-fallthrough=3 is stricter and maps to -Wextra, hence its
> > > choice. GCC's levels were chosen based on the existing linters, static
> > > analyzers, etc. The patterns are unlikely to change (see the gcc
> > > man-page).
> > >
> > > Clang doesn't recognize anything in C mode (hopefully this will be
> > > fixed in the future[1]).
> > >
> > > As long as one of the compilers is able to check this, we'll avoid the
> > > bugs associated with this mis-pattern. Gustavo's efforts here have
> > > found kind of a lot of bugs, so I think it's worth a little churn to
> > > add these (and make minor adjustments to existing comments).
> >
> > Just curious, what compilation phase does this check run in? Could we
> > gain a macro like FALLTHRU or even lowercase 'fallthru' that expanded to
> > whatever the compiler wants to see and then there would only be "one
> > way" to do this? It would alleviate the above concerns, but maybe I'm
> > rehashing something that's already been proposed and rejected.
>
> When this got discussed a while back, the thinking was that since
> we're also dealing with static analyzers (e.g. Coverity) and IDEs that
> literally parse comments in the code, it was most sensible (at least
> for now, prior to there being a formal C "fall through" statement --
> there is for C++ but not yet for C), we'd stick to explicit comments.
> In theory, we will be able to do a tree-wide change to add the C
> statement once it exists.
Ok, thanks for the background. Looks like the perf tool already
introduced the #define __fallthrough that they use for this purpose.
Maybe they're hoping that it will be formalized.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-21 21:43 ` Stephen Boyd
@ 2019-02-21 22:23 ` Kees Cook
-1 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2019-02-21 22:23 UTC (permalink / raw)
To: Stephen Boyd
Cc: Krzysztof Kozlowski, Gustavo A. R. Silva, Kukjin Kim,
Sylwester Nawrocki, Tomasz Figa, Chanwoo Choi, Michael Turquette,
linux-arm-kernel, linux-samsung-soc@vger.kernel.org, linux-clk,
LKML
On Thu, Feb 21, 2019 at 1:43 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Ok, thanks for the background. Looks like the perf tool already
> introduced the #define __fallthrough that they use for this purpose.
> Maybe they're hoping that it will be formalized.
Yeah, GCC has an extension for it (but it's not yet part of the C
standard -- though there are open bugs against Clang to support it).
Our adoption of the markings, thanks to Gustavo's work, has rapidly
increased lately too (we started with something like 2400 instances
and we were gaining about as many each cycle as we removed). 5.1,
though, is closing in on _0_ instances. My thinking is that once we're
to the point where we can globally enable -Wimplicit-fallthrough, then
we can depend on the compiler to enforce this (instead continuing to
depend on external tracking in Coverity and the like). It was a bit of
a chicken-and-egg and I was afraid we were going to be left with this
"partial adoption" for a long time. But we're nearly to the point
where I would be happy doing a tree-wide replacement to __fallthrough
(which should be mechanically easy).
--
Kees Cook
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-21 22:23 ` Kees Cook
0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2019-02-21 22:23 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-samsung-soc@vger.kernel.org, Gustavo A. R. Silva,
Michael Turquette, Tomasz Figa, Krzysztof Kozlowski, LKML,
Chanwoo Choi, Kukjin Kim, Sylwester Nawrocki, linux-clk,
linux-arm-kernel
On Thu, Feb 21, 2019 at 1:43 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Ok, thanks for the background. Looks like the perf tool already
> introduced the #define __fallthrough that they use for this purpose.
> Maybe they're hoping that it will be formalized.
Yeah, GCC has an extension for it (but it's not yet part of the C
standard -- though there are open bugs against Clang to support it).
Our adoption of the markings, thanks to Gustavo's work, has rapidly
increased lately too (we started with something like 2400 instances
and we were gaining about as many each cycle as we removed). 5.1,
though, is closing in on _0_ instances. My thinking is that once we're
to the point where we can globally enable -Wimplicit-fallthrough, then
we can depend on the compiler to enforce this (instead continuing to
depend on external tracking in Coverity and the like). It was a bit of
a chicken-and-egg and I was afraid we were going to be left with this
"partial adoption" for a long time. But we're nearly to the point
where I would be happy doing a tree-wide replacement to __fallthrough
(which should be mechanically easy).
--
Kees Cook
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
2019-02-11 18:15 ` Gustavo A. R. Silva
(?)
@ 2019-02-21 21:44 ` Stephen Boyd
-1 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-21 21:44 UTC (permalink / raw)
To: Gustavo A. R. Silva, Chanwoo Choi, Krzysztof Kozlowski,
Kukjin Kim, Michael Turquette, Sylwester Nawrocki, Tomasz Figa
Cc: linux-arm-kernel, linux-samsung-soc, linux-clk, linux-kernel,
Gustavo A. R. Silva, Kees Cook
Quoting Gustavo A. R. Silva (2019-02-11 10:15:31)
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warnings:
>
> drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
Applied to clk-next
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-21 21:44 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-21 21:44 UTC (permalink / raw)
To: Gustavo A. R. Silva, Chanwoo Choi, Krzysztof Kozlowski,
Kukjin Kim, Michael Turquette, Sylwester Nawrocki, Tomasz Figa
Cc: linux-samsung-soc, Kees Cook, Gustavo A. R. Silva, linux-kernel,
linux-clk, linux-arm-kernel
Quoting Gustavo A. R. Silva (2019-02-11 10:15:31)
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warnings:
>
> drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
Applied to clk-next
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] clk: samsung: s3c2443: Mark expected switch fall-through
@ 2019-02-21 21:44 ` Stephen Boyd
0 siblings, 0 replies; 20+ messages in thread
From: Stephen Boyd @ 2019-02-21 21:44 UTC (permalink / raw)
To: Chanwoo Choi, Krzysztof Kozlowski, Kukjin Kim, Michael Turquette,
Sylwester Nawrocki, Tomasz Figa
Cc: linux-arm-kernel, linux-samsung-soc, linux-clk, linux-kernel,
Gustavo A. R. Silva, Kees Cook
Quoting Gustavo A. R. Silva (2019-02-11 10:15:31)
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warnings:
>
> drivers/clk/samsung/clk-s3c2443.c: In function ‘s3c2443_common_clk_init’:
> drivers/clk/samsung/clk-s3c2443.c:390:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> samsung_clk_register_alias(ctx, s3c2450_aliases,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ARRAY_SIZE(s3c2450_aliases));
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/clk/samsung/clk-s3c2443.c:393:2: note: here
> case S3C2416:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
Applied to clk-next
^ permalink raw reply [flat|nested] 20+ messages in thread