[PATCH v3 0/4] devres: provide and use devm_kstrdup_const()

[PATCH v3 0/4] devres: provide and use devm_kstrdup_const()

[Bartosz Golaszewski]

This series implements devm_kstrdup_const() together with some
prerequisite changes and uses it in pmc-atom driver.

v1 -> v2:
- fixed the changelog in the patch implementing devm_kstrdup_const()
- fixed the kernel doc
- moved is_kernel_rodata() to asm-generic/sections.h
- fixed constness

v2 -> v3:
- rebased on top of 4.19-rc5 as there were some conflicts in the
  pmc-atom driver
- collected Reviewed-by tags

Bartosz Golaszewski (4):
  devres: constify p in devm_kfree()
  mm: move is_kernel_rodata() to asm-generic/sections.h
  devres: provide devm_kstrdup_const()
  clk: pmc-atom: use devm_kstrdup_const()

 drivers/base/devres.c          | 43 ++++++++++++++++++++++++++++++++--
 drivers/clk/x86/clk-pmc-atom.c | 19 ++++-----------
 include/asm-generic/sections.h | 14 +++++++++++
 include/linux/device.h         |  5 +++-
 mm/util.c                      |  7 ------
 5 files changed, 63 insertions(+), 25 deletions(-)

-- 
2.18.0

[PATCH v3 1/4] devres: constify p in devm_kfree()

[Bartosz Golaszewski]

Make devm_kfree() signature uniform with that of kfree(). To avoid
compiler warnings: cast p to (void *) when calling devres_destroy().

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/base/devres.c  | 5 +++--
 include/linux/device.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index f98a097e73f2..438c91a43508 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -885,11 +885,12 @@ EXPORT_SYMBOL_GPL(devm_kasprintf);
  *
  * Free memory allocated with devm_kmalloc().
  */
-void devm_kfree(struct device *dev, void *p)
+void devm_kfree(struct device *dev, const void *p)
 {
 	int rc;
 
-	rc = devres_destroy(dev, devm_kmalloc_release, devm_kmalloc_match, p);
+	rc = devres_destroy(dev, devm_kmalloc_release,
+			    devm_kmalloc_match, (void *)p);
 	WARN_ON(rc);
 }
 EXPORT_SYMBOL_GPL(devm_kfree);
diff --git a/include/linux/device.h b/include/linux/device.h
index 8f882549edee..33f7cb271fbb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -692,7 +692,7 @@ static inline void *devm_kcalloc(struct device *dev,
 {
 	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
 }
-extern void devm_kfree(struct device *dev, void *p);
+extern void devm_kfree(struct device *dev, const void *p);
 extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
 extern void *devm_kmemdup(struct device *dev, const void *src, size_t len,
 			  gfp_t gfp);
-- 
2.18.0

[PATCH v3 2/4] mm: move is_kernel_rodata() to asm-generic/sections.h

[Bartosz Golaszewski]

Export this routine so that we can use it later in devm_kstrdup_const()
and devm_kfree_const().

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 include/asm-generic/sections.h | 14 ++++++++++++++
 mm/util.c                      |  7 -------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 849cd8eb5ca0..d79abca81a52 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -141,4 +141,18 @@ static inline bool init_section_intersects(void *virt, size_t size)
 	return memory_intersects(__init_begin, __init_end, virt, size);
 }
 
+/**
+ * is_kernel_rodata - checks if the pointer address is located in the
+ *                    .rodata section
+ *
+ * @addr: address to check
+ *
+ * Returns: true if the address is located in .rodata, false otherwise.
+ */
+static inline bool is_kernel_rodata(unsigned long addr)
+{
+	return addr >= (unsigned long)__start_rodata &&
+	       addr < (unsigned long)__end_rodata;
+}
+
 #endif /* _ASM_GENERIC_SECTIONS_H_ */
diff --git a/mm/util.c b/mm/util.c
index 9e3ebd2ef65f..470f5cd80b64 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -15,17 +15,10 @@
 #include <linux/vmalloc.h>
 #include <linux/userfaultfd_k.h>
 
-#include <asm/sections.h>
 #include <linux/uaccess.h>
 
 #include "internal.h"
 
-static inline int is_kernel_rodata(unsigned long addr)
-{
-	return addr >= (unsigned long)__start_rodata &&
-		addr < (unsigned long)__end_rodata;
-}
-
 /**
  * kfree_const - conditionally free memory
  * @x: pointer to the memory
-- 
2.18.0

[PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Bartosz Golaszewski]

Provide a resource managed version of kstrdup_const(). This variant
internally calls devm_kstrdup() on pointers that are outside of
.rodata section and returns the string as is otherwise.

Also provide a corresponding version of devm_kfree().

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/base/devres.c  | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 438c91a43508..48185d57bc5b 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -11,6 +11,8 @@
 #include <linux/slab.h>
 #include <linux/percpu.h>
 
+#include <asm/sections.h>
+
 #include "base.h"
 
 struct devres_node {
@@ -822,6 +824,28 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(devm_kstrdup);
 
+/**
+ * devm_kstrdup_const - resource managed conditional string duplication
+ * @dev: device for which to duplicate the string
+ * @s: the string to duplicate
+ * @gfp: the GFP mask used in the kmalloc() call when allocating memory
+ *
+ * Strings allocated by devm_kstrdup_const will be automatically freed when
+ * the associated device is detached.
+ *
+ * RETURNS:
+ * Source string if it is in .rodata section otherwise it falls back to
+ * devm_kstrdup.
+ */
+const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp)
+{
+	if (is_kernel_rodata((unsigned long)s))
+		return s;
+
+	return devm_kstrdup(dev, s, gfp);
+}
+EXPORT_SYMBOL(devm_kstrdup_const);
+
 /**
  * devm_kvasprintf - Allocate resource managed space and format a string
  *		     into that.
@@ -895,6 +919,20 @@ void devm_kfree(struct device *dev, const void *p)
 }
 EXPORT_SYMBOL_GPL(devm_kfree);
 
+/**
+ * devm_kfree_const - Resource managed conditional kfree
+ * @dev: device this memory belongs to
+ * @p: memory to free
+ *
+ * Function calls devm_kfree only if @p is not in .rodata section.
+ */
+void devm_kfree_const(struct device *dev, const void *p)
+{
+	if (!is_kernel_rodata((unsigned long)p))
+		devm_kfree(dev, p);
+}
+EXPORT_SYMBOL(devm_kfree_const);
+
 /**
  * devm_kmemdup - Resource-managed kmemdup
  * @dev: Device this memory belongs to
diff --git a/include/linux/device.h b/include/linux/device.h
index 33f7cb271fbb..79ccc6eb0975 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,
 	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
 }
 extern void devm_kfree(struct device *dev, const void *p);
+extern void devm_kfree_const(struct device *dev, const void *p);
 extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
+extern const char *devm_kstrdup_const(struct device *dev,
+				      const char *s, gfp_t gfp);
 extern void *devm_kmemdup(struct device *dev, const void *src, size_t len,
 			  gfp_t gfp);
 
-- 
2.18.0

[PATCH v3 4/4] clk: pmc-atom: use devm_kstrdup_const()

[Bartosz Golaszewski]

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 <brgl@bgdev.pl>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 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 d977193842df..239197799ea3 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -247,14 +247,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)
 {
@@ -280,8 +272,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);
 
@@ -294,7 +286,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;
@@ -302,7 +295,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);
 	return ERR_PTR(err);
 }
 
@@ -352,8 +344,6 @@ static int plt_clk_probe(struct platform_device *pdev)
 		goto err_drop_mclk;
 	}
 
-	plt_clk_free_parent_names_loop(parent_names, data->nparents);
-
 	platform_set_drvdata(pdev, data);
 	return 0;
 
@@ -362,7 +352,6 @@ static int plt_clk_probe(struct platform_device *pdev)
 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);
 	return err;
 }
 
-- 
2.18.0

Re: [PATCH v3 2/4] mm: move is_kernel_rodata() to asm-generic/sections.h

[Mike Rapoport]

On Mon, Sep 24, 2018 at 12:11:48PM +0200, Bartosz Golaszewski wrote:
> Export this routine so that we can use it later in devm_kstrdup_const()
> and devm_kfree_const().
> 
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Acked-by: Mike Rapoport <rppt@linux.vnet.ibm.com>

> ---
>  include/asm-generic/sections.h | 14 ++++++++++++++
>  mm/util.c                      |  7 -------
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 849cd8eb5ca0..d79abca81a52 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -141,4 +141,18 @@ static inline bool init_section_intersects(void *virt, size_t size)
>  	return memory_intersects(__init_begin, __init_end, virt, size);
>  }
> 
> +/**
> + * is_kernel_rodata - checks if the pointer address is located in the
> + *                    .rodata section
> + *
> + * @addr: address to check
> + *
> + * Returns: true if the address is located in .rodata, false otherwise.
> + */
> +static inline bool is_kernel_rodata(unsigned long addr)
> +{
> +	return addr >= (unsigned long)__start_rodata &&
> +	       addr < (unsigned long)__end_rodata;
> +}
> +
>  #endif /* _ASM_GENERIC_SECTIONS_H_ */
> diff --git a/mm/util.c b/mm/util.c
> index 9e3ebd2ef65f..470f5cd80b64 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -15,17 +15,10 @@
>  #include <linux/vmalloc.h>
>  #include <linux/userfaultfd_k.h>
> 
> -#include <asm/sections.h>
>  #include <linux/uaccess.h>
> 
>  #include "internal.h"
> 
> -static inline int is_kernel_rodata(unsigned long addr)
> -{
> -	return addr >= (unsigned long)__start_rodata &&
> -		addr < (unsigned long)__end_rodata;
> -}
> -
>  /**
>   * kfree_const - conditionally free memory
>   * @x: pointer to the memory
> -- 
> 2.18.0
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Mike Rapoport]

On Mon, Sep 24, 2018 at 12:11:49PM +0200, Bartosz Golaszewski wrote:
> Provide a resource managed version of kstrdup_const(). This variant
> internally calls devm_kstrdup() on pointers that are outside of
> .rodata section and returns the string as is otherwise.
> 
> Also provide a corresponding version of devm_kfree().
> 
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Acked-by: Mike Rapoport <rppt@linux.vnet.ibm.com>

> ---
>  drivers/base/devres.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  3 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 438c91a43508..48185d57bc5b 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/percpu.h>
> 
> +#include <asm/sections.h>
> +
>  #include "base.h"
> 
>  struct devres_node {
> @@ -822,6 +824,28 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
>  }
>  EXPORT_SYMBOL_GPL(devm_kstrdup);
> 
> +/**
> + * devm_kstrdup_const - resource managed conditional string duplication
> + * @dev: device for which to duplicate the string
> + * @s: the string to duplicate
> + * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> + *
> + * Strings allocated by devm_kstrdup_const will be automatically freed when
> + * the associated device is detached.
> + *
> + * RETURNS:
> + * Source string if it is in .rodata section otherwise it falls back to
> + * devm_kstrdup.
> + */
> +const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp)
> +{
> +	if (is_kernel_rodata((unsigned long)s))
> +		return s;
> +
> +	return devm_kstrdup(dev, s, gfp);
> +}
> +EXPORT_SYMBOL(devm_kstrdup_const);
> +
>  /**
>   * devm_kvasprintf - Allocate resource managed space and format a string
>   *		     into that.
> @@ -895,6 +919,20 @@ void devm_kfree(struct device *dev, const void *p)
>  }
>  EXPORT_SYMBOL_GPL(devm_kfree);
> 
> +/**
> + * devm_kfree_const - Resource managed conditional kfree
> + * @dev: device this memory belongs to
> + * @p: memory to free
> + *
> + * Function calls devm_kfree only if @p is not in .rodata section.
> + */
> +void devm_kfree_const(struct device *dev, const void *p)
> +{
> +	if (!is_kernel_rodata((unsigned long)p))
> +		devm_kfree(dev, p);
> +}
> +EXPORT_SYMBOL(devm_kfree_const);
> +
>  /**
>   * devm_kmemdup - Resource-managed kmemdup
>   * @dev: Device this memory belongs to
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 33f7cb271fbb..79ccc6eb0975 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,
>  	return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
>  }
>  extern void devm_kfree(struct device *dev, const void *p);
> +extern void devm_kfree_const(struct device *dev, const void *p);
>  extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
> +extern const char *devm_kstrdup_const(struct device *dev,
> +				      const char *s, gfp_t gfp);
>  extern void *devm_kmemdup(struct device *dev, const void *src, size_t len,
>  			  gfp_t gfp);
> 
> -- 
> 2.18.0
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v3 0/4] devres: provide and use devm_kstrdup_const()

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 12:11:46PM +0200, Bartosz Golaszewski wrote:
> This series implements devm_kstrdup_const() together with some
> prerequisite changes and uses it in pmc-atom driver.
> 

Through which tree you are assuming this would be directed?

> v1 -> v2:
> - fixed the changelog in the patch implementing devm_kstrdup_const()
> - fixed the kernel doc
> - moved is_kernel_rodata() to asm-generic/sections.h
> - fixed constness
> 
> v2 -> v3:
> - rebased on top of 4.19-rc5 as there were some conflicts in the
>   pmc-atom driver
> - collected Reviewed-by tags
> 
> Bartosz Golaszewski (4):
>   devres: constify p in devm_kfree()
>   mm: move is_kernel_rodata() to asm-generic/sections.h
>   devres: provide devm_kstrdup_const()
>   clk: pmc-atom: use devm_kstrdup_const()
> 
>  drivers/base/devres.c          | 43 ++++++++++++++++++++++++++++++++--
>  drivers/clk/x86/clk-pmc-atom.c | 19 ++++-----------
>  include/asm-generic/sections.h | 14 +++++++++++
>  include/linux/device.h         |  5 +++-
>  mm/util.c                      |  7 ------
>  5 files changed, 63 insertions(+), 25 deletions(-)
> 
> -- 
> 2.18.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] devres: provide and use devm_kstrdup_const()

[Bartosz Golaszewski]

pon., 24 wrz 2018 o 13:16 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Mon, Sep 24, 2018 at 12:11:46PM +0200, Bartosz Golaszewski wrote:
> > This series implements devm_kstrdup_const() together with some
> > prerequisite changes and uses it in pmc-atom driver.
> >
>
> Through which tree you are assuming this would be directed?
>

I think that patches 1-3 would be best picked up by Andrew Morton.
Patch 4 looks like it should go through the clock tree unless Stephen
is fine with Acking it and it going through the mm tree together with
others.

Bart

> > v1 -> v2:
> > - fixed the changelog in the patch implementing devm_kstrdup_const()
> > - fixed the kernel doc
> > - moved is_kernel_rodata() to asm-generic/sections.h
> > - fixed constness
> >
> > v2 -> v3:
> > - rebased on top of 4.19-rc5 as there were some conflicts in the
> >   pmc-atom driver
> > - collected Reviewed-by tags
> >
> > Bartosz Golaszewski (4):
> >   devres: constify p in devm_kfree()
> >   mm: move is_kernel_rodata() to asm-generic/sections.h
> >   devres: provide devm_kstrdup_const()
> >   clk: pmc-atom: use devm_kstrdup_const()
> >
> >  drivers/base/devres.c          | 43 ++++++++++++++++++++++++++++++++--
> >  drivers/clk/x86/clk-pmc-atom.c | 19 ++++-----------
> >  include/asm-generic/sections.h | 14 +++++++++++
> >  include/linux/device.h         |  5 +++-
> >  mm/util.c                      |  7 ------
> >  5 files changed, 63 insertions(+), 25 deletions(-)
> >
> > --
> > 2.18.0
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3 4/4] clk: pmc-atom: use devm_kstrdup_const()

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 12:11:50PM +0200, Bartosz Golaszewski wrote:
> 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().

> @@ -352,8 +344,6 @@ static int plt_clk_probe(struct platform_device *pdev)
>  		goto err_drop_mclk;
>  	}
>  
> -	plt_clk_free_parent_names_loop(parent_names, data->nparents);
> -
>  	platform_set_drvdata(pdev, data);
>  	return 0;

I don't think this is a good example.

You changed a behaviour here in the way that you keep all chunks of memory
(even small enough for pointers) during entire life time of the driver, which
pretty likely would be forever till next boot.

In the original case the memory was freed immediately in probe either it fails
or returns with success.

NAK, sorry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] clk: pmc-atom: use devm_kstrdup_const()

[Bartosz Golaszewski]

pon., 24 wrz 2018 o 13:23 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Mon, Sep 24, 2018 at 12:11:50PM +0200, Bartosz Golaszewski wrote:
> > 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().
>
> > @@ -352,8 +344,6 @@ static int plt_clk_probe(struct platform_device *pdev)
> >               goto err_drop_mclk;
> >       }
> >
> > -     plt_clk_free_parent_names_loop(parent_names, data->nparents);
> > -
> >       platform_set_drvdata(pdev, data);
> >       return 0;
>
> I don't think this is a good example.
>
> You changed a behaviour here in the way that you keep all chunks of memory
> (even small enough for pointers) during entire life time of the driver, which
> pretty likely would be forever till next boot.
>
> In the original case the memory was freed immediately in probe either it fails
> or returns with success.
>
> NAK, sorry.
>
>

I see.

I'd like to still merge patches 1-3 and then I'd come up with better
examples for the next release cycle once these are in?

Bart

Re: [PATCH v3 4/4] clk: pmc-atom: use devm_kstrdup_const()

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 01:44:19PM +0200, Bartosz Golaszewski wrote:
> pon., 24 wrz 2018 o 13:23 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > On Mon, Sep 24, 2018 at 12:11:50PM +0200, Bartosz Golaszewski wrote:
> > > 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().
> >
> > > @@ -352,8 +344,6 @@ static int plt_clk_probe(struct platform_device *pdev)
> > >               goto err_drop_mclk;
> > >       }
> > >
> > > -     plt_clk_free_parent_names_loop(parent_names, data->nparents);
> > > -
> > >       platform_set_drvdata(pdev, data);
> > >       return 0;
> >
> > I don't think this is a good example.
> >
> > You changed a behaviour here in the way that you keep all chunks of memory
> > (even small enough for pointers) during entire life time of the driver, which
> > pretty likely would be forever till next boot.
> >
> > In the original case the memory was freed immediately in probe either it fails
> > or returns with success.
> >
> > NAK, sorry.
> >
> >
> 
> I see.
> 
> I'd like to still merge patches 1-3 and then I'd come up with better
> examples for the next release cycle once these are in?

I'm fine with first three, though I can't come up with better example for you
now. My previous comment solely to clk-pmc-atom code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Kees Cook]

On Mon, Sep 24, 2018 at 3:11 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Provide a resource managed version of kstrdup_const(). This variant
> internally calls devm_kstrdup() on pointers that are outside of
> .rodata section and returns the string as is otherwise.
>
> Also provide a corresponding version of devm_kfree().
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/base/devres.c  | 38 ++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h |  3 +++
>  2 files changed, 41 insertions(+)
>
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 438c91a43508..48185d57bc5b 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/percpu.h>
>
> +#include <asm/sections.h>
> +
>  #include "base.h"
>
>  struct devres_node {
> @@ -822,6 +824,28 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
>  }
>  EXPORT_SYMBOL_GPL(devm_kstrdup);
>
> +/**
> + * devm_kstrdup_const - resource managed conditional string duplication
> + * @dev: device for which to duplicate the string
> + * @s: the string to duplicate
> + * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> + *
> + * Strings allocated by devm_kstrdup_const will be automatically freed when
> + * the associated device is detached.
> + *
> + * RETURNS:
> + * Source string if it is in .rodata section otherwise it falls back to
> + * devm_kstrdup.
> + */
> +const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp)
> +{
> +       if (is_kernel_rodata((unsigned long)s))
> +               return s;
> +
> +       return devm_kstrdup(dev, s, gfp);
> +}
> +EXPORT_SYMBOL(devm_kstrdup_const);
> +
>  /**
>   * devm_kvasprintf - Allocate resource managed space and format a string
>   *                  into that.
> @@ -895,6 +919,20 @@ void devm_kfree(struct device *dev, const void *p)
>  }
>  EXPORT_SYMBOL_GPL(devm_kfree);
>
> +/**
> + * devm_kfree_const - Resource managed conditional kfree
> + * @dev: device this memory belongs to
> + * @p: memory to free
> + *
> + * Function calls devm_kfree only if @p is not in .rodata section.
> + */
> +void devm_kfree_const(struct device *dev, const void *p)
> +{
> +       if (!is_kernel_rodata((unsigned long)p))
> +               devm_kfree(dev, p);
> +}
> +EXPORT_SYMBOL(devm_kfree_const);
> +
>  /**
>   * devm_kmemdup - Resource-managed kmemdup
>   * @dev: Device this memory belongs to
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 33f7cb271fbb..79ccc6eb0975 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,
>         return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
>  }
>  extern void devm_kfree(struct device *dev, const void *p);
> +extern void devm_kfree_const(struct device *dev, const void *p);

With devm_kfree and devm_kfree_const both taking "const", how are
devm_kstrdup_const() and devm_kfree_const() going to be correctly
paired at compile time? (i.e. I wasn't expecting the prototype change
to devm_kfree())

-Kees

>  extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
> +extern const char *devm_kstrdup_const(struct device *dev,
> +                                     const char *s, gfp_t gfp);
>  extern void *devm_kmemdup(struct device *dev, const void *src, size_t len,
>                           gfp_t gfp);
>
> --
> 2.18.0
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Bartosz Golaszewski]

czw., 27 wrz 2018 o 01:20 Kees Cook <keescook@chromium.org> napisał(a):
>
> On Mon, Sep 24, 2018 at 3:11 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > Provide a resource managed version of kstrdup_const(). This variant
> > internally calls devm_kstrdup() on pointers that are outside of
> > .rodata section and returns the string as is otherwise.
> >
> > Also provide a corresponding version of devm_kfree().
> >
> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/base/devres.c  | 38 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/device.h |  3 +++
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index 438c91a43508..48185d57bc5b 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -11,6 +11,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/percpu.h>
> >
> > +#include <asm/sections.h>
> > +
> >  #include "base.h"
> >
> >  struct devres_node {
> > @@ -822,6 +824,28 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
> >  }
> >  EXPORT_SYMBOL_GPL(devm_kstrdup);
> >
> > +/**
> > + * devm_kstrdup_const - resource managed conditional string duplication
> > + * @dev: device for which to duplicate the string
> > + * @s: the string to duplicate
> > + * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> > + *
> > + * Strings allocated by devm_kstrdup_const will be automatically freed when
> > + * the associated device is detached.
> > + *
> > + * RETURNS:
> > + * Source string if it is in .rodata section otherwise it falls back to
> > + * devm_kstrdup.
> > + */
> > +const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp)
> > +{
> > +       if (is_kernel_rodata((unsigned long)s))
> > +               return s;
> > +
> > +       return devm_kstrdup(dev, s, gfp);
> > +}
> > +EXPORT_SYMBOL(devm_kstrdup_const);
> > +
> >  /**
> >   * devm_kvasprintf - Allocate resource managed space and format a string
> >   *                  into that.
> > @@ -895,6 +919,20 @@ void devm_kfree(struct device *dev, const void *p)
> >  }
> >  EXPORT_SYMBOL_GPL(devm_kfree);
> >
> > +/**
> > + * devm_kfree_const - Resource managed conditional kfree
> > + * @dev: device this memory belongs to
> > + * @p: memory to free
> > + *
> > + * Function calls devm_kfree only if @p is not in .rodata section.
> > + */
> > +void devm_kfree_const(struct device *dev, const void *p)
> > +{
> > +       if (!is_kernel_rodata((unsigned long)p))
> > +               devm_kfree(dev, p);
> > +}
> > +EXPORT_SYMBOL(devm_kfree_const);
> > +
> >  /**
> >   * devm_kmemdup - Resource-managed kmemdup
> >   * @dev: Device this memory belongs to
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 33f7cb271fbb..79ccc6eb0975 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,
> >         return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
> >  }
> >  extern void devm_kfree(struct device *dev, const void *p);
> > +extern void devm_kfree_const(struct device *dev, const void *p);
>
> With devm_kfree and devm_kfree_const both taking "const", how are
> devm_kstrdup_const() and devm_kfree_const() going to be correctly
> paired at compile time? (i.e. I wasn't expecting the prototype change
> to devm_kfree())
>

I guess the same as with kfree() and kfree_const() which both take
const void * as argument - it's up to users to only use
devm_kfree_const() on resources allocated with devm_kstrdup_const().

Bart

> -Kees
>
> >  extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
> > +extern const char *devm_kstrdup_const(struct device *dev,
> > +                                     const char *s, gfp_t gfp);
> >  extern void *devm_kmemdup(struct device *dev, const void *src, size_t len,
> >                           gfp_t gfp);
> >
> > --
> > 2.18.0
> >
>
>
>
> --
> Kees Cook
> Pixel Security

Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Rasmus Villemoes]

On 2018-09-27 01:13, Kees Cook wrote:
> On Mon, Sep 24, 2018 at 3:11 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> Provide a resource managed version of kstrdup_const(). This variant
>> internally calls devm_kstrdup() on pointers that are outside of
>> .rodata section and returns the string as is otherwise.
>>
>> Also provide a corresponding version of devm_kfree().
>>
>> +/**
>> + * devm_kfree_const - Resource managed conditional kfree
>> + * @dev: device this memory belongs to
>> + * @p: memory to free
>> + *
>> + * Function calls devm_kfree only if @p is not in .rodata section.
>> + */
>> +void devm_kfree_const(struct device *dev, const void *p)
>> +{
>> +       if (!is_kernel_rodata((unsigned long)p))
>> +               devm_kfree(dev, p);
>> +}
>> +EXPORT_SYMBOL(devm_kfree_const);
>> +
>>  /**
>>   * devm_kmemdup - Resource-managed kmemdup
>>   * @dev: Device this memory belongs to
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 33f7cb271fbb..79ccc6eb0975 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,
>>         return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
>>  }
>>  extern void devm_kfree(struct device *dev, const void *p);
>> +extern void devm_kfree_const(struct device *dev, const void *p);
> 
> With devm_kfree and devm_kfree_const both taking "const", how are
> devm_kstrdup_const() and devm_kfree_const() going to be correctly
> paired at compile time? (i.e. I wasn't expecting the prototype change
> to devm_kfree())

Just drop devm_kfree_const and teach devm_kfree to ignore
is_kernel_rodata(). That avoids the 50-100 bytes of overhead for adding
yet another EXPORT_SYMBOL and makes it easier to port drivers to
devm_kstrdup_const (and avoids the bugs Kees is worried about). devm
managed resources are almost never freed explicitly, so that single
extra comparison in devm_kfree shouldn't matter for performance.

Rasmus

Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Geert Uytterhoeven]

Hi Rasmus,

On Thu, Sep 27, 2018 at 12:55 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 2018-09-27 01:13, Kees Cook wrote:
> > On Mon, Sep 24, 2018 at 3:11 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >> Provide a resource managed version of kstrdup_const(). This variant
> >> internally calls devm_kstrdup() on pointers that are outside of
> >> .rodata section and returns the string as is otherwise.
> >>
> >> Also provide a corresponding version of devm_kfree().
> >>
> >> +/**
> >> + * devm_kfree_const - Resource managed conditional kfree
> >> + * @dev: device this memory belongs to
> >> + * @p: memory to free
> >> + *
> >> + * Function calls devm_kfree only if @p is not in .rodata section.
> >> + */
> >> +void devm_kfree_const(struct device *dev, const void *p)
> >> +{
> >> +       if (!is_kernel_rodata((unsigned long)p))
> >> +               devm_kfree(dev, p);
> >> +}
> >> +EXPORT_SYMBOL(devm_kfree_const);
> >> +
> >>  /**
> >>   * devm_kmemdup - Resource-managed kmemdup
> >>   * @dev: Device this memory belongs to
> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> index 33f7cb271fbb..79ccc6eb0975 100644
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,
> >>         return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO);
> >>  }
> >>  extern void devm_kfree(struct device *dev, const void *p);
> >> +extern void devm_kfree_const(struct device *dev, const void *p);
> >
> > With devm_kfree and devm_kfree_const both taking "const", how are
> > devm_kstrdup_const() and devm_kfree_const() going to be correctly
> > paired at compile time? (i.e. I wasn't expecting the prototype change
> > to devm_kfree())
>
> Just drop devm_kfree_const and teach devm_kfree to ignore
> is_kernel_rodata(). That avoids the 50-100 bytes of overhead for adding
> yet another EXPORT_SYMBOL and makes it easier to port drivers to
> devm_kstrdup_const (and avoids the bugs Kees is worried about). devm
> managed resources are almost never freed explicitly, so that single
> extra comparison in devm_kfree shouldn't matter for performance.

I guess we can also teach kfree() to ignore is_kernel_rodata(), and
drop kfree_const()?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()

[Rasmus Villemoes]

On 2018-09-27 13:01, Geert Uytterhoeven wrote:
> Hi Rasmus,
> 
> On Thu, Sep 27, 2018 at 12:55 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> On 2018-09-27 01:13, Kees Cook wrote:
>>
>> Just drop devm_kfree_const and teach devm_kfree to ignore
>> is_kernel_rodata(). That avoids the 50-100 bytes of overhead for adding
>> yet another EXPORT_SYMBOL and makes it easier to port drivers to
>> devm_kstrdup_const (and avoids the bugs Kees is worried about). devm
>> managed resources are almost never freed explicitly, so that single
>> extra comparison in devm_kfree shouldn't matter for performance.
> 
> I guess we can also teach kfree() to ignore is_kernel_rodata(), and
> drop kfree_const()?

In principle, yes, but the difference is that kfree() is called a lot
more frequently, and on normal code paths, whereas devm_kfree is more
often (though not always) called on error paths.

The goal of _const variants of strdup is to save some memory, so one
place to start is to reduce the .text overhead of that feature. And it
avoids introducing subtle bugs if some devm_kfree() call is missed
during conversion to devm_kstrdup_const().

Rasmus