diff mbox series

drm/rockchip: vop2: fix uninitialized variable possible_crtcs

Message ID 20230316132302.531724-1-trix@redhat.com
State New
Headers
Series drm/rockchip: vop2: fix uninitialized variable possible_crtcs |

Commit Message

Tom Rix March 16, 2023, 1:23 p.m. UTC 
  clang reportes this error
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:8: error:
  variable 'possible_crtcs' is used uninitialized whenever 'if'
  condition is false [-Werror,-Wsometimes-uninitialized]
                        if (vp) {
                            ^~
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2336:36: note:
  uninitialized use occurs here
                ret = vop2_plane_init(vop2, win, possible_crtcs);
                                                 ^~~~~~~~~~~~~~
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:4:
  note: remove the 'if' if its condition is always true
                        if (vp) {
                        ^~~~~~~~

The else-statement changes the win->type to OVERLAY without setting the
possible_crtcs variable.  Rework the block, initialize possible_crtcs to
0 to remove the else-statement.  Split the else-if-statement out to its
own if-statement so the OVERLAY check will catch when the win-type has
been changed.

Fixes: 368419a2d429 ("drm/rockchip: vop2: initialize possible_crtcs properly")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Heiko Stübner March 16, 2023, 2:05 p.m. UTC | #1 
Am Donnerstag, 16. März 2023, 14:23:02 CET schrieb Tom Rix:
> clang reportes this error
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:8: error:
>   variable 'possible_crtcs' is used uninitialized whenever 'if'
>   condition is false [-Werror,-Wsometimes-uninitialized]
>                         if (vp) {
>                             ^~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2336:36: note:
>   uninitialized use occurs here
>                 ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                  ^~~~~~~~~~~~~~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:4:
>   note: remove the 'if' if its condition is always true
>                         if (vp) {
>                         ^~~~~~~~
> 
> The else-statement changes the win->type to OVERLAY without setting the
> possible_crtcs variable.  Rework the block, initialize possible_crtcs to
> 0 to remove the else-statement.  Split the else-if-statement out to its
> own if-statement so the OVERLAY check will catch when the win-type has
> been changed.
> 
> Fixes: 368419a2d429 ("drm/rockchip: vop2: initialize possible_crtcs properly")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 03ca32cd2050..fce992c3506f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2301,7 +2301,7 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>  	nvp = 0;
>  	for (i = 0; i < vop2->registered_num_wins; i++) {
>  		struct vop2_win *win = &vop2->win[i];
> -		u32 possible_crtcs;
> +		u32 possible_crtcs = 0;
>  
>  		if (vop2->data->soc_id == 3566) {
>  			/*
> @@ -2327,12 +2327,11 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>  				/* change the unused primary window to overlay window */
>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>  			}
> -		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
> -			possible_crtcs = (1 << nvps) - 1;
> -		} else {
> -			possible_crtcs = 0;
>  		}
>  
> +		if (win->type == DRM_PLANE_TYPE_OVERLAY)
> +			possible_crtcs = (1 << nvps) - 1;
> +

After a long hard stare at the code in question, I think doing it this
way looks like the correct one, as as you mention in the commit message
the first "if" will change the win->type to OVERLAY in one case, but this
then will never be added.

Michael, do you agree/disagree?


Thanks
Heiko

https://lore.kernel.org/r/20230315090158.2442771-1-michael.riesch@wolfvision.net

>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>  		if (ret) {
>  			drm_err(vop2->drm, "failed to init plane %s: %d\n",
>
Michael Riesch March 16, 2023, 2:18 p.m. UTC | #2 
Hi Tom, Heiko,

On 3/16/23 15:05, Heiko Stuebner wrote:
> Am Donnerstag, 16. März 2023, 14:23:02 CET schrieb Tom Rix:
>> clang reportes this error
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:8: error:
>>   variable 'possible_crtcs' is used uninitialized whenever 'if'
>>   condition is false [-Werror,-Wsometimes-uninitialized]
>>                         if (vp) {
>>                             ^~
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2336:36: note:
>>   uninitialized use occurs here
>>                 ret = vop2_plane_init(vop2, win, possible_crtcs);
>>                                                  ^~~~~~~~~~~~~~
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:4:
>>   note: remove the 'if' if its condition is always true
>>                         if (vp) {
>>                         ^~~~~~~~
>>
>> The else-statement changes the win->type to OVERLAY without setting the
>> possible_crtcs variable.  Rework the block, initialize possible_crtcs to
>> 0 to remove the else-statement.  Split the else-if-statement out to its
>> own if-statement so the OVERLAY check will catch when the win-type has
>> been changed.
>>
>> Fixes: 368419a2d429 ("drm/rockchip: vop2: initialize possible_crtcs properly")
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 03ca32cd2050..fce992c3506f 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2301,7 +2301,7 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>>  	nvp = 0;
>>  	for (i = 0; i < vop2->registered_num_wins; i++) {
>>  		struct vop2_win *win = &vop2->win[i];
>> -		u32 possible_crtcs;
>> +		u32 possible_crtcs = 0;
>>  
>>  		if (vop2->data->soc_id == 3566) {
>>  			/*
>> @@ -2327,12 +2327,11 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>>  				/* change the unused primary window to overlay window */
>>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>>  			}
>> -		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>> -			possible_crtcs = (1 << nvps) - 1;
>> -		} else {
>> -			possible_crtcs = 0;
>>  		}
>>  
>> +		if (win->type == DRM_PLANE_TYPE_OVERLAY)
>> +			possible_crtcs = (1 << nvps) - 1;
>> +
> 
> After a long hard stare at the code in question, I think doing it this
> way looks like the correct one, as as you mention in the commit message
> the first "if" will change the win->type to OVERLAY in one case, but this
> then will never be added.
> 
> Michael, do you agree/disagree?

Shoot, this bit of code is more complicated than I believed it would be.
Yes, Tom's patch makes sense to me. But having overlooked the win->type
change in the if-clause makes me think I shouldn't be the judge of that
:-) Still,

Acked-by: Michael Riesch <michael.riesch@wolfvision.net>

Thanks for fixing it and best regards,
Michael

> 
> 
> Thanks
> Heiko
> 
> https://lore.kernel.org/r/20230315090158.2442771-1-michael.riesch@wolfvision.net
> 
>>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>>  		if (ret) {
>>  			drm_err(vop2->drm, "failed to init plane %s: %d\n",
>>
> 
> 
> 
>
Nathan Chancellor March 20, 2023, 5 p.m. UTC | #3 
On Thu, Mar 16, 2023 at 09:23:02AM -0400, Tom Rix wrote:
> clang reportes this error
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:8: error:
>   variable 'possible_crtcs' is used uninitialized whenever 'if'
>   condition is false [-Werror,-Wsometimes-uninitialized]
>                         if (vp) {
>                             ^~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2336:36: note:
>   uninitialized use occurs here
>                 ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                  ^~~~~~~~~~~~~~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:4:
>   note: remove the 'if' if its condition is always true
>                         if (vp) {
>                         ^~~~~~~~
> 
> The else-statement changes the win->type to OVERLAY without setting the
> possible_crtcs variable.  Rework the block, initialize possible_crtcs to
> 0 to remove the else-statement.  Split the else-if-statement out to its
> own if-statement so the OVERLAY check will catch when the win-type has
> been changed.
> 
> Fixes: 368419a2d429 ("drm/rockchip: vop2: initialize possible_crtcs properly")
> Signed-off-by: Tom Rix <trix@redhat.com>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 03ca32cd2050..fce992c3506f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2301,7 +2301,7 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>  	nvp = 0;
>  	for (i = 0; i < vop2->registered_num_wins; i++) {
>  		struct vop2_win *win = &vop2->win[i];
> -		u32 possible_crtcs;
> +		u32 possible_crtcs = 0;
>  
>  		if (vop2->data->soc_id == 3566) {
>  			/*
> @@ -2327,12 +2327,11 @@ static int vop2_create_crtcs(struct vop2 *vop2)
>  				/* change the unused primary window to overlay window */
>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>  			}
> -		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
> -			possible_crtcs = (1 << nvps) - 1;
> -		} else {
> -			possible_crtcs = 0;
>  		}
>  
> +		if (win->type == DRM_PLANE_TYPE_OVERLAY)
> +			possible_crtcs = (1 << nvps) - 1;
> +
>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>  		if (ret) {
>  			drm_err(vop2->drm, "failed to init plane %s: %d\n",
> -- 
> 2.27.0
>
Heiko Stübner March 22, 2023, 8:09 p.m. UTC | #4 
On Thu, 16 Mar 2023 09:23:02 -0400, Tom Rix wrote:
> clang reportes this error
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:8: error:
>   variable 'possible_crtcs' is used uninitialized whenever 'if'
>   condition is false [-Werror,-Wsometimes-uninitialized]
>                         if (vp) {
>                             ^~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2336:36: note:
>   uninitialized use occurs here
>                 ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                  ^~~~~~~~~~~~~~
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2322:4:
>   note: remove the 'if' if its condition is always true
>                         if (vp) {
>                         ^~~~~~~~
> 
> [...]

Applied, thanks!

[1/1] drm/rockchip: vop2: fix uninitialized variable possible_crtcs
      commit: e88adb4ac27a37de4167150b8a6a736d40cf7018

Best regards,
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 03ca32cd2050..fce992c3506f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2301,7 +2301,7 @@  static int vop2_create_crtcs(struct vop2 *vop2)
 	nvp = 0;
 	for (i = 0; i < vop2->registered_num_wins; i++) {
 		struct vop2_win *win = &vop2->win[i];
-		u32 possible_crtcs;
+		u32 possible_crtcs = 0;
 
 		if (vop2->data->soc_id == 3566) {
 			/*
@@ -2327,12 +2327,11 @@  static int vop2_create_crtcs(struct vop2 *vop2)
 				/* change the unused primary window to overlay window */
 				win->type = DRM_PLANE_TYPE_OVERLAY;
 			}
-		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
-			possible_crtcs = (1 << nvps) - 1;
-		} else {
-			possible_crtcs = 0;
 		}
 
+		if (win->type == DRM_PLANE_TYPE_OVERLAY)
+			possible_crtcs = (1 << nvps) - 1;
+
 		ret = vop2_plane_init(vop2, win, possible_crtcs);
 		if (ret) {
 			drm_err(vop2->drm, "failed to init plane %s: %d\n",