[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()
Pratyush Yadav
p.yadav at ti.com
Tue Jun 8 12:40:14 CEST 2021
Hi,
On 14/05/21 10:48AM, Kishon Vijay Abraham I wrote:
> 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 at 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 at 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 at 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 at 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.
I have been reading through this discussion, and there seems to be a lot
of confusion and lots of ideas floating around. I think the first thing
to decide is _what_ will the opional reset API do. Here is what I think
it should do. The point of the optional reset is that the driver does
not care if the reset is present or not. If the reset is present, then
do as it requests. If it is not present, do nothing. If the reset is
present and it cannot be obtained for some reason, tell the driver about
it.
With that in mind, we can decide on how the API should work. I see three
distinct classes come up: reset is successfully obtained, there as an
error, reset is not present. The first class can be represented by a
valid pointer. The second can be represented by ERR_PTR(). The third can
be represented by NULL. Making this separation will let the code neatly
take care of each class. Using ERR_PTR(-ENODATA) mixes up the "error"
and "not present" classes. if (!rst) is much cleaner than if
(PTR_ERR(rst) == -ENODATA).
I think the waters have been muddied because the "normal" reset APIs
require the caller to pass in the reset struct to fill, and the devm and
optional APIs allocate the struct themselves and return it. So the check
for whether a reset is valid changes based on which of the two APIs was
used to get the reset. Linux has been allocating a struct reset_control
and returning the pointer from the get go so they don't have this
problem. I think it is reasonable for U-Boot to update reset_valid() to
check for pointer validity to accomodate both API styles.
So here is my suggestion summarized: return NULL when PTR_ERR() ==
-ENODATA. In all other cases return the pointer as it is. Then in all
reset operations like reset_assert(), etc. check if the pointer passed
in is NULL. If it is, do nothing. If it is an error pointer, return
-EINVAL. Otherwise run the function like normal. reset_valid() should
also be updated check for IS_ERR_OR_NULL().
> >
> > 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
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
More information about the U-Boot
mailing list