[PATCH] drm/i915: mark expected switch fall-through

[PATCH] drm/i915: mark expected switch fall-through

[Gustavo A. R. Silva]

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

Addresses-Coverity-ID: 1470102 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 132fe63..6a40a77 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2566,6 +2566,7 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
 	switch (index) {
 	default:
 		MISSING_CASE(index);
+		/* fall through */
 	case 0:
 		link_clock = 540000;
 		break;
-- 
2.7.4

Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Rodrigo Vivi]

On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Addresses-Coverity-ID: 1470102 ("Missing break in switch")

Any other advantage besides coverity?
Can't we address it by marking as "Intentional" on the tool?

I'm afraid there will be so many more places to add fallthrough
marks....

> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 132fe63..6a40a77 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2566,6 +2566,7 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>  	switch (index) {
>  	default:
>  		MISSING_CASE(index);
> +		/* fall through */
>  	case 0:
>  		link_clock = 540000;
>  		break;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Gustavo A. R. Silva]

On 06/20/2018 02:06 PM, Rodrigo Vivi wrote:
> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> Addresses-Coverity-ID: 1470102 ("Missing break in switch")
> 
> Any other advantage besides coverity?
> Can't we address it by marking as "Intentional" on the tool?
> 

Yes. The advantage of this is that it will eventually allows to enable 
-Wimplicit-fallthrough, hence, enabling the compiler to trigger a 
warning, which will force us to double check if we are actually missing 
a break before committing the code.

The change in the code has nothing to do with the Coverity tool. The 
tool is only reporting the issue, which, in this case, is a false positive.


> I'm afraid there will be so many more places to add fallthrough
> marks....
> 

Oh yeah, there are around 1000 similar places in the whole codebase. 
There is an ongoing effort to review each case. Months ago, it used to 
be around 1500 of these cases.

Thanks
--
Gustavo

>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> index 132fe63..6a40a77 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> @@ -2566,6 +2566,7 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>>   	switch (index) {
>>   	default:
>>   		MISSING_CASE(index);
>> +		/* fall through */
>>   	case 0:
>>   		link_clock = 540000;
>>   		break;
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Jani Nikula]

On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> On 06/20/2018 02:06 PM, Rodrigo Vivi wrote:
>> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>> where we are expecting to fall through.
>>>
>>> Addresses-Coverity-ID: 1470102 ("Missing break in switch")
>> 
>> Any other advantage besides coverity?
>> Can't we address it by marking as "Intentional" on the tool?
>> 
>
> Yes. The advantage of this is that it will eventually allows to enable 
> -Wimplicit-fallthrough, hence, enabling the compiler to trigger a 
> warning, which will force us to double check if we are actually missing 
> a break before committing the code.

I applaud the efforts. Since you're doing the comment changes, do you
have an idea what -Wimplicit-fallthrough=N level is being considered for
kernel?

>> I'm afraid there will be so many more places to add fallthrough
>> marks....
>> 
>
> Oh yeah, there are around 1000 similar places in the whole codebase. 
> There is an ongoing effort to review each case. Months ago, it used to 
> be around 1500 of these cases.

We use our own MISSING_CASE() to indicate stuff that's not supposed to
happen, or to be implemented, etc. and in many cases the fallthrough is
normal. I wonder if we could embed __attribute__ ((fallthrough)) in
there to tackle all of these without a comment.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Gustavo A. R. Silva]

Hi Jani,

On 06/21/2018 03:03 AM, Jani Nikula wrote:
> On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>> On 06/20/2018 02:06 PM, Rodrigo Vivi wrote:
>>> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>> where we are expecting to fall through.
>>>>
>>>> Addresses-Coverity-ID: 1470102 ("Missing break in switch")
>>>
>>> Any other advantage besides coverity?
>>> Can't we address it by marking as "Intentional" on the tool?
>>>
>>
>> Yes. The advantage of this is that it will eventually allows to enable 
>> -Wimplicit-fallthrough, hence, enabling the compiler to trigger a 
>> warning, which will force us to double check if we are actually missing 
>> a break before committing the code.
> 
> I applaud the efforts. Since you're doing the comment changes, do you
> have an idea what -Wimplicit-fallthrough=N level is being considered for
> kernel?
> 

Currently, we are trying level 2.

>>> I'm afraid there will be so many more places to add fallthrough
>>> marks....
>>>
>>
>> Oh yeah, there are around 1000 similar places in the whole codebase. 
>> There is an ongoing effort to review each case. Months ago, it used to 
>> be around 1500 of these cases.
> 
> We use our own MISSING_CASE() to indicate stuff that's not supposed to
> happen, or to be implemented, etc. and in many cases the fallthrough is
> normal. I wonder if we could embed __attribute__ ((fallthrough)) in
> there to tackle all of these without a comment.
> 

I've tried this:

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index 00165ad..829572c 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -40,8 +40,10 @@
 #undef WARN_ON_ONCE
 #define WARN_ON_ONCE(x) WARN_ONCE((x), "%s", "WARN_ON_ONCE(" __stringify(x) ")")

-#define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
-                            __stringify(x), (long)(x))
+#define MISSING_CASE(x) ({ \
+       WARN(1, "Missing case (%s == %ld)\n", __stringify(x), (long)(x)); \
+       __attribute__ ((fallthrough)); \
+})

 #if GCC_VERSION >= 70000
 #define add_overflows(A, B) \

and I get the following warnings as a consequence:

drivers/gpu/drm/i915/intel_pm.c: In function ‘intel_init_clock_gating_hooks’:
drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute ‘fallthrough’
  __attribute__ ((fallthrough)); \
  ^
drivers/gpu/drm/i915/intel_pm.c:9240:3: note: in expansion of macro ‘MISSING_CASE’
   MISSING_CASE(INTEL_DEVID(dev_priv));
   ^~~~~~~~~~~~
drivers/gpu/drm/i915/intel_pm.c: In function ‘intel_read_wm_latency’:
drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute ‘fallthrough’
  __attribute__ ((fallthrough)); \
  ^
drivers/gpu/drm/i915/intel_pm.c:2902:3: note: in expansion of macro ‘MISSING_CASE’
   MISSING_CASE(INTEL_DEVID(dev_priv));
   ^~~~~~~~~~~~

Thanks
--
Gustavo

Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Jani Nikula]

On Tue, 26 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> Hi Jani,
>
> On 06/21/2018 03:03 AM, Jani Nikula wrote:
>> On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>>> On 06/20/2018 02:06 PM, Rodrigo Vivi wrote:
>>>> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote:
>>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>>>> where we are expecting to fall through.
>>>>>
>>>>> Addresses-Coverity-ID: 1470102 ("Missing break in switch")
>>>>
>>>> Any other advantage besides coverity?
>>>> Can't we address it by marking as "Intentional" on the tool?
>>>>
>>>
>>> Yes. The advantage of this is that it will eventually allows to enable 
>>> -Wimplicit-fallthrough, hence, enabling the compiler to trigger a 
>>> warning, which will force us to double check if we are actually missing 
>>> a break before committing the code.
>> 
>> I applaud the efforts. Since you're doing the comment changes, do you
>> have an idea what -Wimplicit-fallthrough=N level is being considered for
>> kernel?
>> 
>
> Currently, we are trying level 2.
>
>>>> I'm afraid there will be so many more places to add fallthrough
>>>> marks....
>>>>
>>>
>>> Oh yeah, there are around 1000 similar places in the whole codebase. 
>>> There is an ongoing effort to review each case. Months ago, it used to 
>>> be around 1500 of these cases.
>> 
>> We use our own MISSING_CASE() to indicate stuff that's not supposed to
>> happen, or to be implemented, etc. and in many cases the fallthrough is
>> normal. I wonder if we could embed __attribute__ ((fallthrough)) in
>> there to tackle all of these without a comment.
>> 
>
> I've tried this:
>
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 00165ad..829572c 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -40,8 +40,10 @@
>  #undef WARN_ON_ONCE
>  #define WARN_ON_ONCE(x) WARN_ONCE((x), "%s", "WARN_ON_ONCE(" __stringify(x) ")")
>
> -#define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> -                            __stringify(x), (long)(x))
> +#define MISSING_CASE(x) ({ \
> +       WARN(1, "Missing case (%s == %ld)\n", __stringify(x), (long)(x)); \
> +       __attribute__ ((fallthrough)); \
> +})
>
>  #if GCC_VERSION >= 70000
>  #define add_overflows(A, B) \
>
> and I get the following warnings as a consequence:

Right. That's because we've used MISSING_CASE() also in if-ladders in
addition to the switch default case. From our POV the usage is similar.

*shrug*

I guess I like /* fall through */ annotations next to MISSING_CASE()
better than having two different macros depending on where they're being
used.

Thanks for trying it out anyway.

BR,
Jani.


>
> drivers/gpu/drm/i915/intel_pm.c: In function ‘intel_init_clock_gating_hooks’:
> drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute ‘fallthrough’
>   __attribute__ ((fallthrough)); \
>   ^
> drivers/gpu/drm/i915/intel_pm.c:9240:3: note: in expansion of macro ‘MISSING_CASE’
>    MISSING_CASE(INTEL_DEVID(dev_priv));
>    ^~~~~~~~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function ‘intel_read_wm_latency’:
> drivers/gpu/drm/i915/i915_utils.h:48:2: error: invalid use of attribute ‘fallthrough’
>   __attribute__ ((fallthrough)); \
>   ^
> drivers/gpu/drm/i915/intel_pm.c:2902:3: note: in expansion of macro ‘MISSING_CASE’
>    MISSING_CASE(INTEL_DEVID(dev_priv));
>    ^~~~~~~~~~~~
>
> Thanks
> --
> Gustavo

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH] drm/i915: mark expected switch fall-through

[Gustavo A. R. Silva]

> 
> Right. That's because we've used MISSING_CASE() also in if-ladders in
> addition to the switch default case. From our POV the usage is similar.
> 

Yep.

> *shrug*
> 
> I guess I like /* fall through */ annotations next to MISSING_CASE()
> better than having two different macros depending on where they're being
> used.
> 

OK. I'll send a patch for the whole i915 subsystem, shortly.

Thanks for the feedback.
--
Gustavo