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

[Sean Anderson]

On 3/1/22 9:58 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 27 Feb 2022 at 12:38, Sean Anderson <seanga2 at gmail.com> wrote:
>>
>> On 2/26/22 1:36 PM, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2 at 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 at 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 at 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 at 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 at 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?
>>
>> The ideal case would be if you wanted to allocate some per-struct-clk
>> data. Then, the correct place to free it would be rfree. But no one
>> does this, and if they did it would probably be better to free things
>> in remove.
>>
>> Actually... no one in clk, reset, or power-domain does anything with
>> rfree. So I am inclined to just remove it altogether.
> 
> Well, I suppose it is easy enough to add later, if needed. What does
> Linux use this for?

As far as I can tell, Linux doesn't have request/free. There's
of_clk_provider->get(), but that corresponds more to of_xlate.
Similarly, there's reset_controller_dev->of_xlate(), but no request/free.

--Sean