intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization
@ 2020-02-20  6:22 Kees Cook
  2020-02-20  7:02 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Kees Cook @ 2020-02-20  6:22 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: intel-gfx, Alexander Potapenko, Kees Cook, linux-kernel

Variables declared in a switch statement before any case statements
cannot be automatically initialized with compiler instrumentation (as
they are not part of any execution flow). With GCC's proposed automatic
stack variable initialization feature, this triggers a warning (and they
don't get initialized). Clang's automatic stack variable initialization
(via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
doesn't initialize such variables[1]. Note that these warnings (or silent
skipping) happen before the dead-store elimination optimization phase,
so even when the automatic initializations are later elided in favor of
direct initializations, the warnings remain.

To avoid these problems, move such variables into the "case" where
they're used or lift them up into the main function body.

drivers/gpu/drm/i915/display/intel_display.c: In function ‘check_digital_port_conflicts’:
drivers/gpu/drm/i915/display/intel_display.c:12963:17: warning: statement will never be executed [-Wswitch-unreachable]
12963 |    unsigned int port_mask;
      |                 ^~~~~~~~~

drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_get_fifo_size’:
drivers/gpu/drm/i915/intel_pm.c:474:7: warning: statement will never be executed [-Wswitch-unreachable]
  474 |   u32 dsparb, dsparb2, dsparb3;
      |       ^~~~~~
drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_atomic_update_fifo’:
drivers/gpu/drm/i915/intel_pm.c:1997:7: warning: statement will never be executed [-Wswitch-unreachable]
 1997 |   u32 dsparb, dsparb2, dsparb3;
      |       ^~~~~~

[1] https://bugs.llvm.org/show_bug.cgi?id=44916

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/gpu/drm/i915/display/intel_display.c |    6 ++++--
 drivers/gpu/drm/i915/intel_pm.c              |    4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 064dd99bbc49..c829cd26f99e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12960,14 +12960,15 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
 		WARN_ON(!connector_state->crtc);
 
 		switch (encoder->type) {
-			unsigned int port_mask;
 		case INTEL_OUTPUT_DDI:
 			if (WARN_ON(!HAS_DDI(to_i915(dev))))
 				break;
 			/* else, fall through */
 		case INTEL_OUTPUT_DP:
 		case INTEL_OUTPUT_HDMI:
-		case INTEL_OUTPUT_EDP:
+		case INTEL_OUTPUT_EDP: {
+			unsigned int port_mask;
+
 			port_mask = 1 << encoder->port;
 
 			/* the same port mustn't appear more than once */
@@ -12976,6 +12977,7 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
 
 			used_ports |= port_mask;
 			break;
+		}
 		case INTEL_OUTPUT_DP_MST:
 			used_mst_ports |=
 				1 << encoder->port;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bd2d30ecc030..17d8833787c4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -469,9 +469,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
 	struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
 	enum pipe pipe = crtc->pipe;
 	int sprite0_start, sprite1_start;
+	u32 dsparb, dsparb2, dsparb3;
 
 	switch (pipe) {
-		u32 dsparb, dsparb2, dsparb3;
 	case PIPE_A:
 		dsparb = I915_READ(DSPARB);
 		dsparb2 = I915_READ(DSPARB2);
@@ -1969,6 +1969,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 	const struct vlv_fifo_state *fifo_state =
 		&crtc_state->wm.vlv.fifo_state;
 	int sprite0_start, sprite1_start, fifo_size;
+	u32 dsparb, dsparb2, dsparb3;
 
 	if (!crtc_state->fifo_changed)
 		return;
@@ -1994,7 +1995,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 	spin_lock(&uncore->lock);
 
 	switch (crtc->pipe) {
-		u32 dsparb, dsparb2, dsparb3;
 	case PIPE_A:
 		dsparb = intel_uncore_read_fw(uncore, DSPARB);
 		dsparb2 = intel_uncore_read_fw(uncore, DSPARB2);

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Distribute switch variables for initialization
  2020-02-20  6:22 [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization Kees Cook
@ 2020-02-20  7:02 ` Patchwork
  2020-02-20 10:21 ` [Intel-gfx] [PATCH] " Jani Nikula
  2020-02-20 13:47 ` Ville Syrjälä
  2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2020-02-20  7:02 UTC (permalink / raw)
  To: Kees Cook; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Distribute switch variables for initialization
URL   : https://patchwork.freedesktop.org/series/73690/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7970 -> Patchwork_16641
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/index.html

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-j1900:       [PASS][1] -> [INCOMPLETE][2] ([i915#45])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-byt-j1900/igt@gem_close_race@basic-threads.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-byt-j1900/igt@gem_close_race@basic-threads.html
    - fi-byt-n2820:       [PASS][3] -> [INCOMPLETE][4] ([i915#45])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-byt-n2820/igt@gem_close_race@basic-threads.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-byt-n2820/igt@gem_close_race@basic-threads.html

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-guc:         [PASS][5] -> [INCOMPLETE][6] ([fdo#108744])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-skl-guc/igt@i915_selftest@live_hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-skl-guc/igt@i915_selftest@live_hangcheck.html

  * igt@i915_selftest@live_sanitycheck:
    - fi-icl-u3:          [PASS][7] -> [DMESG-WARN][8] ([i915#585])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-icl-u3/igt@i915_selftest@live_sanitycheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-icl-u3/igt@i915_selftest@live_sanitycheck.html

  * igt@kms_chamelium@dp-edid-read:
    - fi-cml-u2:          [PASS][9] -> [FAIL][10] ([i915#217] / [i915#976])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-cml-u2/igt@kms_chamelium@dp-edid-read.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-cml-u2/igt@kms_chamelium@dp-edid-read.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-threads:
    - fi-hsw-peppy:       [TIMEOUT][11] ([fdo#112271] / [i915#1084]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-hsw-peppy/igt@gem_close_race@basic-threads.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-hsw-peppy/igt@gem_close_race@basic-threads.html

  * igt@i915_selftest@live_execlists:
    - {fi-tgl-dsi}:       [INCOMPLETE][13] ([i915#529] / [i915#647]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-tgl-dsi/igt@i915_selftest@live_execlists.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-tgl-dsi/igt@i915_selftest@live_execlists.html

  * igt@i915_selftest@live_gtt:
    - fi-kbl-7500u:       [TIMEOUT][15] ([fdo#112271]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-kbl-7500u/igt@i915_selftest@live_gtt.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-kbl-7500u/igt@i915_selftest@live_gtt.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-byt-n2820:       [FAIL][17] ([i915#999]) -> [FAIL][18] ([i915#816])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7970/fi-byt-n2820/igt@runner@aborted.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16641/fi-byt-n2820/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#1084]: https://gitlab.freedesktop.org/drm/intel/issues/1084
  [i915#217]: https://gitlab.freedesktop.org/drm/intel/issues/217
  [i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45
  [i915#529]: https://gitlab.freedesktop.org/drm/intel/issues/529
  [i915#585]: https://gitlab.freedesktop.org/drm/intel/issues/585
  [i915#647]: https://gitlab.freedesktop.org/drm/intel/issues/647
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816
  [i915#976]: https://gitlab.freedesktop.org/drm/intel/issues/976
  [i915#999]: https://gitlab.freedesktop.org/drm/intel/issues/999


Participating hosts (48 -> 45)
------------------------------

  Additional (4): fi-gdg-551 fi-bsw-nick fi-skl-6600u fi-snb-2600 
  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-kbl-7560u fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7970 -> Patchwork_16641

  CI-20190529: 20190529
  CI_DRM_7970: 6b8b833350142345f4b1a6af9486db7d316a7ff1 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5452: c05dc6cd816feb1cc518ce777ab3fd6c81893113 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16641: a76a3fd3503c76419c76ac7df15e92298051e1bf @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a76a3fd3503c drm/i915: Distribute switch variables for initialization

== Logs ==

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization
  2020-02-20  6:22 [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization Kees Cook
  2020-02-20  7:02 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
@ 2020-02-20 10:21 ` Jani Nikula
  2020-02-20 23:42   ` Kees Cook
  2020-02-20 13:47 ` Ville Syrjälä
  2 siblings, 1 reply; 5+ messages in thread
From: Jani Nikula @ 2020-02-20 10:21 UTC (permalink / raw)
  To: Kees Cook, Joonas Lahtinen, Rodrigo Vivi
  Cc: intel-gfx, Alexander Potapenko, Kees Cook, linux-kernel

On Wed, 19 Feb 2020, Kees Cook <keescook@chromium.org> wrote:
> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1]. Note that these warnings (or silent
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
>
> To avoid these problems, move such variables into the "case" where
> they're used or lift them up into the main function body.
>
> drivers/gpu/drm/i915/display/intel_display.c: In function ‘check_digital_port_conflicts’:
> drivers/gpu/drm/i915/display/intel_display.c:12963:17: warning: statement will never be executed [-Wswitch-unreachable]
> 12963 |    unsigned int port_mask;
>       |                 ^~~~~~~~~
>
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_get_fifo_size’:
> drivers/gpu/drm/i915/intel_pm.c:474:7: warning: statement will never be executed [-Wswitch-unreachable]
>   474 |   u32 dsparb, dsparb2, dsparb3;
>       |       ^~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_atomic_update_fifo’:
> drivers/gpu/drm/i915/intel_pm.c:1997:7: warning: statement will never be executed [-Wswitch-unreachable]
>  1997 |   u32 dsparb, dsparb2, dsparb3;
>       |       ^~~~~~
>
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

If you look at i915/Makefile, you'll see that we don't shy away from
enabling lots of extra warnings, and we run our CI with -Werror to keep
it clean. It does not seem like -Wswitch-unreachable does me any good,
though... is it new?

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/display/intel_display.c |    6 ++++--
>  drivers/gpu/drm/i915/intel_pm.c              |    4 ++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 064dd99bbc49..c829cd26f99e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12960,14 +12960,15 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  		WARN_ON(!connector_state->crtc);
>  
>  		switch (encoder->type) {
> -			unsigned int port_mask;
>  		case INTEL_OUTPUT_DDI:
>  			if (WARN_ON(!HAS_DDI(to_i915(dev))))
>  				break;
>  			/* else, fall through */
>  		case INTEL_OUTPUT_DP:
>  		case INTEL_OUTPUT_HDMI:
> -		case INTEL_OUTPUT_EDP:
> +		case INTEL_OUTPUT_EDP: {
> +			unsigned int port_mask;
> +
>  			port_mask = 1 << encoder->port;
>  
>  			/* the same port mustn't appear more than once */
> @@ -12976,6 +12977,7 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  
>  			used_ports |= port_mask;
>  			break;
> +		}
>  		case INTEL_OUTPUT_DP_MST:
>  			used_mst_ports |=
>  				1 << encoder->port;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bd2d30ecc030..17d8833787c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -469,9 +469,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
>  	struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
>  	enum pipe pipe = crtc->pipe;
>  	int sprite0_start, sprite1_start;
> +	u32 dsparb, dsparb2, dsparb3;
>  
>  	switch (pipe) {
> -		u32 dsparb, dsparb2, dsparb3;
>  	case PIPE_A:
>  		dsparb = I915_READ(DSPARB);
>  		dsparb2 = I915_READ(DSPARB2);
> @@ -1969,6 +1969,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  	const struct vlv_fifo_state *fifo_state =
>  		&crtc_state->wm.vlv.fifo_state;
>  	int sprite0_start, sprite1_start, fifo_size;
> +	u32 dsparb, dsparb2, dsparb3;
>  
>  	if (!crtc_state->fifo_changed)
>  		return;
> @@ -1994,7 +1995,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  	spin_lock(&uncore->lock);
>  
>  	switch (crtc->pipe) {
> -		u32 dsparb, dsparb2, dsparb3;
>  	case PIPE_A:
>  		dsparb = intel_uncore_read_fw(uncore, DSPARB);
>  		dsparb2 = intel_uncore_read_fw(uncore, DSPARB2);
>

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization
  2020-02-20  6:22 [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization Kees Cook
  2020-02-20  7:02 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
  2020-02-20 10:21 ` [Intel-gfx] [PATCH] " Jani Nikula
@ 2020-02-20 13:47 ` Ville Syrjälä
  2 siblings, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2020-02-20 13:47 UTC (permalink / raw)
  To: Kees Cook; +Cc: intel-gfx, linux-kernel, Alexander Potapenko

On Wed, Feb 19, 2020 at 10:22:58PM -0800, Kees Cook wrote:
> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1].

Silly compilers.

> Note that these warnings (or silent
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
> 
> To avoid these problems, move such variables into the "case" where
> they're used or lift them up into the main function body.
> 
> drivers/gpu/drm/i915/display/intel_display.c: In function ‘check_digital_port_conflicts’:
> drivers/gpu/drm/i915/display/intel_display.c:12963:17: warning: statement will never be executed [-Wswitch-unreachable]
> 12963 |    unsigned int port_mask;
>       |                 ^~~~~~~~~
> 
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_get_fifo_size’:
> drivers/gpu/drm/i915/intel_pm.c:474:7: warning: statement will never be executed [-Wswitch-unreachable]
>   474 |   u32 dsparb, dsparb2, dsparb3;
>       |       ^~~~~~
> drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_atomic_update_fifo’:
> drivers/gpu/drm/i915/intel_pm.c:1997:7: warning: statement will never be executed [-Wswitch-unreachable]
>  1997 |   u32 dsparb, dsparb2, dsparb3;
>       |       ^~~~~~
> 
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |    6 ++++--
>  drivers/gpu/drm/i915/intel_pm.c              |    4 ++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 064dd99bbc49..c829cd26f99e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12960,14 +12960,15 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  		WARN_ON(!connector_state->crtc);
>  
>  		switch (encoder->type) {
> -			unsigned int port_mask;
>  		case INTEL_OUTPUT_DDI:
>  			if (WARN_ON(!HAS_DDI(to_i915(dev))))
>  				break;
>  			/* else, fall through */
>  		case INTEL_OUTPUT_DP:
>  		case INTEL_OUTPUT_HDMI:
> -		case INTEL_OUTPUT_EDP:
> +		case INTEL_OUTPUT_EDP: {
> +			unsigned int port_mask;

This one I'd just remove and s/port_mask/BIT(encoder->port)/
everywhere. Otherwise lgtm.

> +
>  			port_mask = 1 << encoder->port;
>  
>  			/* the same port mustn't appear more than once */
> @@ -12976,6 +12977,7 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  
>  			used_ports |= port_mask;
>  			break;
> +		}
>  		case INTEL_OUTPUT_DP_MST:
>  			used_mst_ports |=
>  				1 << encoder->port;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bd2d30ecc030..17d8833787c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -469,9 +469,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
>  	struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
>  	enum pipe pipe = crtc->pipe;
>  	int sprite0_start, sprite1_start;
> +	u32 dsparb, dsparb2, dsparb3;
>  
>  	switch (pipe) {
> -		u32 dsparb, dsparb2, dsparb3;
>  	case PIPE_A:
>  		dsparb = I915_READ(DSPARB);
>  		dsparb2 = I915_READ(DSPARB2);
> @@ -1969,6 +1969,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  	const struct vlv_fifo_state *fifo_state =
>  		&crtc_state->wm.vlv.fifo_state;
>  	int sprite0_start, sprite1_start, fifo_size;
> +	u32 dsparb, dsparb2, dsparb3;
>  
>  	if (!crtc_state->fifo_changed)
>  		return;
> @@ -1994,7 +1995,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  	spin_lock(&uncore->lock);
>  
>  	switch (crtc->pipe) {
> -		u32 dsparb, dsparb2, dsparb3;
>  	case PIPE_A:
>  		dsparb = intel_uncore_read_fw(uncore, DSPARB);
>  		dsparb2 = intel_uncore_read_fw(uncore, DSPARB2);
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization
  2020-02-20 10:21 ` [Intel-gfx] [PATCH] " Jani Nikula
@ 2020-02-20 23:42   ` Kees Cook
  0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2020-02-20 23:42 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx, Alexander Potapenko, linux-kernel

On Thu, Feb 20, 2020 at 12:21:14PM +0200, Jani Nikula wrote:
> On Wed, 19 Feb 2020, Kees Cook <keescook@chromium.org> wrote:
> > Variables declared in a switch statement before any case statements
> > cannot be automatically initialized with compiler instrumentation (as
> > they are not part of any execution flow). With GCC's proposed automatic
> > stack variable initialization feature, this triggers a warning (and they
> > don't get initialized). Clang's automatic stack variable initialization
> > (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> > doesn't initialize such variables[1]. Note that these warnings (or silent
> > skipping) happen before the dead-store elimination optimization phase,
> > so even when the automatic initializations are later elided in favor of
> > direct initializations, the warnings remain.
> >
> > To avoid these problems, move such variables into the "case" where
> > they're used or lift them up into the main function body.
> >
> > drivers/gpu/drm/i915/display/intel_display.c: In function ‘check_digital_port_conflicts’:
> > drivers/gpu/drm/i915/display/intel_display.c:12963:17: warning: statement will never be executed [-Wswitch-unreachable]
> > 12963 |    unsigned int port_mask;
> >       |                 ^~~~~~~~~
> >
> > drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_get_fifo_size’:
> > drivers/gpu/drm/i915/intel_pm.c:474:7: warning: statement will never be executed [-Wswitch-unreachable]
> >   474 |   u32 dsparb, dsparb2, dsparb3;
> >       |       ^~~~~~
> > drivers/gpu/drm/i915/intel_pm.c: In function ‘vlv_atomic_update_fifo’:
> > drivers/gpu/drm/i915/intel_pm.c:1997:7: warning: statement will never be executed [-Wswitch-unreachable]
> >  1997 |   u32 dsparb, dsparb2, dsparb3;
> >       |       ^~~~~~
> >
> > [1] https://bugs.llvm.org/show_bug.cgi?id=44916
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks!

> 
> If you look at i915/Makefile, you'll see that we don't shy away from
> enabling lots of extra warnings, and we run our CI with -Werror to keep
> it clean. It does not seem like -Wswitch-unreachable does me any good,
> though... is it new?

It's already enabled by default, but the GCC feature that tweaks it
doesn't exist yet. But it points out a problem that exists for Clang
today, but Clang doesn't actually warn on (yet). So this is a fix to
avoid the silent Clang problem and fix future warnings before they
happen.

-Kees

> 
> BR,
> Jani.
> 
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |    6 ++++--
> >  drivers/gpu/drm/i915/intel_pm.c              |    4 ++--
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 064dd99bbc49..c829cd26f99e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12960,14 +12960,15 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
> >  		WARN_ON(!connector_state->crtc);
> >  
> >  		switch (encoder->type) {
> > -			unsigned int port_mask;
> >  		case INTEL_OUTPUT_DDI:
> >  			if (WARN_ON(!HAS_DDI(to_i915(dev))))
> >  				break;
> >  			/* else, fall through */
> >  		case INTEL_OUTPUT_DP:
> >  		case INTEL_OUTPUT_HDMI:
> > -		case INTEL_OUTPUT_EDP:
> > +		case INTEL_OUTPUT_EDP: {
> > +			unsigned int port_mask;
> > +
> >  			port_mask = 1 << encoder->port;
> >  
> >  			/* the same port mustn't appear more than once */
> > @@ -12976,6 +12977,7 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
> >  
> >  			used_ports |= port_mask;
> >  			break;
> > +		}
> >  		case INTEL_OUTPUT_DP_MST:
> >  			used_mst_ports |=
> >  				1 << encoder->port;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bd2d30ecc030..17d8833787c4 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -469,9 +469,9 @@ static void vlv_get_fifo_size(struct intel_crtc_state *crtc_state)
> >  	struct vlv_fifo_state *fifo_state = &crtc_state->wm.vlv.fifo_state;
> >  	enum pipe pipe = crtc->pipe;
> >  	int sprite0_start, sprite1_start;
> > +	u32 dsparb, dsparb2, dsparb3;
> >  
> >  	switch (pipe) {
> > -		u32 dsparb, dsparb2, dsparb3;
> >  	case PIPE_A:
> >  		dsparb = I915_READ(DSPARB);
> >  		dsparb2 = I915_READ(DSPARB2);
> > @@ -1969,6 +1969,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> >  	const struct vlv_fifo_state *fifo_state =
> >  		&crtc_state->wm.vlv.fifo_state;
> >  	int sprite0_start, sprite1_start, fifo_size;
> > +	u32 dsparb, dsparb2, dsparb3;
> >  
> >  	if (!crtc_state->fifo_changed)
> >  		return;
> > @@ -1994,7 +1995,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> >  	spin_lock(&uncore->lock);
> >  
> >  	switch (crtc->pipe) {
> > -		u32 dsparb, dsparb2, dsparb3;
> >  	case PIPE_A:
> >  		dsparb = intel_uncore_read_fw(uncore, DSPARB);
> >  		dsparb2 = intel_uncore_read_fw(uncore, DSPARB2);
> >
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-02-20 23:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20  6:22 [Intel-gfx] [PATCH] drm/i915: Distribute switch variables for initialization Kees Cook
2020-02-20  7:02 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2020-02-20 10:21 ` [Intel-gfx] [PATCH] " Jani Nikula
2020-02-20 23:42   ` Kees Cook
2020-02-20 13:47 ` Ville Syrjälä

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).