linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] drm/i915: Distribute switch variables for initialization
@ 2020-02-21  0:05 Kees Cook
  2020-02-21 11:30 ` [Intel-gfx] " Ville Syrjälä
  0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2020-02-21  0:05 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: Alexander Potapenko, intel-gfx, 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>
---
v2: remove port_mask entirely (Ville Syrjälä)
v1: https://lore.kernel.org/lkml/20200220062258.68854-1-keescook@chromium.org
---
 drivers/gpu/drm/i915/display/intel_display.c | 7 ++-----
 drivers/gpu/drm/i915/intel_pm.c              | 4 ++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 064dd99bbc49..5f8c61932e82 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -12960,7 +12960,6 @@ 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;
@@ -12968,13 +12967,11 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
 		case INTEL_OUTPUT_DP:
 		case INTEL_OUTPUT_HDMI:
 		case INTEL_OUTPUT_EDP:
-			port_mask = 1 << encoder->port;
-
 			/* the same port mustn't appear more than once */
-			if (used_ports & port_mask)
+			if (used_ports & BIT(encoder->port))
 				ret = false;
 
-			used_ports |= port_mask;
+			used_ports |= BIT(encoder->port);
 			break;
 		case INTEL_OUTPUT_DP_MST:
 			used_mst_ports |=
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);
-- 
2.20.1


-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915: Distribute switch variables for initialization
  2020-02-21  0:05 [PATCH v2] drm/i915: Distribute switch variables for initialization Kees Cook
@ 2020-02-21 11:30 ` Ville Syrjälä
  2020-02-23 15:32   ` Jani Nikula
  0 siblings, 1 reply; 3+ messages in thread
From: Ville Syrjälä @ 2020-02-21 11:30 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, intel-gfx,
	Alexander Potapenko, linux-kernel

On Thu, Feb 20, 2020 at 04:05:17PM -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]. 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>
> ---
> v2: remove port_mask entirely (Ville Syrjälä)
> v1: https://lore.kernel.org/lkml/20200220062258.68854-1-keescook@chromium.org
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 7 ++-----
>  drivers/gpu/drm/i915/intel_pm.c              | 4 ++--
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 064dd99bbc49..5f8c61932e82 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -12960,7 +12960,6 @@ 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;
> @@ -12968,13 +12967,11 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>  		case INTEL_OUTPUT_DP:
>  		case INTEL_OUTPUT_HDMI:
>  		case INTEL_OUTPUT_EDP:
> -			port_mask = 1 << encoder->port;
> -
>  			/* the same port mustn't appear more than once */
> -			if (used_ports & port_mask)
> +			if (used_ports & BIT(encoder->port))
>  				ret = false;
>  
> -			used_ports |= port_mask;
> +			used_ports |= BIT(encoder->port);

Thanks. Looks good.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  			break;
>  		case INTEL_OUTPUT_DP_MST:
>  			used_mst_ports |=
> 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);
> -- 
> 2.20.1
> 
> 
> -- 
> Kees Cook
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915: Distribute switch variables for initialization
  2020-02-21 11:30 ` [Intel-gfx] " Ville Syrjälä
@ 2020-02-23 15:32   ` Jani Nikula
  0 siblings, 0 replies; 3+ messages in thread
From: Jani Nikula @ 2020-02-23 15:32 UTC (permalink / raw)
  To: Ville Syrjälä, Kees Cook
  Cc: Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Alexander Potapenko,
	linux-kernel

On Fri, 21 Feb 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Feb 20, 2020 at 04:05:17PM -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]. 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>
>> ---
>> v2: remove port_mask entirely (Ville Syrjälä)
>> v1: https://lore.kernel.org/lkml/20200220062258.68854-1-keescook@chromium.org
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c | 7 ++-----
>>  drivers/gpu/drm/i915/intel_pm.c              | 4 ++--
>>  2 files changed, 4 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 064dd99bbc49..5f8c61932e82 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -12960,7 +12960,6 @@ 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;
>> @@ -12968,13 +12967,11 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>>  		case INTEL_OUTPUT_DP:
>>  		case INTEL_OUTPUT_HDMI:
>>  		case INTEL_OUTPUT_EDP:
>> -			port_mask = 1 << encoder->port;
>> -
>>  			/* the same port mustn't appear more than once */
>> -			if (used_ports & port_mask)
>> +			if (used_ports & BIT(encoder->port))
>>  				ret = false;
>>  
>> -			used_ports |= port_mask;
>> +			used_ports |= BIT(encoder->port);
>
> Thanks. Looks good.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the patch and review, pushed to dinq.

BR,
Jani.

>
>>  			break;
>>  		case INTEL_OUTPUT_DP_MST:
>>  			used_mst_ports |=
>> 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);
>> -- 
>> 2.20.1
>> 
>> 
>> -- 
>> Kees Cook
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-23 15:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-21  0:05 [PATCH v2] drm/i915: Distribute switch variables for initialization Kees Cook
2020-02-21 11:30 ` [Intel-gfx] " Ville Syrjälä
2020-02-23 15:32   ` Jani Nikula

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