[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

✓ Fi.CI.BAT: success for drm/i915: mark expected switch fall-through

[Patchwork]

== Series Details ==

Series: drm/i915: mark expected switch fall-through
URL   : https://patchwork.freedesktop.org/series/45088/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4348 -> Patchwork_9371 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45088/revisions/1/mbox/


== Changes ==

  No changes found


== Participating hosts (44 -> 37) ==

  Additional (1): fi-hsw-peppy 
  Missing    (8): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-glk-dsi fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-x1275 


== Build changes ==

    * Linux: CI_DRM_4348 -> Patchwork_9371

  CI_DRM_4348: 3a2fbf8fe32d909c5d44e61e7d212ae694e9e473 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4526: 4bbfb4fb14b3deab9bc4db9911280b35c22b718c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9371: 4934357efe0e8f39afbeaba6dd3e25ee2cd790e8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4934357efe0e drm/i915: mark expected switch fall-through

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9371/issues.html
_______________________________________________
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

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
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]

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

✓ Fi.CI.IGT: success for drm/i915: mark expected switch fall-through

[Patchwork]

== Series Details ==

Series: drm/i915: mark expected switch fall-through
URL   : https://patchwork.freedesktop.org/series/45088/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4348_full -> Patchwork_9371_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9371_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9371_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9371_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          SKIP -> PASS +7

    
== Known issues ==

  Here are the changes found in Patchwork_9371_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_evict:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@gem_exec_schedule@pi-ringfull-vebox:
      shard-kbl:          NOTRUN -> FAIL (fdo#103158)

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-glk:          PASS -> FAIL (fdo#103060)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-apl:          DMESG-FAIL (fdo#106947, fdo#106560) -> PASS

    igt@gem_exec_await@wide-contexts:
      shard-glk:          FAIL (fdo#105900) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-panning-vs-hang:
      shard-snb:          DMESG-WARN (fdo#103821) -> PASS

    igt@kms_flip@plain-flip-ts-check:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip_tiling@flip-to-x-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-wc:
      shard-kbl:          FAIL (fdo#106067) -> PASS

    igt@perf@enable-disable:
      shard-kbl:          DMESG-FAIL (fdo#106064) -> PASS

    igt@pm_rpm@system-suspend-execbuf:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@pm_rps@waitboost:
      shard-kbl:          FAIL (fdo#102250) -> PASS

    
    ==== Warnings ====

    igt@drv_selftest@live_gtt:
      shard-glk:          FAIL (fdo#105347) -> INCOMPLETE (fdo#103359, k.org#198133)

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102250 https://bugs.freedesktop.org/show_bug.cgi?id=102250
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106064 https://bugs.freedesktop.org/show_bug.cgi?id=106064
  fdo#106067 https://bugs.freedesktop.org/show_bug.cgi?id=106067
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4348 -> Patchwork_9371

  CI_DRM_4348: 3a2fbf8fe32d909c5d44e61e7d212ae694e9e473 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4526: 4bbfb4fb14b3deab9bc4db9911280b35c22b718c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9371: 4934357efe0e8f39afbeaba6dd3e25ee2cd790e8 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9371/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: mark expected switch fall-through (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: mark expected switch fall-through (rev2)
URL   : https://patchwork.freedesktop.org/series/45088/
State : failure

== Summary ==

Applying: drm/i915: mark expected switch fall-through
error: patch failed: drivers/gpu/drm/i915/i915_utils.h:40
error: drivers/gpu/drm/i915/i915_utils.h: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Using index info to reconstruct a base tree...
Patch failed at 0001 drm/i915: mark expected switch fall-through
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx