Re: [PATCH] clk: st: Fix error paths and allocation style

[Gabriel Fernandez]

Hi Stephen,

You can add my
Acked-by: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxx>

Thanks

Gabriel.

On 8 July 2015 at 03:50, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> The error paths in this file leak memory and mappings and test
> for pointers being valid after dereferencing them. Fix these
> problems and properly free resources on errors. Fix some
> stylistic things too like using sizeof(*ptr) and fitting more
> code on a single line. Note that we don't unregister clocks here.
> That needs a clk_composite_unregister() API that we don't have
> right now.
>
> Cc: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxx>
> Cc: Pankaj Dev <pankaj.dev@xxxxxx>
> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> ---
>  drivers/clk/st/clkgen-mux.c | 83 ++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/clk/st/clkgen-mux.c b/drivers/clk/st/clkgen-mux.c
> index 717c4a91a17b..ab9298395264 100644
> --- a/drivers/clk/st/clkgen-mux.c
> +++ b/drivers/clk/st/clkgen-mux.c
> @@ -30,7 +30,7 @@ static const char ** __init clkgen_mux_get_parents(struct device_node *np,
>         if (WARN_ON(nparents <= 0))
>                 return ERR_PTR(-EINVAL);
>
> -       parents = kzalloc(nparents * sizeof(const char *), GFP_KERNEL);
> +       parents = kcalloc(nparents, sizeof(const char *), GFP_KERNEL);
>         if (!parents)
>                 return ERR_PTR(-ENOMEM);
>
> @@ -215,7 +215,7 @@ static const struct clk_ops clkgena_divmux_ops = {
>  /**
>   * clk_register_genamux - register a genamux clock with the clock framework
>   */
> -static struct clk *clk_register_genamux(const char *name,
> +static struct clk * __init clk_register_genamux(const char *name,
>                                 const char **parent_names, u8 num_parents,
>                                 void __iomem *reg,
>                                 const struct clkgena_divmux_data *muxdata,
> @@ -369,11 +369,10 @@ static const struct of_device_id clkgena_divmux_of_match[] = {
>         {}
>  };
>
> -static void __iomem * __init clkgen_get_register_base(
> -                               struct device_node *np)
> +static void __iomem * __init clkgen_get_register_base(struct device_node *np)
>  {
>         struct device_node *pnode;
> -       void __iomem *reg = NULL;
> +       void __iomem *reg;
>
>         pnode = of_get_parent(np);
>         if (!pnode)
> @@ -398,7 +397,7 @@ static void __init st_of_clkgena_divmux_setup(struct device_node *np)
>         if (WARN_ON(!match))
>                 return;
>
> -       data = (struct clkgena_divmux_data *)match->data;
> +       data = match->data;
>
>         reg = clkgen_get_register_base(np);
>         if (!reg)
> @@ -406,18 +405,18 @@ static void __init st_of_clkgena_divmux_setup(struct device_node *np)
>
>         parents = clkgen_mux_get_parents(np, &num_parents);
>         if (IS_ERR(parents))
> -               return;
> +               goto err_parents;
>
>         clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>         if (!clk_data)
> -               goto err;
> +               goto err_alloc;
>
>         clk_data->clk_num = data->num_outputs;
> -       clk_data->clks = kzalloc(clk_data->clk_num * sizeof(struct clk *),
> +       clk_data->clks = kcalloc(clk_data->clk_num, sizeof(struct clk *),
>                                  GFP_KERNEL);
>
>         if (!clk_data->clks)
> -               goto err;
> +               goto err_alloc_clks;
>
>         for (i = 0; i < clk_data->clk_num; i++) {
>                 struct clk *clk;
> @@ -447,11 +446,13 @@ static void __init st_of_clkgena_divmux_setup(struct device_node *np)
>         of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
>         return;
>  err:
> -       if (clk_data)
> -               kfree(clk_data->clks);
> -
> +       kfree(clk_data->clks);
> +err_alloc_clks:
>         kfree(clk_data);
> +err_alloc:
>         kfree(parents);
> +err_parents:
> +       iounmap(reg);
>  }
>  CLK_OF_DECLARE(clkgenadivmux, "st,clkgena-divmux", st_of_clkgena_divmux_setup);
>
> @@ -491,7 +492,7 @@ static void __init st_of_clkgena_prediv_setup(struct device_node *np)
>         void __iomem *reg;
>         const char *parent_name, *clk_name;
>         struct clk *clk;
> -       struct clkgena_prediv_data *data;
> +       const struct clkgena_prediv_data *data;
>
>         match = of_match_node(clkgena_prediv_of_match, np);
>         if (!match) {
> @@ -499,7 +500,7 @@ static void __init st_of_clkgena_prediv_setup(struct device_node *np)
>                 return;
>         }
>
> -       data = (struct clkgena_prediv_data *)match->data;
> +       data = match->data;
>
>         reg = clkgen_get_register_base(np);
>         if (!reg)
> @@ -507,18 +508,18 @@ static void __init st_of_clkgena_prediv_setup(struct device_node *np)
>
>         parent_name = of_clk_get_parent_name(np, 0);
>         if (!parent_name)
> -               return;
> +               goto err;
>
>         if (of_property_read_string_index(np, "clock-output-names",
>                                           0, &clk_name))
> -               return;
> +               goto err;
>
>         clk = clk_register_divider_table(NULL, clk_name, parent_name,
>                                          CLK_GET_RATE_NOCACHE,
>                                          reg + data->offset, data->shift, 1,
>                                          0, data->table, NULL);
>         if (IS_ERR(clk))
> -               return;
> +               goto err;
>
>         of_clk_add_provider(np, of_clk_src_simple_get, clk);
>         pr_debug("%s: parent %s rate %u\n",
> @@ -527,6 +528,8 @@ static void __init st_of_clkgena_prediv_setup(struct device_node *np)
>                 (unsigned int)clk_get_rate(clk));
>
>         return;
> +err:
> +       iounmap(reg);
>  }
>  CLK_OF_DECLARE(clkgenaprediv, "st,clkgena-prediv", st_of_clkgena_prediv_setup);
>
> @@ -630,7 +633,7 @@ static void __init st_of_clkgen_mux_setup(struct device_node *np)
>         void __iomem *reg;
>         const char **parents;
>         int num_parents;
> -       struct clkgen_mux_data *data;
> +       const struct clkgen_mux_data *data;
>
>         match = of_match_node(mux_of_match, np);
>         if (!match) {
> @@ -638,7 +641,7 @@ static void __init st_of_clkgen_mux_setup(struct device_node *np)
>                 return;
>         }
>
> -       data = (struct clkgen_mux_data *)match->data;
> +       data = match->data;
>
>         reg = of_iomap(np, 0);
>         if (!reg) {
> @@ -650,7 +653,7 @@ static void __init st_of_clkgen_mux_setup(struct device_node *np)
>         if (IS_ERR(parents)) {
>                 pr_err("%s: Failed to get parents (%ld)\n",
>                                 __func__, PTR_ERR(parents));
> -               return;
> +               goto err_parents;
>         }
>
>         clk = clk_register_mux(NULL, np->name, parents, num_parents,
> @@ -666,12 +669,14 @@ static void __init st_of_clkgen_mux_setup(struct device_node *np)
>                         __clk_get_name(clk_get_parent(clk)),
>                         (unsigned int)clk_get_rate(clk));
>
> +       kfree(parents);
>         of_clk_add_provider(np, of_clk_src_simple_get, clk);
> +       return;
>
>  err:
>         kfree(parents);
> -
> -       return;
> +err_parents:
> +       iounmap(reg);
>  }
>  CLK_OF_DECLARE(clkgen_mux, "st,clkgen-mux", st_of_clkgen_mux_setup);
>
> @@ -707,12 +712,12 @@ static void __init st_of_clkgen_vcc_setup(struct device_node *np)
>         const char **parents;
>         int num_parents, i;
>         struct clk_onecell_data *clk_data;
> -       struct clkgen_vcc_data *data;
> +       const struct clkgen_vcc_data *data;
>
>         match = of_match_node(vcc_of_match, np);
>         if (WARN_ON(!match))
>                 return;
> -       data = (struct clkgen_vcc_data *)match->data;
> +       data = match->data;
>
>         reg = of_iomap(np, 0);
>         if (!reg)
> @@ -720,18 +725,18 @@ static void __init st_of_clkgen_vcc_setup(struct device_node *np)
>
>         parents = clkgen_mux_get_parents(np, &num_parents);
>         if (IS_ERR(parents))
> -               return;
> +               goto err_parents;
>
>         clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>         if (!clk_data)
> -               goto err;
> +               goto err_alloc;
>
>         clk_data->clk_num = VCC_MAX_CHANNELS;
> -       clk_data->clks = kzalloc(clk_data->clk_num * sizeof(struct clk *),
> +       clk_data->clks = kcalloc(clk_data->clk_num, sizeof(struct clk *),
>                                  GFP_KERNEL);
>
>         if (!clk_data->clks)
> -               goto err;
> +               goto err_alloc_clks;
>
>         for (i = 0; i < clk_data->clk_num; i++) {
>                 struct clk *clk;
> @@ -750,21 +755,21 @@ static void __init st_of_clkgen_vcc_setup(struct device_node *np)
>                 if (*clk_name == '\0')
>                         continue;
>
> -               gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> +               gate = kzalloc(sizeof(*gate), GFP_KERNEL);
>                 if (!gate)
> -                       break;
> +                       goto err;
>
> -               div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
> +               div = kzalloc(sizeof(*div), GFP_KERNEL);
>                 if (!div) {
>                         kfree(gate);
> -                       break;
> +                       goto err;
>                 }
>
> -               mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
> +               mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>                 if (!mux) {
>                         kfree(gate);
>                         kfree(div);
> -                       break;
> +                       goto err;
>                 }
>
>                 gate->reg = reg + VCC_GATE_OFFSET;
> @@ -823,10 +828,12 @@ err:
>                 kfree(container_of(composite->mux_hw, struct clk_mux, hw));
>         }
>
> -       if (clk_data)
> -               kfree(clk_data->clks);
> -
> +       kfree(clk_data->clks);
> +err_alloc_clks:
>         kfree(clk_data);
> +err_alloc:
>         kfree(parents);
> +err_parents:
> +       iounmap(reg);
>  }
>  CLK_OF_DECLARE(clkgen_vcc, "st,clkgen-vcc", st_of_clkgen_vcc_setup);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/