| Message ID | ZSj+6/J6YsoSpLak@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/msm: remove unnecessary NULL check | expand |
On Fri, Oct 13, 2023 at 11:25:15AM +0300, Dan Carpenter wrote: > This NULL check was required when it was added, but we shuffled the code > around and now it's not. The inconsistent NULL checking triggers a > Smatch warning: > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: > variable dereferenced before check 'mdp5_kms' (see line 782) > > Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe function" > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> (already provided for (implicit) v1, but wasn't picked up) Thanks Uwe
On 13/10/2023 11:25, Dan Carpenter wrote: > This NULL check was required when it was added, but we shuffled the code > around and now it's not. The inconsistent NULL checking triggers a > Smatch warning: > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: > variable dereferenced before check 'mdp5_kms' (see line 782) > > Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe function" > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: Added a Fixes tag. It's not really a bug fix and so adding the > fixes tag is slightly unfair but it should prevent this patch from > accidentally getting backported before the refactoring and causing an > issue. > > Btw, fixes tags are often unfair like this. People look at fixes tags > and think, "the fix introduced a bug" but actually it's really common > that the fix was just not complete. But from a backporting perspective > it makes sense to tie them together. > > Plus everyone introduces bugs. If you're not introducing bugs, then > you're probably not writing a lot of code. > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On 10/13/2023 1:25 AM, Dan Carpenter wrote: > This NULL check was required when it was added, but we shuffled the code > around and now it's not. The inconsistent NULL checking triggers a > Smatch warning: > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: > variable dereferenced before check 'mdp5_kms' (see line 782) > > Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe function" > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: Added a Fixes tag. It's not really a bug fix and so adding the > fixes tag is slightly unfair but it should prevent this patch from > accidentally getting backported before the refactoring and causing an > issue. > > Btw, fixes tags are often unfair like this. People look at fixes tags > and think, "the fix introduced a bug" but actually it's really common > that the fix was just not complete. But from a backporting perspective > it makes sense to tie them together. > > Plus everyone introduces bugs. If you're not introducing bugs, then > you're probably not writing a lot of code. > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > LGTM, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On 11/1/2023 12:23 PM, Abhinav Kumar wrote: > > > On 10/13/2023 1:25 AM, Dan Carpenter wrote: >> This NULL check was required when it was added, but we shuffled the code >> around and now it's not. The inconsistent NULL checking triggers a >> Smatch warning: >> >> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: >> variable dereferenced before check 'mdp5_kms' (see line 782) >> >> Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the >> _probe function" A small error here. Its missing the closing brace for the Fixes tag. Checkpatch cries without it. I have fixed it while applying. >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> v2: Added a Fixes tag. It's not really a bug fix and so adding the >> fixes tag is slightly unfair but it should prevent this patch from >> accidentally getting backported before the refactoring and causing an >> issue. >> >> Btw, fixes tags are often unfair like this. People look at fixes tags >> and think, "the fix introduced a bug" but actually it's really common >> that the fix was just not complete. But from a backporting perspective >> it makes sense to tie them together. >> >> Plus everyone introduces bugs. If you're not introducing bugs, then >> you're probably not writing a lot of code. >> >> drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> > > LGTM, > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >
On Thu, Nov 16, 2023 at 01:05:52PM -0800, Abhinav Kumar wrote: > > > On 11/1/2023 12:23 PM, Abhinav Kumar wrote: > > > > > > On 10/13/2023 1:25 AM, Dan Carpenter wrote: > > > This NULL check was required when it was added, but we shuffled the code > > > around and now it's not.? The inconsistent NULL checking triggers a > > > Smatch warning: > > > > > > ???? drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: > > > ???? variable dereferenced before check 'mdp5_kms' (see line 782) > > > > > > Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the > > > _probe function" > > A small error here. Its missing the closing brace for the Fixes tag. > Checkpatch cries without it. > Sorry. I must have accidentally deleted it after I ran checkpatch. > I have fixed it while applying. Thanks! regards, dan carpenter
On Fri, 13 Oct 2023 11:25:15 +0300, Dan Carpenter wrote: > This NULL check was required when it was added, but we shuffled the code > around and now it's not. The inconsistent NULL checking triggers a > Smatch warning: > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: > variable dereferenced before check 'mdp5_kms' (see line 782) > > [...] Applied, thanks! [1/1] drm/msm: remove unnecessary NULL check https://gitlab.freedesktop.org/drm/msm/-/commit/56466f653cb5 Best regards,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c index 11d9fc2c6bf5..ec933d597e20 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c @@ -844,8 +844,7 @@ static int mdp5_init(struct platform_device *pdev, struct drm_device *dev) return 0; fail: - if (mdp5_kms) - mdp5_destroy(mdp5_kms); + mdp5_destroy(mdp5_kms); return ret; }
This NULL check was required when it was added, but we shuffled the code around and now it's not. The inconsistent NULL checking triggers a Smatch warning: drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: variable dereferenced before check 'mdp5_kms' (see line 782) Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe function" Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: Added a Fixes tag. It's not really a bug fix and so adding the fixes tag is slightly unfair but it should prevent this patch from accidentally getting backported before the refactoring and causing an issue. Btw, fixes tags are often unfair like this. People look at fixes tags and think, "the fix introduced a bug" but actually it's really common that the fix was just not complete. But from a backporting perspective it makes sense to tie them together. Plus everyone introduces bugs. If you're not introducing bugs, then you're probably not writing a lot of code. drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)