| Message ID | 20230124054706.3921383-2-michael.riesch@wolfvision.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: vop2: add support for the rgb output block | expand |
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? Cheers, Nathan
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
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) {
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(-)