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

[Sean Anderson <seanga2@gmail.com>]

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.

--Sean