On Wed 29 Apr 2020 at 05:14, Bernard Zhao <bern...@vivo.com> wrote:
> In common init function, when run into err branch, we didn`t
> use kfree to release kzmalloc area, this may bring in memleak
Thx for reporting this Bernard.
I'm not a fan of adding kfree everywhere. I'd much prefer a label and
clear error exit path.
That being said, the allocation is probably not the only thing that
needs to be undone in case of error. I guess this is due to conversion
to CLK_OF_DECLARE_DRIVER() which forced to drop all the devm_
This was done because the clock controller was required early in the
boot sequence.
There is 2 paths to properly solve this:
1) Old school: manually undo everything with every error exit condition
Doable but probably a bit messy
2) Convert back the driver to a real platform driver and use devm_.
We would still need the controller to register early but I wonder if
we could use the same method as drivers/clk/mediatek/clk-mt2701.c and
use arch_initcall() ?
Martin, you did the initial conversion, what do you think of option 2 ?
Would it still answer the problem you were trying to solve back then ?
One added benefit of option 2 is we could drop CLK_OF_DECLARE_DRIVER().
We could even do the same in for the other SoCs, which I suppose would
avoid a fair amount of probe deferral.
>
> Signed-off-by: Bernard Zhao <bern...@vivo.com>
> ---
> drivers/clk/meson/meson8b.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index 34a70c4b4899..0f07d5a4cd16 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -3687,6 +3687,7 @@ static void __init meson8b_clkc_init_common(struct
> device_node *np,
> if (ret) {
> pr_err("%s: Failed to register clkc reset controller: %d\n",
> __func__, ret);
> + kfree(rstc);
> return;
> }
>
> @@ -3710,8 +3711,10 @@ static void __init meson8b_clkc_init_common(struct
> device_node *np,
> continue;
>
> ret = of_clk_hw_register(np, clk_hw_onecell_data->hws[i]);
> - if (ret)
> + if (ret) {
> + kfree(rstc);
> return;
> + }
> }
>
> meson8b_cpu_nb_data.cpu_clk = clk_hw_onecell_data->hws[CLKID_CPUCLK];
> @@ -3727,13 +3730,16 @@ static void __init meson8b_clkc_init_common(struct
> device_node *np,
> if (ret) {
> pr_err("%s: failed to register the CPU clock notifier\n",
> __func__);
> + kfree(rstc);
> return;
> }
>
> ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
> clk_hw_onecell_data);
> - if (ret)
> + if (ret) {
> pr_err("%s: failed to register clock provider\n", __func__);
> + kfree(rstc);
> + }
> }
>
> static void __init meson8_clkc_init(struct device_node *np)
