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@xxxxxxxxx>
>> Reported-by: Dan Carpenter <error27@xxxxxxxxx>
>> Signed-off-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx>
>> ---
>> 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
