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

Stephen Boyd
2024-08-28T00:19:06Z

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.