Re: [Intel-gfx] [PATCH] drm/i915: Mark expected switch fall-throughs

2017-12-04 Thread Daniel Vetter 
On Mon, Dec 4, 2017 at 4:20 PM, Gustavo A. R. Silva
 wrote:
> Hi Joonas,
>
> Quoting Joonas Lahtinen :
>
>> 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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Mark expected switch fall-throughs

2017-12-04 Thread Gustavo A. R. Silva 

Hi Joonas,

Quoting Joonas Lahtinen :


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

Re: [PATCH] drm/i915: Mark expected switch fall-throughs

2017-12-04 Thread Joonas Lahtinen 
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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

[PATCH] drm/i915: Mark expected switch fall-throughs

2017-11-27 Thread Gustavo A. R. Silva 
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 
---
 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(-)

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 = 13;
break;
@@ -1110,6 +,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
@@