devres: provide and use devm_kstrdup_const()
Subsystems (5)
v3 cover devres: provide and use devm_kstrdup_const()
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(-)
Signature · 1 line
2.18.0
PATCH v3 1/4 devres: constify p in devm_kfree()
Make devm_kfree() signature uniform with that of kfree(). To avoid compiler warnings: cast p to (void *) when calling devres_destroy().
--- drivers/base/devres.c | 5 +++-- include/linux/device.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-)
drivers/base/devres.c +3 -2
@@ -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);
include/linux/device.h +1 -1
@@ -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);
Signature · 1 line
2.18.0
PATCH v3 2/4 mm: move is_kernel_rodata() to asm-generic/sections.h
In reply to Bartosz GolaszewskiExport this routine so that we can use it later in devm_kstrdup_const() and devm_kfree_const().
--- include/asm-generic/sections.h | 14 ++++++++++++++ mm/util.c | 7 ------- 2 files changed, 14 insertions(+), 7 deletions(-)
include/asm-generic/sections.h +14 -0
@@ -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_ */
mm/util.c +0 -7
@@ -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
Signature · 1 line
2.18.0
Re: [PATCH v3 2/4] mm: move is_kernel_rodata() to asm-generic/sections.h
Quoted 5 lines · Bartosz Golaszewski
Export this routine so that we can use it later in
devm_kstrdup_const()anddevm_kfree_const().Signed-off-by Bartosz Golaszewski <[email protected]>Reviewed-by Bjorn Andersson <[email protected]>
Quoted 53 lines
--- include/asm-generic/sections.h | 14 ++++++++++++++ mm/util.c | 7 ------- 2 files changed, 14 insertions(+), 7 deletions(-)include/asm-generic/sections.h +14
-0@@ -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_ */ mm/util.c +0
-7@@ -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 Signature · 1 line
2.18.0
Signature · 2 lines
Sincerely yours, Mike.
PATCH v3 3/4 devres: provide devm_kstrdup_const()
In reply to Bartosz GolaszewskiProvide 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().
--- drivers/base/devres.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 41 insertions(+)
drivers/base/devres.c +38 -0
@@ -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
include/linux/device.h +3 -0
@@ -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);
Signature · 1 line
2.18.0
Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()
Quoted 8 lines · Bartosz Golaszewski
Provide a resource managed version of
kstrdup_const(). This variant internally callsdevm_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 <[email protected]>Reviewed-by Bjorn Andersson <[email protected]>
Quoted 86 lines
--- drivers/base/devres.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 41 insertions(+)drivers/base/devres.c +38
-0@@ -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 include/linux/device.h +3
-0@@ -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);Signature · 1 line
2.18.0
Signature · 2 lines
Sincerely yours, Mike.
Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()
In reply to Bartosz GolaszewskiQuoted 81 lines · Bartosz Golaszewski
Provide a resource managed version of
kstrdup_const(). This variant internally callsdevm_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 <[email protected]>Reviewed-by Bjorn Andersson <[email protected]>--- drivers/base/devres.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 41 insertions(+)drivers/base/devres.c +38
-0@@ -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 include/linux/device.h +0
-0
include/linux/device.h static inline void *devm_kcalloc(struct device *dev, · line 696
Quoted 4 lines
include/linux/device.h +1
-0@@ -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
Quoted 9 lines
include/linux/device.h +2
-1extern 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
Signature · 2 lines
Kees Cook Pixel Security
Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()
czw., 27 wrz 2018 o 01:20 Kees Cook <[email protected]> napisa=C5=82(a)= :
Quoted 2 lines
On Mon, Sep 24, 2018 at 3:11 AM, Bartosz Golaszewski <[email protected]> wrot=
e:
Quoted 21 lines
Quoted 21 lines
Provide a resource managed version of
kstrdup_const(). This variant internally callsdevm_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 <[email protected]>Reviewed-by Bjorn Andersson <[email protected]>--- drivers/base/devres.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/device.h | 3 +++ 2 files changed, 41 insertions(+)drivers/base/devres.c +0
-0@@ -11,6 +11,8 @@#include <linux/slab.h> #include <linux/percpu.h>
Quoted 6 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char =
Quoted 6 lines
drivers/base/devres.c +2
-0+#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)
Quoted 5 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
Quoted 5 lines
drivers/base/devres.c +2
-0}EXPORT_SYMBOL_GPL (devm_kstrdup); +/** +* devm_kstrdup_const - resource managed conditional string duplicatio=
n
Quoted 3 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
Quoted 3 lines
drivers/base/devres.c +3
-0+* @dev: device for which to duplicate the string +* @s: the string to duplicate +* @gfp: the GFP mask used in the kmalloc() call when allocating memor=
y
Quoted 2 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char = · line 833
Quoted 2 lines
drivers/base/devres.c +2
-0+* +* Strings allocated by devm_kstrdup_const will be automatically freed=
when
Quoted 4 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
Quoted 4 lines
drivers/base/devres.c +4
-0+* the associated device is detached. +* +* RETURNS: +* Source string if it is in .rodata section otherwise it falls back t=
o
Quoted 3 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
Quoted 3 lines
drivers/base/devres.c +3
-0+* devm_kstrdup. +*/ +const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_=
t gfp)
Quoted 10 lines
drivers/base/devres.c char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
Quoted 10 lines
drivers/base/devres.c +8
-0+{ +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 stri=
ng
Quoted 19 lines
Quoted 19 lines
drivers/base/devres.c +14
-0* 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); +
Quoted 8 lines
include/linux/device.h static inline void *devm_kcalloc(struct device *de=
Quoted 8 lines
drivers/base/devres.c +0
-0/** * devm_kmemdup - Resource-managed kmemdup * @dev: Device this memory belongs to include/linux/device.h +0
-0@@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *de=
v,
Quoted 10 lines
include/linux/device.h static inline void *devm_kcalloc(struct device *dev, · line 696
Quoted 4 lines
include/linux/device.h +1
-0return 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_kfreeanddevm_kfree_constboth taking "const", how aredevm_kstrdup_const()anddevm_kfree_const()going to be correctly paired at compile time? (i.e. I wasn't expecting the prototype change todevm_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
Quoted 3 lines
-Kees
Quoted 1 line
include/linux/device.h +0
-0extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp=
) __malloc;
Quoted 3 lines
include/linux/device.h static inline void *devm_kcalloc(struct device *dev,
Quoted 3 lines
include/linux/device.h +2
-0+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,
Quoted 11 lines
Quoted 5 lines
include/linux/device.h +0
-1gfp_t gfp); --2.18.0
--
Kees Cook
Pixel Security
Re: [PATCH v3 3/4] devres: provide devm_kstrdup_const()
In reply to Kees CookQuoted 30 lines · Kees Cook
Quoted 29 lines · Bartosz Golaszewski
Provide a resource managed version of
kstrdup_const(). This variant internally callsdevm_kstrdup()on pointers that are outside of .rodata section and returns the string as is otherwise.Also provide a corresponding version of
devm_kfree().drivers/base/devres.c +14
-0+/** +* 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 include/linux/device.h +0
-0@@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,return devm_kmalloc_array (dev, n, size, flags | __GFP_ZERO);
Quoted 8 lines
include/linux/device.h static inline void *devm_kcalloc(struct device *dev, · line 696
Quoted 3 lines
include/linux/device.h +1
-0}extern void devm_kfree (struct device *dev,const void *p); +extern void devm_kfree_const (struct device *dev,const void *p);With
devm_kfreeanddevm_kfree_constboth taking "const", how aredevm_kstrdup_const()anddevm_kfree_const()going to be correctly paired at compile time? (i.e. I wasn't expecting the prototype change todevm_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()
Hi Rasmus,
Quoted 40 lines · Rasmus Villemoes
Quoted 30 lines · Kees Cook
Quoted 29 lines · Bartosz Golaszewski
Provide a resource managed version of
kstrdup_const(). This variant internally callsdevm_kstrdup()on pointers that are outside of .rodata section and returns the string as is otherwise.Also provide a corresponding version of
devm_kfree().drivers/base/devres.c +14
-0+/** +* 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 include/linux/device.h +0
-0@@ -693,7 +693,10 @@ static inline void *devm_kcalloc(struct device *dev,return devm_kmalloc_array (dev, n, size, flags | __GFP_ZERO);Quoted 8 lines
include/linux/device.h static inline void *devm_kcalloc(struct device *dev, · line 696
Quoted 3 lines
include/linux/device.h +1
-0}extern void devm_kfree (struct device *dev,const void *p); +extern void devm_kfree_const (struct device *dev,const void *p);With
devm_kfreeanddevm_kfree_constboth taking "const", how aredevm_kstrdup_const()anddevm_kfree_const()going to be correctly paired at compile time? (i.e. I wasn't expecting the prototype change todevm_kfree())
Quoted 6 lines
Just drop
devm_kfree_constand teachdevm_kfreeto ignoreis_kernel_rodata(). That avoids the 50-100 bytes of overhead for adding yet anotherEXPORT_SYMBOLand makes it easier to port drivers todevm_kstrdup_const(and avoids the bugs Kees is worried about). devm managed resources are almost never freed explicitly, so that single extra comparison indevm_kfreeshouldn'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
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] 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()
Quoted 13 lines · Geert Uytterhoeven
Hi Rasmus,
Quoted 8 lines · Rasmus Villemoes
On 2018-09-27 01:13, Kees Cook wrote:
Just drop
devm_kfree_constand teachdevm_kfreeto ignoreis_kernel_rodata(). That avoids the 50-100 bytes of overhead for adding yet anotherEXPORT_SYMBOLand makes it easier to port drivers todevm_kstrdup_const(and avoids the bugs Kees is worried about). devm managed resources are almost never freed explicitly, so that single extra comparison indevm_kfreeshouldn't matter for performance.
Quoted 2 lines
I guess we can also teach
kfree()to ignoreis_kernel_rodata(), and dropkfree_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
PATCH v3 4/4 clk: pmc-atom: use devm_kstrdup_const()
In reply to Bartosz GolaszewskiUse 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().
--- drivers/clk/x86/clk-pmc-atom.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
drivers/clk/x86/clk-pmc-atom.c +4 -15
@@ -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; }
Signature · 1 line
2.18.0
Andy Shevchenko
NAKRe: [PATCH v3 4/4] clk: pmc-atom: use devm_kstrdup_const()
drivers/clk/x86/clk-pmc-atom.c static int plt_clk_probe(struct platform_device *pdev) · line 348
Quoted 13 lines · 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()withdevm_kcalloc().drivers/clk/x86/clk-pmc-atom.c +0
-2@@ -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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 4/4] clk: pmc-atom: use devm_kstrdup_const()
pon., 24 wrz 2018 o 13:23 Andy Shevchenko <[email protected]> napisa=C5=82(a):
Quoted 8 lines · Bartosz Golaszewski
Review static int plt_clk_probe(struct platform_device *pd=
Quoted 6 lines
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()withdevm_kcalloc().(quoted diff) +0
-0@@ -352,8 +344,6 @@ static int plt_clk_probe(struct platform_device *pd=
ev)
Quoted 11 lines
drivers/clk/x86/clk-pmc-atom.c static int plt_clk_probe(struct platform_device *pdev) · line 348
Quoted 7 lines
drivers/clk/x86/clk-pmc-atom.c +0
-2goto 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 memor=
y
Quoted 1 line
(even small enough for pointers) during entire life time of the driver, w=
hich
Quoted 3 lines
pretty likely would be forever till next boot.
In the original case the memory was freed immediately in probe either it =
fails
Quoted 5 lines
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()
Quoted 27 lines · Bartosz Golaszewski
pon., 24 wrz 2018 o 13:23 Andy Shevchenko
<[email protected]> napisał(a):Quoted 25 lines · Bartosz Golaszewski
drivers/clk/x86/clk-pmc-atom.c static int plt_clk_probe(struct platform_device *pdev) · line 348
Quoted 13 lines
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()withdevm_kcalloc().drivers/clk/x86/clk-pmc-atom.c +0
-2@@ -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.
Quoted 8 lines
Quoted 3 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 3 lines · Bartosz Golaszewski
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?
Quoted 27 lines
v1 -> v2:
- fixed the changelog in the patch implementingdevm_kstrdup_const()
- fixed the kernel doc
- movedis_kernel_rodata()toasm-generic/sections.h
- fixed constnessv2 -> v3:
- rebased on top of 4.19-rc5 as there were some conflicts in the pmc-atom driver
- collected Reviewed-by tagsBartosz Golaszewski (4):
devres: constify p indevm_kfree()
mm: moveis_kernel_rodata()toasm-generic/sections.h
devres: providedevm_kstrdup_const()
clk: pmc-atom: usedevm_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(-)Signature · 1 line
2.18.0
Signature · 2 lines
With Best Regards, Andy Shevchenko
pon., 24 wrz 2018 o 13:16 Andy Shevchenko <[email protected]> napisa=C5=82(a):
Quoted 8 lines · Bartosz Golaszewski
Quoted 3 lines
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
Quoted 33 lines
Quoted 24 lines
v1 -> v2:
- fixed the changelog in the patch implementingdevm_kstrdup_const()
- fixed the kernel doc
- movedis_kernel_rodata()toasm-generic/sections.h
- fixed constnessv2 -> v3:
- rebased on top of 4.19-rc5 as there were some conflicts in the pmc-atom driver
- collected Reviewed-by tagsBartosz Golaszewski (4):
devres: constify p indevm_kfree()
mm: moveis_kernel_rodata()toasm-generic/sections.h
devres: providedevm_kstrdup_const()
clk: pmc-atom: usedevm_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(-)Quoted 3 lines
--
2.18.0--
With Best Regards,
Andy Shevchenko