| Message ID | 20171127221701.GA15983@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-11-27 at 16:17 -0600, Gustavo A. R. Silva wrote: > 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
Hi Joonas, Quoting Joonas Lahtinen <joonas.lahtinen@linux.intel.com>: > On Mon, 2017-11-27 at 16:17 -0600, Gustavo A. R. Silva wrote: >> 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
On Mon, Dec 4, 2017 at 4:20 PM, Gustavo A. R. Silva <garsilva@embeddedor.com> wrote: > Hi Joonas, > > Quoting Joonas Lahtinen <joonas.lahtinen@linux.intel.com>: > >> On Mon, 2017-11-27 at 16:17 -0600, Gustavo A. R. Silva wrote: >>> >>> 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 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 > > Thanks! > -- > Gustavo A. R. Silva > > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3a140ee..326cf16 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -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; diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index b2a6d62..5879cd3 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -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; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 878acc4..1f7907f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -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: diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7bc60c8..4c3696c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -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: diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index ab5bf4e..d02f37d 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -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: diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 8af286c..c60dc67 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -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; diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 7437944..3860438 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -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;
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 <garsilva@embeddedor.com> --- 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(-)