diff mbox series

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

Message ID 20220121210618.3482550-6-dmitry.baryshkov@linaro.org
State Accepted
Commit 740828c73a36028f560e378f0007717bcfa27a02
Headers show
Series drm/msm/dpu: simplify RM code | expand

Commit Message

Dmitry Baryshkov Jan. 21, 2022, 9:06 p.m. UTC
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(-)

Comments

Abhinav Kumar Feb. 14, 2022, 7:15 p.m. UTC | #1
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;
Dmitry Baryshkov Feb. 14, 2022, 8:43 p.m. UTC | #2
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;
Abhinav Kumar Feb. 14, 2022, 9:25 p.m. UTC | #3
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 mbox series

Patch

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;