On Mon, Aug 27, 2018 at 02:28:45PM +0200, Bartosz Golaszewski wrote:

2018-08-27 12:39 GMT+02:00 Mike Rapoport [off-list ref]:

quoted

On Mon, Aug 27, 2018 at 10:21:01AM +0200, Bartosz Golaszewski wrote:

quoted

Use devm_kstrdup_const() in the pmc-atom driver. This mostly serves as
an example of how to use this new routine to shrink driver code.

While we're at it: replace a call to kcalloc() with devm_kcalloc().

Signed-off-by: Bartosz Golaszewski <redacted>
---
 drivers/clk/x86/clk-pmc-atom.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 08ef69945ffb..daa2192e6568 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c

@@ -253,14 +253,6 @@ static void plt_clk_unregister_fixed_rate_loop(struct clk_plt_data *data,
              plt_clk_unregister_fixed_rate(data->parents[i]);
 }

-static void plt_clk_free_parent_names_loop(const char **parent_names,
-                                        unsigned int i)
-{
-     while (i--)
-             kfree_const(parent_names[i]);
-     kfree(parent_names);
-}
-
 static void plt_clk_unregister_loop(struct clk_plt_data *data,
                                  unsigned int i)
 {

@@ -286,8 +278,8 @@ static const char **plt_clk_register_parents(struct platform_device *pdev,
      if (!data->parents)
              return ERR_PTR(-ENOMEM);

-     parent_names = kcalloc(nparents, sizeof(*parent_names),
-                            GFP_KERNEL);
+     parent_names = devm_kcalloc(&pdev->dev, nparents,
+                                 sizeof(*parent_names), GFP_KERNEL);
      if (!parent_names)
              return ERR_PTR(-ENOMEM);

@@ -300,7 +292,8 @@ static const char **plt_clk_register_parents(struct platform_device *pdev,
                      err = PTR_ERR(data->parents[i]);
                      goto err_unreg;
              }
-             parent_names[i] = kstrdup_const(clks[i].name, GFP_KERNEL);
+             parent_names[i] = devm_kstrdup_const(&pdev->dev,
+                                                  clks[i].name, GFP_KERNEL);
      }

      data->nparents = nparents;

@@ -308,7 +301,6 @@ static const char **plt_clk_register_parents(struct platform_device *pdev,

 err_unreg:
      plt_clk_unregister_fixed_rate_loop(data, i);
-     plt_clk_free_parent_names_loop(parent_names, i);

What happens if clks[i].name is not a part of RO data? The devm_kstrdup_const
will allocate memory and nothing will ever free it...

I'm looking at it and trying to see if I'm missing something, but
AFAIK the whole concept of devm_* is to leave out the resource
management part.

devm_kstrdup_const() will internally call devm_kstrdup() for strings
that are not in .rodata and once the device is detached, the string
will be freed (or not if it's in .rodata).

And when it's going to be freed, how the resource management will know
whether it's .rodata or not?
 

BR
Bart

quoted

And, please don't drop kfree(parent_names) here.

quoted

      return ERR_PTR(err);
 }

@@ -351,15 +343,12 @@ static int plt_clk_probe(struct platform_device *pdev)
              goto err_unreg_clk_plt;
      }

-     plt_clk_free_parent_names_loop(parent_names, data->nparents);
-
      platform_set_drvdata(pdev, data);
      return 0;

 err_unreg_clk_plt:
      plt_clk_unregister_loop(data, i);
      plt_clk_unregister_parents(data);
-     plt_clk_free_parent_names_loop(parent_names, data->nparents);

Ditto.

quoted

      return err;
 }

--
2.18.0

--
Sincerely yours,
Mike.

-- 
Sincerely yours,
Mike.