Re: [PATCH v1 1/1] clk: devres: Simplify devres handling functions

[Stephen Boyd <sboyd@kernel.org>]

Quoting Andy Shevchenko (2024-08-22 08:58:22)
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 90e6078fb6e1..f03d60706a85 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -28,15 +30,13 @@ static struct clk *__devm_clk_get(struct device *dev, const char *id,
>         struct clk *clk;
>         int ret;
>  
> -       state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
> +       state = devm_kmalloc(dev, sizeof(*state), GFP_KERNEL);

When is this allocation freed if the get() fails? When the calling
device driver detaches?

>         if (!state)
>                 return ERR_PTR(-ENOMEM);
>  
>         clk = get(dev, id);
> -       if (IS_ERR(clk)) {
> -               ret = PTR_ERR(clk);
> -               goto err_clk_get;
> -       }
> +       if (IS_ERR(clk))
> +               return clk;
>  
>         if (init) {
>                 ret = init(clk);
> @@ -47,16 +47,14 @@ static struct clk *__devm_clk_get(struct device *dev, const char *id,
>         state->clk = clk;
>         state->exit = exit;
>  
> -       devres_add(dev, state);
> +       ret = devm_add_action_or_reset(dev, devm_clk_release, state);
> +       if (ret)
> +               goto err_clk_init;

Shouldn't we return an error here? Otherwise we call clk_put() twice?

>  
>         return clk;
>  
>  err_clk_init:
> -
>         clk_put(clk);
> -err_clk_get:
> -
> -       devres_free(state);
>         return ERR_PTR(ret);
>  }
>  

I stopped reading, sorry! If you want to do this, please add a bunch of
KUnit tests.