Skip to contentSkip to conversation

drm/i915: mark expected switch fall-through

PATCH In review Gustavo A. R. Silva 4 participants 10 messages Jun 20, 2018 – Jun 28, 2018

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 drm/i915: mark expected switch fall-through

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 <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
index 132fe63..6a40a77 100644
@@ -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;
Signature · 1 line
2.7.4
CI
#1 lore raw

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

== Series Details ==

== 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

maintainer
#2 lore raw
In reply to Gustavo A. R. Silva
Quoted 4 lines · 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")

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

Quoted 24 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
index 132fe63..6a40a77 100644
@@ -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;
Signature · 6 lines
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#3 lore raw
Quoted 9 lines · Rodrigo Vivi
Quoted 4 lines · 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")

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.

Quoted 3 lines

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

Quoted 24 lines
Quoted 24 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 +
  1 file changed, 1 insertion(+)
drivers/gpu/drm/i915/intel_dpll_mgr.c +1 -0
index 132fe63..6a40a77 100644
@@ -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;
Signature · 6 lines
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
maintainer
#4 lore raw
Quoted 7 lines · Gustavo A. R. Silva
Quoted 6 lines · Rodrigo Vivi
Quoted 4 lines · 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")

Quoted 8 lines
Quoted 3 lines

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?

Quoted 7 lines
Quoted 3 lines

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.

Signature · 5 lines
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#5 lore raw

Hi Jani,

Quoted 12 lines · Jani Nikula
Quoted 11 lines · Gustavo A. R. Silva
Quoted 9 lines · Rodrigo Vivi
Quoted 4 lines · 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")

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

Quoted 9 lines
Quoted 4 lines

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.

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 3 lines

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:

drivers/gpu/drm/i915/i915_utils.h +4 -2
index 00165ad..829572c 100644
@@ -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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

maintainer
#6 lore raw
Quoted 61 lines · Gustavo A. R. Silva

Hi Jani,

Quoted 12 lines · Jani Nikula
Quoted 11 lines · Gustavo A. R. Silva
Quoted 9 lines · Rodrigo Vivi
Quoted 4 lines · 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")

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

Quoted 9 lines
Quoted 4 lines

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.

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

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

Quoted 9 lines
Quoted 3 lines

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:

drivers/gpu/drm/i915/i915_utils.h +4 -2
index 00165ad..829572c 100644
@@ -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) \
Quoted 1 line

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.

Quoted 19 lines

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

Signature · 5 lines
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#7 lore raw
Quoted 5 lines

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.

Quoted 6 lines

*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

CI
#8 lore raw

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

In reply to Gustavo A. R. Silva
== Series Details ==

== 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#9 lore raw

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

In reply to Gustavo A. R. Silva
== Series Details ==

== 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx