[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

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

Hi Simon,

On 14/05/21 5:26 am, Simon Glass wrote:
> Hi Kishon,
> 
> On Thu, 13 May 2021 at 00:15, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 11/05/21 10:09 pm, Simon Glass wrote:
>>> Hi Kishon,
>>>
>>> On Tue, 11 May 2021 at 00:26, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 07/05/21 10:04 pm, Simon Glass wrote:
>>>>> Hi Kishon,
>>>>>
>>>>> On Fri, 7 May 2021 at 05:02, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>>>
>>>>>> In order for client to know whether it was able to successfully get a
>>>>>> reset controller or not, do not return NULL on error for
>>>>>> devm_reset_control_get_optional() and
>>>>>> devm_reset_bulk_get_optional_by_node().
>>>>>>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> ---
>>>>>>  drivers/reset/reset-uclass.c       | 16 ++--------------
>>>>>>  drivers/reset/sandbox-reset-test.c |  2 +-
>>>>>>  2 files changed, 3 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
>>>>>> index ac89eaf098..906f58ce3d 100644
>>>>>> --- a/drivers/reset/reset-uclass.c
>>>>>> +++ b/drivers/reset/reset-uclass.c
>>>>>> @@ -299,12 +299,7 @@ struct reset_ctl *devm_reset_control_get(struct udevice *dev, const char *id)
>>>>>>  struct reset_ctl *devm_reset_control_get_optional(struct udevice *dev,
>>>>>>                                                   const char *id)
>>>>>>  {
>>>>>> -       struct reset_ctl *r = devm_reset_control_get(dev, id);
>>>>>> -
>>>>>> -       if (IS_ERR(r))
>>>>>> -               return NULL;
>>>>>> -
>>>>>> -       return r;
>>>>>> +       return devm_reset_control_get(dev, id);
>>>>>>  }
>>>>>
>>>>> We need to get some updates to the API (function comments in the
>>>>> header) here. I'm not sure what the intent is.
>>>>
>>>> right, that has to be fixed.
>>>>>
>>>>> I thought these functions were going to return a valid (but possibly
>>>>> empty) reset_ctl always?
>>>>
>>>> I thought about it again and felt it might not be correct to return
>>>> reset_ctl always. The reset control is optional only if the consumer
>>>> node doesn't have populated reset properties.
>>>>
>>>> If we always return valid reset_ctl possibly with it's dev member
>>>> initialized or not initialized, it would not be possible to tell it's
>>>> not initialized because of the absence of reset property or due to some
>>>> other valid error.
>>>
>>> reset_valid() should check if dev is NULL or not, like with clock and
>>> GPIO. Is that enough?
>>
>> clock compares with ENODATA and return "0" (code snippet below). For
>> reset we are modifying this to return ENODATA itself and use it for
>> comparing in the reset APIs.
>>
>> int clk_get_optional_nodev(ofnode node, const char *name, struct clk *clk)
>> {
>>         int ret;
>>
>>         ret = clk_get_by_name_nodev(node, name, clk);
>>         if (ret == -ENODATA)
>>                 return 0;
>>
>>         return ret;
>> }
>>
> 
> We must just be talking at cross-purposes. The function you show above
> returns an error code, so there is no need for the caller to check clk
> if an error is returned. For the -ENODATA case, note that
> clk_get_by_name_nodev() sets clk->dev to NULL, thus ensuring that
> clk_valid() works as expected.

hmm, I pointed to the wrong function from clk-uclass. The below clk
function is similar to our reset usecase

struct clk *devm_clk_get_optional(struct udevice *dev, const char *id)
{
	struct clk *clk = devm_clk_get(dev, id);

	if (PTR_ERR(clk) == -ENODATA)
		return NULL;

	return clk;
}

For this case, clk_valid() works because it checks for "clk" pointer and
for ENODATA case (get_optional), the client will have clk = NULL and
pass NULL to all the clock APIs.

IIUC, you don't prefer returning NULL to client on ENODATA (or any other
error)? And that's why unlike devm_clk_get_optional() which returns NULL
on ENODATA, $patch directly returns devm_reset_control_get().

So for reset it could return
	1) valid reset_ctl pointer with dev initialized
	2) -ENOMEM (if allocation of reset_ctl itself fails)
	3) -ENODATA (if reset-names is not populated in DT)
	4) -ENOENT/-EINVAL (for other DT errors)

clk-uclass has a separate class of APIs that ends with _nodev(). Here
the client allocates memory for clk and let clk framework populate
clk->dev. For the other APIs where clk framework itself returns clk
pointer, there is no case where clk framework returns a valid clk
pointer but without clk->dev initialized (the second case is what we
deal in reset).

Since reset at this point doesn't deal with _nodev(), the latter case
above for clk is what should be considered (i.e reset returns valid
reset_ctrl pointer with dev initialized or return an error value).

So again coming back to the return values mentioned above [ 1) valid
reset, 2) -ENOMEM, 3) -ENODATA, 4) -ENOENT/-EINVAL ].

For get_optional()
  1) Should we return NULL on -ENODATA (similar to devm_clk_get_optional())?
  2) If we decide to return error value instead of NULL, the check would
simply move to client and reset_valid(). i.e The client would check
return value of get_optional() and if it's -ENODDATA, it's not going to
error out and will also pass -ENODATA to reset APIs. The reset APIs will
check again if it's ENODATA and handle it gracefully (to be done in
reset_valid()).
  3) We return NULL for optional() similar to devm_clk_get_optional()
and not consider the _nodev() APIs. Here reset_valid() will only check
if reset_ctrl is NULL or not.
> 
> But all of your patches are about a function that returns a pointer,
> which is a different thing....

Sorry about this. I pointed to the wrong function creating more confusion.
> 
>> I can add reset_valid() and add the conditional checks corresponding to
>> the changes made for reset (like return -ENODATA instead of 0).
> 
> I'm a bit lost at this point...will look at what you send.

I hope I've explained the differences between clk and reset above.
Please see and provide your feedback.

Thanks
Kishon