Skip to contentSkip to conversation

drm/i915: Mark expected switch fall-throughs

PATCH In review Gustavo A. R. Silva 3 participants 4 messages Nov 27, 2017 – Dec 4, 2017

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

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

Addresses-Coverity-ID: 141432
Addresses-Coverity-ID: 141433
Addresses-Coverity-ID: 141434
Addresses-Coverity-ID: 141435
Addresses-Coverity-ID: 141436
Addresses-Coverity-ID: 1357360
Addresses-Coverity-ID: 1357403
Addresses-Coverity-ID: 1357433
Addresses-Coverity-ID: 1392622
Addresses-Coverity-ID: 1415273
Addresses-Coverity-ID: 1435752
Addresses-Coverity-ID: 1441500
Addresses-Coverity-ID: 1454596

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem.c         | 3 ++-
 drivers/gpu/drm/i915/intel_cdclk.c      | 4 ++++
 drivers/gpu/drm/i915/intel_display.c    | 2 ++
 drivers/gpu/drm/i915/intel_drv.h        | 1 +
 drivers/gpu/drm/i915/intel_engine_cs.c  | 2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
 drivers/gpu/drm/i915/intel_sdvo.c       | 6 ++++++
 7 files changed, 18 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_gem.c +2 -1
index 3a140ee..326cf16 100644
@@ -1977,6 +1977,7 @@ int i915_gem_fault(struct vm_fault *vmf)
 			ret = VM_FAULT_SIGBUS;
 			break;
 		}
+		/* fall through */
 	case -EAGAIN:
 		/*
 		 * EAGAIN means the gpu is hung and we'll wait for the error

@@ -2644,7 +2645,7 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj,
 	switch (type) {
 	default:
 		MISSING_CASE(type);
-		/* fallthrough to use PAGE_KERNEL anyway */
+		/* fall through - To use PAGE_KERNEL anyway */
 	case I915_MAP_WB:
 		pgprot = PAGE_KERNEL;
 		break;
drivers/gpu/drm/i915/intel_cdclk.c +4 -0
index b2a6d62..5879cd3 100644
@@ -316,6 +316,7 @@ static void pnv_get_cdclk(struct drm_i915_private *dev_priv,
 		break;
 	default:
 		DRM_ERROR("Unknown pnv display core clock 0x%04x\n", gcfgc);
+		/* fall through */
 	case GC_DISPLAY_CLOCK_133_MHZ_PNV:
 		cdclk_state->cdclk = 133333;
 		break;

@@ -1110,6 +1111,7 @@ static int bxt_de_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
 	switch (cdclk) {
 	default:
 		MISSING_CASE(cdclk);
+		/* fall through */
 	case 144000:
 	case 288000:
 	case 384000:

@@ -1134,6 +1136,7 @@ static int glk_de_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
 	switch (cdclk) {
 	default:
 		MISSING_CASE(cdclk);
+		/* fall through */
 	case  79200:
 	case 158400:
 	case 316800:

@@ -1592,6 +1595,7 @@ static int cnl_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
 	switch (cdclk) {
 	default:
 		MISSING_CASE(cdclk);
+		/* fall through */
 	case 168000:
 	case 336000:
 		ratio = dev_priv->cdclk.hw.ref == 19200 ? 35 : 28;
drivers/gpu/drm/i915/intel_display.c +2 -0
index 878acc4..1f7907f 100644
@@ -9049,6 +9049,7 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
 		switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
 		default:
 			WARN(1, "unknown pipe linked to edp transcoder\n");
+			/* fall through */
 		case TRANS_DDI_EDP_INPUT_A_ONOFF:
 		case TRANS_DDI_EDP_INPUT_A_ON:
 			trans_edp_pipe = PIPE_A;

@@ -10751,6 +10752,7 @@ static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 		case INTEL_OUTPUT_UNKNOWN:
 			if (WARN_ON(!HAS_DDI(to_i915(dev))))
 				break;
+			/* fall through */
 		case INTEL_OUTPUT_DP:
 		case INTEL_OUTPUT_HDMI:
 		case INTEL_OUTPUT_EDP:
drivers/gpu/drm/i915/intel_drv.h +1 -0
index 7bc60c8..4c3696c 100644
@@ -1149,6 +1149,7 @@ enc_to_dig_port(struct drm_encoder *encoder)
 	switch (intel_encoder->type) {
 	case INTEL_OUTPUT_UNKNOWN:
 		WARN_ON(!HAS_DDI(to_i915(encoder->dev)));
+		/* fall through */
 	case INTEL_OUTPUT_DP:
 	case INTEL_OUTPUT_EDP:
 	case INTEL_OUTPUT_HDMI:
drivers/gpu/drm/i915/intel_engine_cs.c +2 -0
index ab5bf4e..d02f37d 100644
@@ -153,6 +153,7 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
 		switch (INTEL_GEN(dev_priv)) {
 		default:
 			MISSING_CASE(INTEL_GEN(dev_priv));
+			/* fall through */
 		case 10:
 			return GEN10_LR_CONTEXT_RENDER_SIZE;
 		case 9:

@@ -183,6 +184,7 @@ __intel_engine_context_size(struct drm_i915_private *dev_priv, u8 class)
 		break;
 	default:
 		MISSING_CASE(class);
+		/* fall through */
 	case VIDEO_DECODE_CLASS:
 	case VIDEO_ENHANCEMENT_CLASS:
 	case COPY_ENGINE_CLASS:
drivers/gpu/drm/i915/intel_runtime_pm.c +1 -0
index 8af286c..c60dc67 100644
@@ -2740,6 +2740,7 @@ static void cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
 	switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
 	default:
 		MISSING_CASE(val);
+		/* fall through */
 	case VOLTAGE_INFO_0_85V | PROCESS_INFO_DOT_0:
 		procmon = &cnl_procmon_values[PROCMON_0_85V_DOT_0];
 		break;
drivers/gpu/drm/i915/intel_sdvo.c +6 -0
index 7437944..3860438 100644
@@ -1327,6 +1327,7 @@ static void intel_sdvo_pre_enable(struct intel_encoder *intel_encoder,
 	switch (crtc_state->pixel_multiplier) {
 	default:
 		WARN(1, "unknown pixel multiplier specified\n");
+		/* fall through */
 	case 1: rate = SDVO_CLOCK_RATE_MULT_1X; break;
 	case 2: rate = SDVO_CLOCK_RATE_MULT_2X; break;
 	case 4: rate = SDVO_CLOCK_RATE_MULT_4X; break;

@@ -2275,14 +2276,19 @@ intel_sdvo_guess_ddc_bus(struct intel_sdvo *sdvo)
 	switch (sdvo->controlled_output) {
 	case SDVO_OUTPUT_LVDS1:
 		mask |= SDVO_OUTPUT_LVDS1;
+		/* fall through */
 	case SDVO_OUTPUT_LVDS0:
 		mask |= SDVO_OUTPUT_LVDS0;
+		/* fall through */
 	case SDVO_OUTPUT_TMDS1:
 		mask |= SDVO_OUTPUT_TMDS1;
+		/* fall through */
 	case SDVO_OUTPUT_TMDS0:
 		mask |= SDVO_OUTPUT_TMDS0;
+		/* fall through */
 	case SDVO_OUTPUT_RGB1:
 		mask |= SDVO_OUTPUT_RGB1;
+		/* fall through */
 	case SDVO_OUTPUT_RGB0:
 		mask |= SDVO_OUTPUT_RGB0;
 		break;
Signature · 1 line
2.7.4
maintainer
#1 lore raw
Quoted 2 lines · Gustavo A. R. Silva

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

I have to say I'm totally not sold on regexps matching comment contents. Was something more explicit ever considered? Like:

#define FALLTHROUGH __attribute__((fallthrough));

With the appropriate version checks, of course.

Regards, Joonas

Signature · 7 lines
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw

Hi Joonas,

Quoting Joonas Lahtinen <[email protected]>:

Quoted 11 lines · Gustavo A. R. Silva
Quoted 2 lines

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

I have to say I'm totally not sold on regexps matching comment contents. Was something more explicit ever considered? Like:

#define FALLTHROUGH __attribute__((fallthrough));

With the appropriate version checks, of course.

One of the arguments is that comments lets us leverage the existing static analyzers.

We've been discussing this during the last week, feel free to join the discussion:

http://www.spinics.net/lists/kernel/msg2659908.html
http://www.spinics.net/lists/kernel/msg2659906.html

Thanks!
--
Gustavo A. R. Silva

#3 lore raw
Quoted 18 lines · Gustavo A. R. Silva

Hi Joonas,

Quoting Joonas Lahtinen <[email protected]>:

Quoted 6 lines · Gustavo A. R. Silva
Quoted 3 lines

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

Quoted 7 lines

I have to say I'm totally not sold on regexps matching comment contents. Was something more explicit ever considered? Like:

#define FALLTHROUGH __attribute__((fallthrough));

With the appropriate version checks, of course.

Quoted 8 lines

One of the arguments is that comments lets us leverage the existing static analyzers.

We've been discussing this during the last week, feel free to join the discussion:

http://www.spinics.net/lists/kernel/msg2659908.html
http://www.spinics.net/lists/kernel/msg2659906.html

If we go with existing rules, then either pls patch coding style, or be a bit more liberal in what you accept. E.g. fallthrough vs fall through seems a bit a bikeshed (and will be an endless source of work for you).

I'd also claim that "this shouldn't happen, dump a backtrace and hope for the best" style macros like i915's MISSING_CASE or WARN_ON (as the only thing) should count as an auto-fallthrough annotation.

From a quick look, that would cover everything in your patch. -Daniel

Quoted 14 lines

Thanks!
--
Gustavo A. R. Silva

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

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx