Re: [PATCH v5 5/6] drm/msm/dpu: fix error handling in dpu_rm_init

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > >   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;