diff mbox series

[v2] clk: ti: dra7-atl: don't allocate `parent_names' variable

Message ID 20221018160352.1591428-1-dario.binacchi@amarulasolutions.com
State New
Headers
Series [v2] clk: ti: dra7-atl: don't allocate `parent_names' variable |

Commit Message

Dario Binacchi Oct. 18, 2022, 4:03 p.m. UTC 
The `parent_names' variable was freed also in case of kzalloc() error.
Instead of modifying the code to perform a proper memory release, I
decided to fix the bug by not allocating memory.
Since only one parent name is referenced, it is not necessary to
allocate this variable at runtime and therefore you can avoid calling
the kzalloc() function. This simplifies the code (even calls to kfree
can be removed) and improves the performance of the routine.

Note: Although no operation is performed by kfree() on a NULL pointer,
it was however suboptimal and semantically wrong doing it.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Reported-by: kernel test robot <lkp@intel.com>

---

Changes in v2:
- Fix compiling error
- Add kernel test robot's Reported-by tag.

 drivers/clk/ti/clk-dra7-atl.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Comments

Stephen Boyd Oct. 28, 2022, 12:27 a.m. UTC | #1 
Quoting Dario Binacchi (2022-10-18 09:03:52)
> diff --git a/drivers/clk/ti/clk-dra7-atl.c b/drivers/clk/ti/clk-dra7-atl.c
> index ff4d6a951681..78482d1a4a33 100644
> --- a/drivers/clk/ti/clk-dra7-atl.c
> +++ b/drivers/clk/ti/clk-dra7-atl.c
> @@ -188,24 +188,17 @@ static void __init of_dra7_atl_clock_setup(struct device_node *node)
>                 goto cleanup;
>         }
>  
> -       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> -
> -       if (!parent_names)
> -               goto cleanup;
> -
>         parent_names[0] = of_clk_get_parent_name(node, 0);

Can you use struct clk_parent_data instead and assign index to 0? Then
we don't even need to use of_clk_get_parent_name() here.
Dario Binacchi Oct. 30, 2022, 1 p.m. UTC | #2 
Hi Stephen,

On Fri, Oct 28, 2022 at 2:27 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dario Binacchi (2022-10-18 09:03:52)
> > diff --git a/drivers/clk/ti/clk-dra7-atl.c b/drivers/clk/ti/clk-dra7-atl.c
> > index ff4d6a951681..78482d1a4a33 100644
> > --- a/drivers/clk/ti/clk-dra7-atl.c
> > +++ b/drivers/clk/ti/clk-dra7-atl.c
> > @@ -188,24 +188,17 @@ static void __init of_dra7_atl_clock_setup(struct device_node *node)
> >                 goto cleanup;
> >         }
> >
> > -       parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> > -
> > -       if (!parent_names)
> > -               goto cleanup;
> > -
> >         parent_names[0] = of_clk_get_parent_name(node, 0);
>
> Can you use struct clk_parent_data instead and assign index to 0? Then
> we don't even need to use of_clk_get_parent_name() here.

I tried to test your suggestions on another platform (I don't have the
hw to test the driver change) but if I
don't add pdata.name = of_clk_get_parent_name () the board boot up fails.

As far I can see from the clk_core_populate_parent_map()

....
/* Copy everything over because it might be __initdata */
for (i = 0, parent = parents; i < num_parents; i++, parent++) {
    parent->index = -1;
    if (parent_names) {
        /* throw a WARN if any entries are NULL */
       WARN(!parent_names[i],
            "%s: invalid NULL in %s's .parent_names\n",
            __func__, core->name);
        ret = clk_cpy_name(&parent->name, parent_names[i],
                                        true);
    } else if (parent_data) {
        parent->hw = parent_data[i].hw;
        parent->index = parent_data[i].index;
        ret = clk_cpy_name(&parent->fw_name,
                                         parent_data[i].fw_name, false);
        if (!ret)
            ret = clk_cpy_name(&parent->name,
                                            parent_data[i].name,
                                            false);
...


The function clk_cpy_name() is called with the parameter "mus_exist"
to true in the path "parent_names" and false
in the path "parent_data". Therefore, in the path "parent_data" it is
allowed that parent-> name is not set.
In doing so, therefore, the change would not even be backward compatible.

So, IMHO, there are 2 possible options:
 1 okay to use parent_data, but we keep using of_clk_get_parent_name
() to set parent_data::name.
 2 okay to use the version v2 of the patch.

What do you think?

Thanks and regards,
Dario
Stephen Boyd Nov. 1, 2022, 6:35 p.m. UTC | #3 
Quoting Dario Binacchi (2022-10-30 06:00:46)
> 
> I tried to test your suggestions on another platform (I don't have the
> hw to test the driver change) but if I
> don't add pdata.name = of_clk_get_parent_name () the board boot up fails.
> 
> As far I can see from the clk_core_populate_parent_map()
> 
> ....
> /* Copy everything over because it might be __initdata */
> for (i = 0, parent = parents; i < num_parents; i++, parent++) {
>     parent->index = -1;
>     if (parent_names) {
>         /* throw a WARN if any entries are NULL */
>        WARN(!parent_names[i],
>             "%s: invalid NULL in %s's .parent_names\n",
>             __func__, core->name);
>         ret = clk_cpy_name(&parent->name, parent_names[i],
>                                         true);
>     } else if (parent_data) {
>         parent->hw = parent_data[i].hw;
>         parent->index = parent_data[i].index;
>         ret = clk_cpy_name(&parent->fw_name,
>                                          parent_data[i].fw_name, false);
>         if (!ret)
>             ret = clk_cpy_name(&parent->name,
>                                             parent_data[i].name,
>                                             false);
> ...
> 
> 
> The function clk_cpy_name() is called with the parameter "mus_exist"
> to true in the path "parent_names" and false
> in the path "parent_data". Therefore, in the path "parent_data" it is
> allowed that parent-> name is not set.
> In doing so, therefore, the change would not even be backward compatible.
> 
> So, IMHO, there are 2 possible options:
>  1 okay to use parent_data, but we keep using of_clk_get_parent_name
> () to set parent_data::name.
>  2 okay to use the version v2 of the patch.
> 
> What do you think?

I am confused.

The struct clk_parent_data::name being used is whatever string is
returned by of_clk_get_parent_name(node, 0). That is the same as setting
struct clk_parent_data::index to 0, and not assigning the 'name' or
'fw_name' field of the parent data structure. This is a compatible
change because of_clk_get_parent_name() is getting the name of the clk
in 'clocks' for 'node' at index 0. Using the index 0 in clk_parent_data
tells clk framework that the parent of the clk being registered is the
clk in 'clocks' for the 'dev->node' that is passed in during
clk_register(). If you don't have a device pointer, use
of_clk_hw_register() to pass 'node' directly. It looks like you will
have to do that in this case to get the node pointer registered with
this clk.
diff mbox series

Patch

diff --git a/drivers/clk/ti/clk-dra7-atl.c b/drivers/clk/ti/clk-dra7-atl.c
index ff4d6a951681..78482d1a4a33 100644
--- a/drivers/clk/ti/clk-dra7-atl.c
+++ b/drivers/clk/ti/clk-dra7-atl.c
@@ -164,7 +164,7 @@  static void __init of_dra7_atl_clock_setup(struct device_node *node)
 {
 	struct dra7_atl_desc *clk_hw = NULL;
 	struct clk_init_data init = { NULL };
-	const char **parent_names = NULL;
+	const char *parent_names[1];
 	const char *name;
 	struct clk *clk;
 
@@ -188,24 +188,17 @@  static void __init of_dra7_atl_clock_setup(struct device_node *node)
 		goto cleanup;
 	}
 
-	parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
-
-	if (!parent_names)
-		goto cleanup;
-
 	parent_names[0] = of_clk_get_parent_name(node, 0);
 
 	init.parent_names = parent_names;
 
 	clk = ti_clk_register(NULL, &clk_hw->hw, name);
-
 	if (!IS_ERR(clk)) {
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
-		kfree(parent_names);
 		return;
 	}
+
 cleanup:
-	kfree(parent_names);
 	kfree(clk_hw);
 }
 CLK_OF_DECLARE(dra7_atl_clock, "ti,dra7-atl-clock", of_dra7_atl_clock_setup);