| Message ID | 20220121210618.3482550-6-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: simplify RM code | expand |
On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote: > Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If > the value is NULL, then the function will return 0 instead of a proper > return code. Moreover none of dpu_hw_*_init() functions can return NULL. > So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR(). > Can you please give an example of a case where dpu_hw_*_init() can return NULL? All dpu_hw_*_init() functions are only called if the corresponding hw*_counts are valid. So I would like to understand this. Now, if NULL is treated as a non-error case, should we atleast print a message indicating so? > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 96554e962e38..7497538adae1 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > hw = dpu_hw_lm_init(lm->id, mmio, cat); > - if (IS_ERR_OR_NULL(hw)) { > + if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed lm object creation: err %d\n", rc); > goto fail; > @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat); > - if (IS_ERR_OR_NULL(hw)) { > + if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed merge_3d object creation: err %d\n", > rc); > @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > hw = dpu_hw_pingpong_init(pp->id, mmio, cat); > - if (IS_ERR_OR_NULL(hw)) { > + if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed pingpong object creation: err %d\n", > rc); > @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > hw = dpu_hw_intf_init(intf->id, mmio, cat); > - if (IS_ERR_OR_NULL(hw)) { > + if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed intf object creation: err %d\n", rc); > goto fail; > @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > hw = dpu_hw_ctl_init(ctl->id, mmio, cat); > - if (IS_ERR_OR_NULL(hw)) { > + if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed ctl object creation: err %d\n", rc); > goto fail; > @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm, > continue; > } > hw = dpu_hw_dspp_init(dspp->id, mmio, cat); > - if (IS_ERR_OR_NULL(hw)) { > + if (IS_ERR(hw)) { > rc = PTR_ERR(hw); > DPU_ERROR("failed dspp object creation: err %d\n", rc); > goto fail;
On 14/02/2022 22:15, Abhinav Kumar wrote: > > > On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote: >> Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If >> the value is NULL, then the function will return 0 instead of a proper >> return code. Moreover none of dpu_hw_*_init() functions can return NULL. >> So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR(). >> > Can you please give an example of a case where dpu_hw_*_init() can > return NULL? > > All dpu_hw_*_init() functions are only called if the corresponding > hw*_counts are valid. So I would like to understand this. > > Now, if NULL is treated as a non-error case, should we atleast print > a message indicating so? - No dpu_hw_*init() can return NULL - If at some point any of these functions returns NULL, it will cause a premature dpu_rm_init() termination with the success (=0) status, leaving parts of RM uninitialized. Thus I'm replacing IS_ERR_OR_NULL with IS_ERR() > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> index 96554e962e38..7497538adae1 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm, >> continue; >> } >> hw = dpu_hw_lm_init(lm->id, mmio, cat); >> - if (IS_ERR_OR_NULL(hw)) { >> + if (IS_ERR(hw)) { >> rc = PTR_ERR(hw); >> DPU_ERROR("failed lm object creation: err %d\n", rc); >> goto fail; >> @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm, >> continue; >> } >> hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat); >> - if (IS_ERR_OR_NULL(hw)) { >> + if (IS_ERR(hw)) { >> rc = PTR_ERR(hw); >> DPU_ERROR("failed merge_3d object creation: err %d\n", >> rc); >> @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm, >> continue; >> } >> hw = dpu_hw_pingpong_init(pp->id, mmio, cat); >> - if (IS_ERR_OR_NULL(hw)) { >> + if (IS_ERR(hw)) { >> rc = PTR_ERR(hw); >> DPU_ERROR("failed pingpong object creation: err %d\n", >> rc); >> @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm, >> continue; >> } >> hw = dpu_hw_intf_init(intf->id, mmio, cat); >> - if (IS_ERR_OR_NULL(hw)) { >> + if (IS_ERR(hw)) { >> rc = PTR_ERR(hw); >> DPU_ERROR("failed intf object creation: err %d\n", rc); >> goto fail; >> @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm, >> continue; >> } >> hw = dpu_hw_ctl_init(ctl->id, mmio, cat); >> - if (IS_ERR_OR_NULL(hw)) { >> + if (IS_ERR(hw)) { >> rc = PTR_ERR(hw); >> DPU_ERROR("failed ctl object creation: err %d\n", rc); >> goto fail; >> @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm, >> continue; >> } >> hw = dpu_hw_dspp_init(dspp->id, mmio, cat); >> - if (IS_ERR_OR_NULL(hw)) { >> + if (IS_ERR(hw)) { >> rc = PTR_ERR(hw); >> DPU_ERROR("failed dspp object creation: err %d\n", rc); >> goto fail;
On 2/14/2022 12:43 PM, Dmitry Baryshkov wrote: > On 14/02/2022 22:15, Abhinav Kumar wrote: >> >> >> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote: >>> Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If >>> the value is NULL, then the function will return 0 instead of a proper >>> return code. Moreover none of dpu_hw_*_init() functions can return NULL. >>> So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR(). >>> >> Can you please give an example of a case where dpu_hw_*_init() can >> return NULL? >> >> All dpu_hw_*_init() functions are only called if the corresponding >> hw*_counts are valid. So I would like to understand this. >> >> Now, if NULL is treated as a non-error case, should we atleast print >> a message indicating so? > > - No dpu_hw_*init() can return NULL > > - If at some point any of these functions returns NULL, it will cause a > premature dpu_rm_init() termination with the success (=0) status, > leaving parts of RM uninitialized. > > Thus I'm replacing IS_ERR_OR_NULL with IS_ERR() Ah okay, got it. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------ >>> 1 file changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> index 96554e962e38..7497538adae1 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> hw = dpu_hw_lm_init(lm->id, mmio, cat); >>> - if (IS_ERR_OR_NULL(hw)) { >>> + if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed lm object creation: err %d\n", rc); >>> goto fail; >>> @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat); >>> - if (IS_ERR_OR_NULL(hw)) { >>> + if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed merge_3d object creation: err %d\n", >>> rc); >>> @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> hw = dpu_hw_pingpong_init(pp->id, mmio, cat); >>> - if (IS_ERR_OR_NULL(hw)) { >>> + if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed pingpong object creation: err %d\n", >>> rc); >>> @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> hw = dpu_hw_intf_init(intf->id, mmio, cat); >>> - if (IS_ERR_OR_NULL(hw)) { >>> + if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed intf object creation: err %d\n", rc); >>> goto fail; >>> @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> hw = dpu_hw_ctl_init(ctl->id, mmio, cat); >>> - if (IS_ERR_OR_NULL(hw)) { >>> + if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed ctl object creation: err %d\n", rc); >>> goto fail; >>> @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm, >>> continue; >>> } >>> hw = dpu_hw_dspp_init(dspp->id, mmio, cat); >>> - if (IS_ERR_OR_NULL(hw)) { >>> + if (IS_ERR(hw)) { >>> rc = PTR_ERR(hw); >>> DPU_ERROR("failed dspp object creation: err %d\n", rc); >>> goto fail; > >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 96554e962e38..7497538adae1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm, continue; } hw = dpu_hw_lm_init(lm->id, mmio, cat); - if (IS_ERR_OR_NULL(hw)) { + if (IS_ERR(hw)) { rc = PTR_ERR(hw); DPU_ERROR("failed lm object creation: err %d\n", rc); goto fail; @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm, continue; } hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat); - if (IS_ERR_OR_NULL(hw)) { + if (IS_ERR(hw)) { rc = PTR_ERR(hw); DPU_ERROR("failed merge_3d object creation: err %d\n", rc); @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm, continue; } hw = dpu_hw_pingpong_init(pp->id, mmio, cat); - if (IS_ERR_OR_NULL(hw)) { + if (IS_ERR(hw)) { rc = PTR_ERR(hw); DPU_ERROR("failed pingpong object creation: err %d\n", rc); @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm, continue; } hw = dpu_hw_intf_init(intf->id, mmio, cat); - if (IS_ERR_OR_NULL(hw)) { + if (IS_ERR(hw)) { rc = PTR_ERR(hw); DPU_ERROR("failed intf object creation: err %d\n", rc); goto fail; @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm, continue; } hw = dpu_hw_ctl_init(ctl->id, mmio, cat); - if (IS_ERR_OR_NULL(hw)) { + if (IS_ERR(hw)) { rc = PTR_ERR(hw); DPU_ERROR("failed ctl object creation: err %d\n", rc); goto fail; @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm, continue; } hw = dpu_hw_dspp_init(dspp->id, mmio, cat); - if (IS_ERR_OR_NULL(hw)) { + if (IS_ERR(hw)) { rc = PTR_ERR(hw); DPU_ERROR("failed dspp object creation: err %d\n", rc); goto fail;
Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If the value is NULL, then the function will return 0 instead of a proper return code. Moreover none of dpu_hw_*_init() functions can return NULL. So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR(). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)