Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly

From: Michael Riesch <michael.riesch@wolfvision.net>
To: Nathan Chancellor <nathan@kernel.org>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Sandy Huang <hjc@rock-chips.com>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Sascha Hauer <sha@pengutronix.de>,
	kernel test robot <lkp@intel.com>,
	Dan Carpenter <error27@gmail.com>,
	llvm@lists.linux.dev
Subject: Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly
Date: Wed, 15 Mar 2023 09:35:03 +0100	[thread overview]
Message-ID: <46483806-a382-3ac6-62ab-fe2506444ee3@wolfvision.net> (raw)
In-Reply-To: <20230314160821.GA13416@dev-arch.thelio-3990X>

Hi Nathan,

On 3/14/23 17:08, Nathan Chancellor wrote:
> Hi Michael,
> 
> On Tue, Jan 24, 2023 at 06:47:01AM +0100, Michael Riesch wrote:
>> The variable possible_crtcs is only initialized for primary and
>> overlay planes. Since the VOP2 driver only supports these plane
>> types at the moment, the current code is safe. However, in order
>> to provide a future-proof solution, fix the initialization of
>> the variable.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <error27@gmail.com>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>> v3:
>>  - no changes
>> v2:
>>  - new patch
>>
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 8cecf81a5ae0..374ef821b453 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
>>  				/* change the unused primary window to overlay window */
>>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>>  			}
>> -		}
>> -
>> -		if (win->type == DRM_PLANE_TYPE_OVERLAY)
>> +		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>>  			possible_crtcs = (1 << nvps) - 1;
>> +		} else {
>> +			possible_crtcs = 0;
>> +		}
>>  
>>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>>  		if (ret) {
>> -- 
>> 2.30.2
>>
> 
> This patch is now in -next as commit 368419a2d429 ("drm/rockchip: vop2:
> initialize possible_crtcs properly") and it actually appears to
> introduce a path where possible_crtcs could be used uninitialized.
> 
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316: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:2330:36: note: uninitialized use occurs here
>                   ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                    ^~~~~~~~~~~~~~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:4: note: remove the 'if' if its condition is always true
>                           if (vp) {
>                           ^~~~~~~~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2298:21: note: initialize the variable 'possible_crtcs' to silence this warning
>                   u32 possible_crtcs;
>                                     ^
>                                      = 0
>   1 error generated.
> 
> Prior to this change, if that else path was hit, clang recognized based on
> the assignment that the next if statement would always be true. Now, if
> the else path is taken, the possible_crtcs assignment will be missed. Is
> that intentional?

As it turns out, the approach in my patch does not cover all paths. I'll
submit a follow-up patch that initializes possible_crtcs = 0 and drops
the else path. This should solve the issue for good.

Regards, Michael