[PATCH v3 03/20] drivers: reset: Handle gracefully NULL pointers

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi Simon,

On 06/05/21 5:07 am, Simon Glass wrote:
> Hi Kishon,
> 
> On Tue, 4 May 2021 at 21:25, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 04/05/21 10:28 pm, Simon Glass wrote:
>>> Hi Kishon,
>>>
>>> On Tue, 4 May 2021 at 04:42, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>
>>>> The reset framework provides devm_reset_control_get_optional()
>>>> which can return NULL (not an error case). So all the other reset_ops
>>>> should handle NULL gracefully. Prepare the way for a managed reset
>>>> API by handling NULL pointers without crashing nor failing.
>>>>
>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/reset/reset-uclass.c | 35 ++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>>
>>> I am still not a fan of this. There is no way to know whether the
>>> reset API call actually did anything. Normally we return -EINVAL or
>>> something like that when the value is invalid (e.g. see GPIO uclass).
>>
>> The reset modification is more in-line with how clk uclass is designed.
>>
>> There every API first checks if the clock is valid and returns 0 (will
>> be the case for optional clocks)
>>        if (!clk_valid(clk))
>>                 return 0;
>>
>> If you don't prefer this approach, I'll drop this patch and handle it
>> appropriately in the client driver (serdes driver). Please let me know.
> 
> Yes I see that with clk. IMO the return code is incorrect there also,
> since it should be -ENOENT or something like that, to indicate that
> there is actually no clock.
> 
> The clk.h header gives no indication that it accepts a NULL clock. So
> here is what I think:
> 
> 1. Update clk_valid() to require clk to be a valid pointer, so it is
> considered invalid only if clk-dev.

So for API's like clk_enable(), clk_disable(), should it return 0 or
-ENOENT; i.e if clk is valid and clk->dev is NULL?
> 
> 2. Update the reset 'managed API' to return an empty reset record
> instead of NULL.

Okay, something like below?

--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -326,9 +326,6 @@ struct reset_ctl
*devm_reset_control_get_optional(struct udevice *dev,
 {
        struct reset_ctl *r = devm_reset_control_get(dev, id);

-       if (IS_ERR(r))
-               return NULL;
-
        return r;
 }

The only thing to note here is _get_optional() is a wrapper to _get()
with no modification.
> 
> 3. Your problem goes away, and (I think) we are still compatible with
> the linux API.
> 
>>
>>>
>>> The function you mention says:  * Returns a struct reset_ctl or a
>>> dummy reset controller if it failed.
>>>
>>> But it does not appear to do that?
>>
>> Right, looks like it's incorrect from when the patch was actually
>> introduced. It was returning "NULL" for optional resets.
>>
>> commit 139e4a1cbe60de96d4febbc6db5882929801ca46
>> Author: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> Date:   Wed Sep 9 15:37:03 2020 +0530
>>
>>     drivers: reset: Add a managed API to get reset controllers from the DT
> 
> OK. So what should it be?

I meant the comment in header is incorrect since it was returning NULL
if it failed.

Thanks,
Kishon