Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass <sjg@chromium.org>]

Hi Sean,

On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 2/1/22 10:59 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 1/27/22 4:35 PM, Simon Glass wrote:
> >>> Hi Sean,
> >>>
> >>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>
> >>>> On 1/27/22 10:05 AM, Simon Glass wrote:
> >>>>> Hi Sean,
> >>>>>
> >>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>
> >>>>>> When freeing a clock there is not much we can do if there is an error, and
> >>>>>> most callers do not actually check the return value. Even e.g. checking to
> >>>>>> make sure that clk->id is valid should have been done in request() in the
> >>>>>> first place (unless someone is messing with the driver behind our back).
> >>>>>> Just return void and don't bother returning an error.
> >>>>>>
> >>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >>>>>> ---
> >>>>>>
> >>>>>>     drivers/clk/clk-uclass.c  | 7 +++----
> >>>>>>     drivers/clk/clk_sandbox.c | 6 +++---
> >>>>>>     include/clk-uclass.h      | 8 +++-----
> >>>>>>     3 files changed, 9 insertions(+), 12 deletions(-)
> >>>>>>
> >>>>>
> >>>>> We have the same thing in other places too, but I am a little worried
> >>>>> about removing error checking. We try to avoid checking arguments too
> >>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
> >>>>> an invalid clk should be caught by a debug assertion rather than a
> >>>>> full check. But with driver model we have generally added an error
> >>>>> return to every uclass method, for consistency and to permit returning
> >>>>> error information if needed.
> >>>>>
> >>>>> Regards,
> >>>>> Simon
> >>>>>
> >>>>
> >>>> So there are a few reasons why I don't think a return value is useful
> >>>> here. To illustrate this, consider a typical user of the clock API:
> >>>>
> >>>>           struct clk a, b;
> >>>>
> >>>>           ret = clk_get_by_name(dev, "a", &a);
> >>>>           if (ret)
> >>>>                   return ret;
> >>>>
> >>>>           ret = clk_get_by_name(dev, "b", &b);
> >>>>           if (ret)
> >>>>                   goto free_a;
> >>>>
> >>>>           ret = clk_set_rate(&a, 5000000);
> >>>>           if (ret)
> >>>>                   goto free_b;
> >>>>
> >>>>           ret = clk_enable(&b);
> >>>>
> >>>> free_b:
> >>>>           clk_free(&b);
> >>>> free_a:
> >>>>           clk_free(&a);
> >>>>           return ret;
> >>>>
> >>>> - Because a and b are "thick pointers" they do not need any cleanup to
> >>>>      free their own resources. The only cleanup might be if the clock
> >>>>      driver has allocated something in clk_request (more on this below)
> >>>> - By the time we call clk_free, the mutable portions of the function
> >>>>      have already completed. In effect, the function has succeeded,
> >>>>      regardless of whether clk_free fails. Additionally, we cannot take any
> >>>>      action if it fails, since we still have to free both clocks.
> >>>> - clk_free occurs during the error path of the function. Even if it
> >>>>      errored, we do not want to override the existing error from one of the
> >>>>      functions doing "real" work.
> >>>>
> >>>> The last thing is that no clock driver actually does anything in rfree.
> >>>> The only driver with this function is the sandbox driver. I would like
> >>>> to remove the function altogether. As I understand it, the existing API
> >>>> is inspired by the reset drivers, so I would like to review its usage in
> >>>> the reset subsystem before removing it for the clock subsystem. I also
> >>>> want to make some changes to how rates and enables/disables are
> >>>> calculated which might provide a case for rfree. But once that is
> >>>> complete I think there will be no users still.
> >>>
> >>> What does this all look like in Linux?
> >>
> >> Their equivalent (clk_put) returns void, and generally so do most other
> >> cleanup functions, since .device_remove also returns void.
> >
> > We really cannot ignore errors from device_remove().
>
> Once you are at device_remove, all the users are gone and it's up to the
> device to clean up after itself. And often there is nothing we can do
> once remove is called. As you yourself say in device_remove,
>
>         /* We can't put the children back */

Well this assumes that device_remove() is actually removing the
device, not just disabling DMA, etc.

>
> Really the only sensible thing is to print an error and continue booting
> if possible.
>
> And of course no clock drivers actually use this function anyway, nor do
> (all but 5) users check it.
>
> > Anyway I think what you say about the 'thick pointer' makes sense. But
> > my expectation was that removing a clock might turn off a clock above
> > it in the tree, for example.
>
> No, this just frees resources (as is documented). If you want to turn
> off a clock, you have to call clk_disable. In fact, a very common use
> case is just like the example above, where the consmer frees the clock
> after enabling it.
>
> (This is also why clk->enable_count/rate are basically useless for
> anything other than CCF clocks)

How about a clock provided by an audio codec on an I2C bus? Should
clk_free() do anything in that case? I assume not. I think the
compelling part of your argument is that it is a  'think pointer' and
disable does nothing. So can you update clk_rfree() etc. to document
what is allowed to be done in that function?

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon