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

[Simon Glass]

Hi Pratyush,

On Tue, 8 Jun 2021 at 04:40, Pratyush Yadav <p.yadav at ti.com> wrote:
>
> 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().

Thank you for the effort and explanation. This helps a lot.

In driver model U-Boot uses a separate arg for functions that return a
pointer. The return value is an int and indicates an error. This is a
design decision that was made early on. It is slightly less efficient,
although checking for (x >= 0xffff0000) is not free either. It avoids
the PTR_ERR() and ERR_PTR() stuff so the code is more readable. It
allows use of addresses at the top of address space, although this is
seldom useful. It also ensures that NULL is not a valid pointer, which
is likely appreciated by coverity and the like. Making the caller pass
in the pointer avoids malloc() which is also a design decision in
U-Boot. But mostly it is about readability.

That said, we aim for compatibility with Linux APIs, and devm is
imported from linux. So U-Boot's devm also uses the error-return
thing.

In the interests of linux compatibility I think it is somewhat
defensible for reset_valid() to check for a NULL pointer. However I
don't like returning errors from these functions. There is no way in
reset_valid() to know if the thing passed in came from a devm function
or a normal reset function. So if we check for IS_ERR() in
reset_valid() then we are forcing the linux convention onto the normal
reset calls.

To me a better approach is to return a valid 'struct reset_ctl'
pointer, but one where reset_valid() will return false (i.e. ->dev is
NULL). We can have one of these in the devm code, perhaps static, and
return the same one in all cases where there is a failure.

Do you need to get the error code from devm functions? Presumably if
it is -ENOENT then it means there is no reset but it is OK. Any other
error indicates something has gone wrong.

If you don't need the error code, then the above is all we need. If
you do need the error code, then I wonder whether we need something
that converts a struct reset_ctl (as returned by a devm function) into
an error and a reset_ctl? Then we can call this to interpret the
result from the devm function.

e.g.

static reset_ctl devm_no_reset;

int devm_to_reset(struct reset_ctl *in, struct reset_ctl **outp)
{
   if (IS_ERR_OR_NULL(in)) {
      *outp = &devm_no_reset;
      if (!in)
         return -ENOENT;
      else
         return PTR_ERR(in);
   }
   *outp = in;

   return 0;
}

The above (ugly) logic is what is being carried around in that pointer
variable, since it has three types of value:

- valid reset
- invalid reset but that's OK
- invalid reset due to an error

So sample code would change from:

   struct reset_ctl *reset;
   int ret;

   reset = devm_reset_control_get(dev, "myreset");
   if (IS_ERR(reset))
      return log_msg_ret("reset", -ENOENT);

to:

   struct reset_ctl *rerr, *reset;
   int ret;

   rerr = devm_reset_control_get(dev, "myreset");
   ret = devm_to_reset(rerr, &reset);
   if (ret)
      return log_msg_ret("reset", ret);

I think the above helps join the linux and U-Boot APIs without too
much pain. I will note that there is no use of the devm_reset_... API
in U-Boot, apart from in tests, so this cannot be described as an
critical feature.

What do you think?

Regards,
Simon


>
> > >
> > > 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.