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

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Stephen Boyd <swboyd@chromium.org>,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH v5 5/6] drm/msm/dpu: fix error handling in dpu_rm_init
Date: Mon, 14 Feb 2022 11:15:38 -0800	[thread overview]
Message-ID: <5b517150-ae78-98c6-b9a8-d84905f38f56@quicinc.com> (raw)
In-Reply-To: <20220121210618.3482550-6-dmitry.baryshkov@linaro.org>

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;