Skip to contentSkip to conversation

devres: provide and use devm_krealloc()

PATCH Superseded Bartosz Golaszewski 5 participants 24 messages Jun 29, 2020 – Apr 12, 2021

Subsystems (3)
Documentation Driver core Hardware monitoring

v2 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

v2 cover devres: provide and use devm_krealloc()

From: Bartosz Golaszewski <[email protected]>

Regular krealloc() obviously can't work with managed memory. This series implements devm_krealloc() and adds the first user with hope that this helper will be adopted by other drivers currently using non-managed krealloc().

Some additional changes to the code modified by main patches are included.

v1 -> v2:
- remove leftover call to hwmon_device_unregister() from pmbus_core.c
- add a patch extending devm_kmalloc() to handle zero size case
- use WARN_ON() instead of WARN_ONCE() in devm_krealloc() when passed a pointer to non-managed memory
- correctly handle the case when devm_krealloc() is passed a pointer to memory in .rodata (potentially returned by devm_kstrdup_const())
- correctly handle ZERO_SIZE_PTR passed as the ptr argument in devm_krealloc()

Bartosz Golaszewski (6):
devres: remove stray space from devm_kmalloc() definition
devres: move the size check from alloc_dr() into a separate function
device: remove 'extern' attribute from function prototypes in device.h
devres: handle zero size in devm_kmalloc()
devres: provide devm_krealloc()
hwmon: pmbus: use more devres helpers

 .../driver-api/driver-model/devres.rst        |   1 +
 drivers/base/devres.c                         |  75 +++++-
 drivers/hwmon/pmbus/pmbus_core.c              |  28 +--
 include/linux/device.h                        | 225 +++++++++---------
 4 files changed, 187 insertions(+), 142 deletions(-)
Signature · 1 line
2.26.1
author
#1 lore raw

PATCH v2 1/6 devres: remove stray space from devm_kmalloc() definition

From: Bartosz Golaszewski <[email protected]>

Use the preferred coding style for functions returning pointers.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/base/devres.c +1 -1
index 0bbb328bd17f..c34327219c34 100644
@@ -807,7 +807,7 @@ static int devm_kmalloc_match(struct device *dev, void *res, void *data)
  * RETURNS:
  * Pointer to allocated memory on success, NULL on failure.
  */
-void * devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
 	struct devres *dr;
Signature · 1 line
2.26.1
author
#2 lore raw

PATCH v2 2/6 devres: move the size check from alloc_dr() into a separate function

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We will perform the same size check in devm_krealloc(). Move the relevant code into a separate helper.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/base/devres.c +11 -3
index c34327219c34..1df1fb10b2d9 100644
@@ -89,15 +89,23 @@ static struct devres_group * node_to_group(struct devres_node *node)
 	return NULL;
 }
 
+static bool check_dr_size(size_t size, size_t *tot_size)
+{
+	/* We must catch any near-SIZE_MAX cases that could overflow. */
+	if (unlikely(check_add_overflow(sizeof(struct devres),
+					size, tot_size)))
+		return false;
+
+	return true;
+}
+
 static __always_inline struct devres * alloc_dr(dr_release_t release,
 						size_t size, gfp_t gfp, int nid)
 {
 	size_t tot_size;
 	struct devres *dr;
 
-	/* We must catch any near-SIZE_MAX cases that could overflow. */
-	if (unlikely(check_add_overflow(sizeof(struct devres), size,
-					&tot_size)))
+	if (!check_dr_size(size, &tot_size))
 		return NULL;
 
 	dr = kmalloc_node_track_caller(tot_size, gfp, nid);
Signature · 1 line
2.26.1
author
#3 lore raw

PATCH v2 3/6 device: remove 'extern' attribute from function prototypes in device.h

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Functions are declared 'extern' implicitly by the compiler. There's no reason to prepend every prototype with it. Remove the 'extern' keyword from all function declarations in linux/device.h.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 include/linux/device.h | 223 ++++++++++++++++++++---------------------
 1 file changed, 107 insertions(+), 116 deletions(-)
include/linux/device.h +107 -116
index 15460a5ac024..9cadb647cacc 100644
@@ -145,68 +145,66 @@ ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
 	struct device_attribute dev_attr_##_name =		\
 		__ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
 
-extern int device_create_file(struct device *device,
-			      const struct device_attribute *entry);
-extern void device_remove_file(struct device *dev,
-			       const struct device_attribute *attr);
-extern bool device_remove_file_self(struct device *dev,
-				    const struct device_attribute *attr);
-extern int __must_check device_create_bin_file(struct device *dev,
+int device_create_file(struct device *device,
+		       const struct device_attribute *entry);
+void device_remove_file(struct device *dev,
+			const struct device_attribute *attr);
+bool device_remove_file_self(struct device *dev,
+			     const struct device_attribute *attr);
+int __must_check device_create_bin_file(struct device *dev,
 					const struct bin_attribute *attr);
-extern void device_remove_bin_file(struct device *dev,
-				   const struct bin_attribute *attr);
+void device_remove_bin_file(struct device *dev,
+			    const struct bin_attribute *attr);
 
 /* device resource management */
 typedef void (*dr_release_t)(struct device *dev, void *res);
 typedef int (*dr_match_t)(struct device *dev, void *res, void *match_data);
 
 #ifdef CONFIG_DEBUG_DEVRES
-extern void *__devres_alloc_node(dr_release_t release, size_t size, gfp_t gfp,
-				 int nid, const char *name) __malloc;
+void *__devres_alloc_node(dr_release_t release, size_t size, gfp_t gfp,
+			  int nid, const char *name) __malloc;
 #define devres_alloc(release, size, gfp) \
 	__devres_alloc_node(release, size, gfp, NUMA_NO_NODE, #release)
 #define devres_alloc_node(release, size, gfp, nid) \
 	__devres_alloc_node(release, size, gfp, nid, #release)
 #else
-extern void *devres_alloc_node(dr_release_t release, size_t size, gfp_t gfp,
-			       int nid) __malloc;
+void *devres_alloc_node(dr_release_t release, size_t size,
+			gfp_t gfp, int nid) __malloc;
 static inline void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp)
 {
 	return devres_alloc_node(release, size, gfp, NUMA_NO_NODE);
 }
 #endif
 
-extern void devres_for_each_res(struct device *dev, dr_release_t release,
-				dr_match_t match, void *match_data,
-				void (*fn)(struct device *, void *, void *),
-				void *data);
-extern void devres_free(void *res);
-extern void devres_add(struct device *dev, void *res);
-extern void *devres_find(struct device *dev, dr_release_t release,
-			 dr_match_t match, void *match_data);
-extern void *devres_get(struct device *dev, void *new_res,
-			dr_match_t match, void *match_data);
-extern void *devres_remove(struct device *dev, dr_release_t release,
-			   dr_match_t match, void *match_data);
-extern int devres_destroy(struct device *dev, dr_release_t release,
-			  dr_match_t match, void *match_data);
-extern int devres_release(struct device *dev, dr_release_t release,
-			  dr_match_t match, void *match_data);
+void devres_for_each_res(struct device *dev, dr_release_t release,
+			 dr_match_t match, void *match_data,
+			 void (*fn)(struct device *, void *, void *),
+			 void *data);
+void devres_free(void *res);
+void devres_add(struct device *dev, void *res);
+void *devres_find(struct device *dev, dr_release_t release,
+		  dr_match_t match, void *match_data);
+void *devres_get(struct device *dev, void *new_res,
+		 dr_match_t match, void *match_data);
+void *devres_remove(struct device *dev, dr_release_t release,
+		    dr_match_t match, void *match_data);
+int devres_destroy(struct device *dev, dr_release_t release,
+		   dr_match_t match, void *match_data);
+int devres_release(struct device *dev, dr_release_t release,
+		   dr_match_t match, void *match_data);
 
 /* devres group */
-extern void * __must_check devres_open_group(struct device *dev, void *id,
-					     gfp_t gfp);
-extern void devres_close_group(struct device *dev, void *id);
-extern void devres_remove_group(struct device *dev, void *id);
-extern int devres_release_group(struct device *dev, void *id);
+void * __must_check devres_open_group(struct device *dev, void *id, gfp_t gfp);
+void devres_close_group(struct device *dev, void *id);
+void devres_remove_group(struct device *dev, void *id);
+int devres_release_group(struct device *dev, void *id);
 
 /* managed devm_k.alloc/kfree for device drivers */
-extern void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
-extern __printf(3, 0)
-char *devm_kvasprintf(struct device *dev, gfp_t gfp, const char *fmt,
-		      va_list ap) __malloc;
-extern __printf(3, 4)
-char *devm_kasprintf(struct device *dev, gfp_t gfp, const char *fmt, ...) __malloc;
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
+__printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
+				     const char *fmt, va_list ap) __malloc;
+__printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,
+				    const char *fmt, ...) __malloc;
 static inline void *devm_kzalloc(struct device *dev, size_t size, gfp_t gfp)
 {
 	return devm_kmalloc(dev, size, gfp | __GFP_ZERO);

@@ -226,16 +224,14 @@ 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 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);
+void devm_kfree(struct device *dev, const void *p);
+char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc;
+const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp);
+void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp);
 
-extern unsigned long devm_get_free_pages(struct device *dev,
-					 gfp_t gfp_mask, unsigned int order);
-extern void devm_free_pages(struct device *dev, unsigned long addr);
+unsigned long devm_get_free_pages(struct device *dev,
+				  gfp_t gfp_mask, unsigned int order);
+void devm_free_pages(struct device *dev, unsigned long addr);
 
 void __iomem *devm_ioremap_resource(struct device *dev,
 				    const struct resource *res);

@@ -651,8 +647,7 @@ static inline const char *dev_name(const struct device *dev)
 	return kobject_name(&dev->kobj);
 }
 
-extern __printf(2, 3)
-int dev_set_name(struct device *dev, const char *name, ...);
+__printf(2, 3) int dev_set_name(struct device *dev, const char *name, ...);
 
 #ifdef CONFIG_NUMA
 static inline int dev_to_node(struct device *dev)

@@ -809,39 +804,38 @@ static inline bool dev_has_sync_state(struct device *dev)
 /*
  * High level routines for use by the bus drivers
  */
-extern int __must_check device_register(struct device *dev);
-extern void device_unregister(struct device *dev);
-extern void device_initialize(struct device *dev);
-extern int __must_check device_add(struct device *dev);
-extern void device_del(struct device *dev);
-extern int device_for_each_child(struct device *dev, void *data,
-		     int (*fn)(struct device *dev, void *data));
-extern int device_for_each_child_reverse(struct device *dev, void *data,
-		     int (*fn)(struct device *dev, void *data));
-extern struct device *device_find_child(struct device *dev, void *data,
-				int (*match)(struct device *dev, void *data));
-extern struct device *device_find_child_by_name(struct device *parent,
-						const char *name);
-extern int device_rename(struct device *dev, const char *new_name);
-extern int device_move(struct device *dev, struct device *new_parent,
-		       enum dpm_order dpm_order);
-extern int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid);
-extern const char *device_get_devnode(struct device *dev,
-				      umode_t *mode, kuid_t *uid, kgid_t *gid,
-				      const char **tmp);
+int __must_check device_register(struct device *dev);
+void device_unregister(struct device *dev);
+void device_initialize(struct device *dev);
+int __must_check device_add(struct device *dev);
+void device_del(struct device *dev);
+int device_for_each_child(struct device *dev, void *data,
+			  int (*fn)(struct device *dev, void *data));
+int device_for_each_child_reverse(struct device *dev, void *data,
+				  int (*fn)(struct device *dev, void *data));
+struct device *device_find_child(struct device *dev, void *data,
+				 int (*match)(struct device *dev, void *data));
+struct device *device_find_child_by_name(struct device *parent,
+					 const char *name);
+int device_rename(struct device *dev, const char *new_name);
+int device_move(struct device *dev, struct device *new_parent,
+		enum dpm_order dpm_order);
+int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid);
+const char *device_get_devnode(struct device *dev, umode_t *mode, kuid_t *uid,
+			       kgid_t *gid, const char **tmp);
 
 static inline bool device_supports_offline(struct device *dev)
 {
 	return dev->bus && dev->bus->offline && dev->bus->online;
 }
 
-extern void lock_device_hotplug(void);
-extern void unlock_device_hotplug(void);
-extern int lock_device_hotplug_sysfs(void);
-extern int device_offline(struct device *dev);
-extern int device_online(struct device *dev);
-extern void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
-extern void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+void lock_device_hotplug(void);
+void unlock_device_hotplug(void);
+int lock_device_hotplug_sysfs(void);
+int device_offline(struct device *dev);
+int device_online(struct device *dev);
+void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
 void device_set_of_node_from_dev(struct device *dev, const struct device *dev2);
 
 static inline int dev_num_vf(struct device *dev)

@@ -854,14 +848,13 @@ static inline int dev_num_vf(struct device *dev)
 /*
  * Root device objects for grouping under /sys/devices
  */
-extern struct device *__root_device_register(const char *name,
-					     struct module *owner);
+struct device *__root_device_register(const char *name, struct module *owner);
 
 /* This is a macro to avoid include problems with THIS_MODULE */
 #define root_device_register(name) \
 	__root_device_register(name, THIS_MODULE)
 
-extern void root_device_unregister(struct device *root);
+void root_device_unregister(struct device *root);
 
 static inline void *dev_get_platdata(const struct device *dev)
 {

@@ -872,33 +865,31 @@ static inline void *dev_get_platdata(const struct device *dev)
  * Manual binding of a device to driver. See drivers/base/bus.c
  * for information on use.
  */
-extern int __must_check device_bind_driver(struct device *dev);
-extern void device_release_driver(struct device *dev);
-extern int  __must_check device_attach(struct device *dev);
-extern int __must_check driver_attach(struct device_driver *drv);
-extern void device_initial_probe(struct device *dev);
-extern int __must_check device_reprobe(struct device *dev);
+int __must_check device_bind_driver(struct device *dev);
+void device_release_driver(struct device *dev);
+int  __must_check device_attach(struct device *dev);
+int __must_check driver_attach(struct device_driver *drv);
+void device_initial_probe(struct device *dev);
+int __must_check device_reprobe(struct device *dev);
 
-extern bool device_is_bound(struct device *dev);
+bool device_is_bound(struct device *dev);
 
 /*
  * Easy functions for dynamically creating devices on the fly
  */
-extern __printf(5, 6)
-struct device *device_create(struct class *cls, struct device *parent,
-			     dev_t devt, void *drvdata,
-			     const char *fmt, ...);
-extern __printf(6, 7)
-struct device *device_create_with_groups(struct class *cls,
-			     struct device *parent, dev_t devt, void *drvdata,
-			     const struct attribute_group **groups,
-			     const char *fmt, ...);
-extern void device_destroy(struct class *cls, dev_t devt);
-
-extern int __must_check device_add_groups(struct device *dev,
-					const struct attribute_group **groups);
-extern void device_remove_groups(struct device *dev,
-				 const struct attribute_group **groups);
+__printf(5, 6) struct device *
+device_create(struct class *cls, struct device *parent, dev_t devt,
+	      void *drvdata, const char *fmt, ...);
+__printf(6, 7) struct device *
+device_create_with_groups(struct class *cls, struct device *parent, dev_t devt,
+			  void *drvdata, const struct attribute_group **groups,
+			  const char *fmt, ...);
+void device_destroy(struct class *cls, dev_t devt);
+
+int __must_check device_add_groups(struct device *dev,
+				   const struct attribute_group **groups);
+void device_remove_groups(struct device *dev,
+			  const struct attribute_group **groups);
 
 static inline int __must_check device_add_group(struct device *dev,
 					const struct attribute_group *grp)

@@ -916,14 +907,14 @@ static inline void device_remove_group(struct device *dev,
 	return device_remove_groups(dev, groups);
 }
 
-extern int __must_check devm_device_add_groups(struct device *dev,
+int __must_check devm_device_add_groups(struct device *dev,
 					const struct attribute_group **groups);
-extern void devm_device_remove_groups(struct device *dev,
-				      const struct attribute_group **groups);
-extern int __must_check devm_device_add_group(struct device *dev,
-					const struct attribute_group *grp);
-extern void devm_device_remove_group(struct device *dev,
-				     const struct attribute_group *grp);
+void devm_device_remove_groups(struct device *dev,
+			       const struct attribute_group **groups);
+int __must_check devm_device_add_group(struct device *dev,
+				       const struct attribute_group *grp);
+void devm_device_remove_group(struct device *dev,
+			      const struct attribute_group *grp);
 
 /*
  * Platform "fixup" functions - allow the platform to have their say

@@ -940,21 +931,21 @@ extern int (*platform_notify_remove)(struct device *dev);
  * get_device - atomically increment the reference count for the device.
  *
  */
-extern struct device *get_device(struct device *dev);
-extern void put_device(struct device *dev);
-extern bool kill_device(struct device *dev);
+struct device *get_device(struct device *dev);
+void put_device(struct device *dev);
+bool kill_device(struct device *dev);
 
 #ifdef CONFIG_DEVTMPFS
-extern int devtmpfs_mount(void);
+int devtmpfs_mount(void);
 #else
 static inline int devtmpfs_mount(void) { return 0; }
 #endif
 
 /* drivers/base/power/shutdown.c */
-extern void device_shutdown(void);
+void device_shutdown(void);
 
 /* debugging and troubleshooting/diagnostic helpers. */
-extern const char *dev_driver_string(const struct device *dev);
+const char *dev_driver_string(const struct device *dev);
 
 /* Device links interface. */
 struct device_link *device_link_add(struct device *consumer,
Signature · 1 line
2.26.1
author
#4 lore raw

PATCH v2 4/6 devres: handle zero size in devm_kmalloc()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/base/devres.c +6 -3
index 1df1fb10b2d9..ed615d3b9cf1 100644
@@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
 	struct devres *dr;
 
+	if (unlikely(!size))
+		return ZERO_SIZE_PTR;
+
 	/* use raw alloc_dr for kmalloc caller tracing */
 	dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
 	if (unlikely(!dr))

@@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p)
 	int rc;
 
 	/*
-	 * Special case: pointer to a string in .rodata returned by
-	 * devm_kstrdup_const().
+	 * Special cases: pointer to a string in .rodata returned by
+	 * devm_kstrdup_const() or NULL/ZERO ptr.
 	 */
-	if (unlikely(is_kernel_rodata((unsigned long)p)))
+	if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p)))
 		return;
 
 	rc = devres_destroy(dev, devm_kmalloc_release,
Signature · 1 line
2.26.1
#5 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

Hi Bartosz,

Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/base/devres.c +3 -0
index 1df1fb10b2d9..ed615d3b9cf1 100644
@@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
 	struct devres *dr;
 
+	if (unlikely(!size))
+		return ZERO_SIZE_PTR;
+
 	/* use raw alloc_dr for kmalloc caller tracing */
 	dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
 	if (unlikely(!dr))

@@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p)
 	int rc;
 
 	/*

drivers/base/devres.c void devm_kfree(struct device *dev, const void *p) · line 962

Quoted 10 lines
drivers/base/devres.c +3 -3
-	 * Special case: pointer to a string in .rodata returned by
-	 * devm_kstrdup_const().
+	 * Special cases: pointer to a string in .rodata returned by
+	 * devm_kstrdup_const() or NULL/ZERO ptr.
 	 */
-	if (unlikely(is_kernel_rodata((unsigned long)p)))
+	if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p)))
 		return;
 
 	rc = devres_destroy(dev, devm_kmalloc_release,

This change caught a bug in one of our Tegra drivers, which I am in the process of fixing. Once I bisected to this commit it was easy to track down, but I am wondering if there is any reason why we don't add a WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended up doing to find the bug.

Jon

Signature · 1 line
nvpublic
author
#6 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

Quoted 51 lines · Jon Hunter

Hi Bartosz,

Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/base/devres.c +3 -0
index 1df1fb10b2d9..ed615d3b9cf1 100644
@@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
      struct devres *dr;
 
+     if (unlikely(!size))
+             return ZERO_SIZE_PTR;
+
      /* use raw alloc_dr for kmalloc caller tracing */
      dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
      if (unlikely(!dr))

@@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p)
      int rc;
 
      /*

drivers/base/devres.c void devm_kfree(struct device *dev, const void *p) · line 962

Quoted 10 lines
drivers/base/devres.c +3 -3
-      * Special case: pointer to a string in .rodata returned by
-      * devm_kstrdup_const().
+      * Special cases: pointer to a string in .rodata returned by
+      * devm_kstrdup_const() or NULL/ZERO ptr.
       */
-     if (unlikely(is_kernel_rodata((unsigned long)p)))
+     if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p)))
              return;
 
      rc = devres_destroy(dev, devm_kmalloc_release,

This change caught a bug in one of our Tegra drivers, which I am in the process of fixing. Once I bisected to this commit it was easy to track down, but I am wondering if there is any reason why we don't add a WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended up doing to find the bug.

Quoted 4 lines

Jon

--
nvpublic

Hi Jon,

this is in line with what the regular kmalloc() does. If size is zero, it returns ZERO_SIZE_PTR. It's not an error condition. Actually in user-space malloc() does a similar thing: for size == 0 it allocates one-byte and returns a pointer to it (at least in glibc).

Bartosz

#7 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

Quoted 57 lines · Bartosz Golaszewski
Quoted 51 lines · Jon Hunter

Hi Bartosz,

Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/base/devres.c +3 -0
index 1df1fb10b2d9..ed615d3b9cf1 100644
@@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
      struct devres *dr;
 
+     if (unlikely(!size))
+             return ZERO_SIZE_PTR;
+
      /* use raw alloc_dr for kmalloc caller tracing */
      dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
      if (unlikely(!dr))

@@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p)
      int rc;
 
      /*

drivers/base/devres.c void devm_kfree(struct device *dev, const void *p) · line 962

Quoted 10 lines
drivers/base/devres.c +3 -3
-      * Special case: pointer to a string in .rodata returned by
-      * devm_kstrdup_const().
+      * Special cases: pointer to a string in .rodata returned by
+      * devm_kstrdup_const() or NULL/ZERO ptr.
       */
-     if (unlikely(is_kernel_rodata((unsigned long)p)))
+     if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p)))
              return;
 
      rc = devres_destroy(dev, devm_kmalloc_release,

This change caught a bug in one of our Tegra drivers, which I am in the process of fixing. Once I bisected to this commit it was easy to track down, but I am wondering if there is any reason why we don't add a WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended up doing to find the bug.

Quoted 4 lines

Jon

--
nvpublic

Quoted 6 lines

Hi Jon,

this is in line with what the regular kmalloc() does. If size is zero, it returns ZERO_SIZE_PTR. It's not an error condition. Actually in user-space malloc() does a similar thing: for size == 0 it allocates one-byte and returns a pointer to it (at least in glibc).

Yes that's fine, I was just wondering if there is any reason not to WARN as well?

Cheers
Jon

Signature · 1 line
nvpublic
author
#8 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

Quoted 68 lines · Jon Hunter
Quoted 57 lines · Bartosz Golaszewski
Quoted 51 lines · Jon Hunter

Hi Bartosz,

Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/base/devres.c +3 -0
index 1df1fb10b2d9..ed615d3b9cf1 100644
@@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
      struct devres *dr;
 
+     if (unlikely(!size))
+             return ZERO_SIZE_PTR;
+
      /* use raw alloc_dr for kmalloc caller tracing */
      dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
      if (unlikely(!dr))

@@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p)
      int rc;
 
      /*

drivers/base/devres.c void devm_kfree(struct device *dev, const void *p) · line 962

Quoted 10 lines
drivers/base/devres.c +3 -3
-      * Special case: pointer to a string in .rodata returned by
-      * devm_kstrdup_const().
+      * Special cases: pointer to a string in .rodata returned by
+      * devm_kstrdup_const() or NULL/ZERO ptr.
       */
-     if (unlikely(is_kernel_rodata((unsigned long)p)))
+     if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p)))
              return;
 
      rc = devres_destroy(dev, devm_kmalloc_release,

This change caught a bug in one of our Tegra drivers, which I am in the process of fixing. Once I bisected to this commit it was easy to track down, but I am wondering if there is any reason why we don't add a WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended up doing to find the bug.

Quoted 4 lines

Jon

--
nvpublic

Quoted 6 lines

Hi Jon,

this is in line with what the regular kmalloc() does. If size is zero, it returns ZERO_SIZE_PTR. It's not an error condition. Actually in user-space malloc() does a similar thing: for size == 0 it allocates one-byte and returns a pointer to it (at least in glibc).

Quoted 6 lines

Yes that's fine, I was just wondering if there is any reason not to WARN as well?

Cheers
Jon

Why? Nothing bad happens. Regular kmalloc() doesn't warn, why should devm_kmalloc() do?

Bartosz

#9 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

Quoted 72 lines · Bartosz Golaszewski
Quoted 71 lines · Jon Hunter
Quoted 57 lines · Bartosz Golaszewski
Quoted 51 lines · Jon Hunter

Hi Bartosz,

Quoted 29 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/base/devres.c +3 -0
index 1df1fb10b2d9..ed615d3b9cf1 100644
@@ -819,6 +819,9 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 {
      struct devres *dr;
 
+     if (unlikely(!size))
+             return ZERO_SIZE_PTR;
+
      /* use raw alloc_dr for kmalloc caller tracing */
      dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
      if (unlikely(!dr))

@@ -950,10 +953,10 @@ void devm_kfree(struct device *dev, const void *p)
      int rc;
 
      /*

drivers/base/devres.c void devm_kfree(struct device *dev, const void *p) · line 962

Quoted 10 lines
drivers/base/devres.c +3 -3
-      * Special case: pointer to a string in .rodata returned by
-      * devm_kstrdup_const().
+      * Special cases: pointer to a string in .rodata returned by
+      * devm_kstrdup_const() or NULL/ZERO ptr.
       */
-     if (unlikely(is_kernel_rodata((unsigned long)p)))
+     if (unlikely(is_kernel_rodata((unsigned long)p) || ZERO_OR_NULL_PTR(p)))
              return;
 
      rc = devres_destroy(dev, devm_kmalloc_release,

This change caught a bug in one of our Tegra drivers, which I am in the process of fixing. Once I bisected to this commit it was easy to track down, but I am wondering if there is any reason why we don't add a WARN_ON() if size is 0 in devm_kmalloc? It was essentially what I ended up doing to find the bug.

Quoted 4 lines

Jon

--
nvpublic

Quoted 6 lines

Hi Jon,

this is in line with what the regular kmalloc() does. If size is zero, it returns ZERO_SIZE_PTR. It's not an error condition. Actually in user-space malloc() does a similar thing: for size == 0 it allocates one-byte and returns a pointer to it (at least in glibc).

Yes that's fine, I was just wondering if there is any reason not to WARN as well?

Quoted 6 lines
Quoted 3 lines

Cheers
Jon

Why? Nothing bad happens. Regular kmalloc() doesn't warn, why should devm_kmalloc() do?

Simply because it is easier to track down a bug. In my case the NULL pointer crash did not occur until entering suspend when the memory, that was allocated at probe time, was first actually accessed. So it was not immediately obvious which call to devm_kmalloc caused the problem. Anyway, if kmalloc does not warn either, then fine, it was purely a question.

Jon

Signature · 1 line
nvpublic
#10 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

In reply to Bartosz Golaszewski

Hi Bartosz,

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

This is wrong if you consider devm_krealloc API that you added. The premise of devm_krealloc() is that it does not disturb devres "stack", however in this case there is no entry in the stack. Consider:

	ptr = devm_kzalloc(dev, 0, GFP_KERNEL);
	...
	more devm API calls
	...

	/* This allocation will be on top of devm stack, not bottom ! */
	ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL);

And also:

	ptr = devm_kzalloc(dev, 16, GFP_KERNEL);
	...
	more devm API calls
	...
	/* Here we lose out position */
	ptr = devm_krealloc(dev, ptr, 0, GFP_KERNEL);
	...
	/* and now our memory allocation will be released first */
	ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL);

IMO special-casing 0-size allocations for managed memory allocations should not be done.

Thanks.

Signature · 1 line
Dmitry
author
#11 lore raw

Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()

Quoted 36 lines · Dmitry Torokhov

Hi Bartosz,

Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Make devm_kmalloc() behave similarly to non-managed kmalloc(): return ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle this case.

This is wrong if you consider devm_krealloc API that you added. The premise of devm_krealloc() is that it does not disturb devres "stack", however in this case there is no entry in the stack. Consider:

        ptr = devm_kzalloc(dev, 0, GFP_KERNEL);
        ...
        more devm API calls
        ...

        /* This allocation will be on top of devm stack, not bottom ! */
        ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL);

And also:

        ptr = devm_kzalloc(dev, 16, GFP_KERNEL);
        ...
        more devm API calls
        ...
        /* Here we lose out position */
        ptr = devm_krealloc(dev, ptr, 0, GFP_KERNEL);
        ...
        /* and now our memory allocation will be released first */
        ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL);
Quoted 7 lines

IMO special-casing 0-size allocations for managed memory allocations should not be done.

Thanks.

--
Dmitry

You're right about the ordering being lost. At the same time allocating 0 bytes is quite a special case and should result in returning ZERO_SIZE_PTR as the fault dump resulting from its dereference will indicate what the bug is.

I need to give it a thought because I'm not yet sure what the right solution would be. Let me get back to you.

Bartosz

author
#12 lore raw

PATCH v2 5/6 devres: provide devm_krealloc()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 50 +++++++++++++++++++
 include/linux/device.h                        |  2 +
 3 files changed, 53 insertions(+)
Documentation/driver-api/driver-model/devres.rst +1 -0
index e0b58c392e4f..0a2572c3813c 100644
@@ -352,6 +352,7 @@ MEM
   devm_kfree()
   devm_kmalloc()
   devm_kmalloc_array()
+  devm_krealloc()
   devm_kmemdup()
   devm_kstrdup()
   devm_kvasprintf()
drivers/base/devres.c +50 -0

56 lines not on this page · raw message

include/linux/device.h +2 -0
index 9cadb647cacc..228063c6392c 100644
@@ -201,6 +201,8 @@ int devres_release_group(struct device *dev, void *id);
 
 /* managed devm_k.alloc/kfree for device drivers */
 void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
+void *devm_krealloc(struct device *dev, void *ptr, size_t size,
+		    gfp_t gfp) __must_check;
 __printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
 				     const char *fmt, va_list ap) __malloc;
 __printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,
Signature · 1 line
2.26.1
maintainer
#13 lore raw

Re: [PATCH v2 5/6] devres: provide devm_krealloc()

Quoted 74 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 50 +++++++++++++++++++
 include/linux/device.h                        |  2 +
 3 files changed, 53 insertions(+)
Documentation/driver-api/driver-model/devres.rst +1 -0
index e0b58c392e4f..0a2572c3813c 100644
@@ -352,6 +352,7 @@ MEM
   devm_kfree()
   devm_kmalloc()
   devm_kmalloc_array()
+  devm_krealloc()
   devm_kmemdup()
   devm_kstrdup()
   devm_kvasprintf()
drivers/base/devres.c +39 -0
index ed615d3b9cf1..4b8870ef6a3f 100644
@@ -837,6 +837,56 @@ void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(devm_kmalloc);
 
+void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
+{
+	struct devres *old_dr, *new_dr;
+	struct list_head old_head;
+	unsigned long flags;
+	void *ret = NULL;
+	size_t tot_size;
+
+	if (unlikely(!new_size)) {
+		devm_kfree(dev, ptr);
+		return ZERO_SIZE_PTR;
+	}
+
+	if (unlikely(ZERO_OR_NULL_PTR(ptr)))
+		return devm_kmalloc(dev, new_size, gfp);
+
+	if (WARN_ON(is_kernel_rodata((unsigned long)ptr)))
+		/*
+		 * We cannot reliably realloc a const string returned by
+		 * devm_kstrdup_const().
+		 */
+		return NULL;
+
+	if (!check_dr_size(new_size, &tot_size))
+		return NULL;
+
+	spin_lock_irqsave(&dev->devres_lock, flags);
+
+	old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
+	if (WARN_ON(!old_dr))
+		/* Memory chunk not managed or managed by a different device. */
+		goto out;
+
+	old_head = old_dr->node.entry;
+
+	new_dr = krealloc(old_dr, tot_size, gfp);
+	if (!new_dr)
+		goto out;
+

drivers/base/devres.c void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) · line 888

Quoted 10 lines
drivers/base/devres.c +10 -0
+	if (new_dr != old_dr)
+		list_replace(&old_head, &new_dr->node.entry);
+
+	ret = new_dr->data;
+
+out:
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_krealloc);

That's a lot of logic that does not seem to match up with the krealloc() logic in mm/slab_common.c, are you sure we need to do all of that?

Who wants this?

thanks,

greg k-h

author
#14 lore raw

Re: [PATCH v2 5/6] devres: provide devm_krealloc()

Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by Bartosz Golaszewski <[email protected]>

[snip!]

Quoted 4 lines

That's a lot of logic that does not seem to match up with the krealloc() logic in mm/slab_common.c, are you sure we need to do all of that?

What are you referring to exactly? The check for rodata? It's because devm_kfree() handles this case, while regular kfree() (or krealloc()) doesn't - there's kfree_const() but no devm_kfree_const().

Quoted 1 line

Who wants this?

The hwmon commit I mentioned in my response to patch 6/6 explicitly mentions the lack of this helper.

Bartosz

author
#15 lore raw

Re: [PATCH v2 5/6] devres: provide devm_krealloc()

Quoted 26 lines · Bartosz Golaszewski
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by Bartosz Golaszewski <[email protected]>

[snip!]

Quoted 4 lines

That's a lot of logic that does not seem to match up with the krealloc() logic in mm/slab_common.c, are you sure we need to do all of that?

What are you referring to exactly? The check for rodata? It's because devm_kfree() handles this case, while regular kfree() (or krealloc()) doesn't - there's kfree_const() but no devm_kfree_const().

Quoted 6 lines
Quoted 1 line

Who wants this?

The hwmon commit I mentioned in my response to patch 6/6 explicitly mentions the lack of this helper.

Bartosz

Hi Greg,

As we've established in the discussion under the iio patch that there will in fact be more users of this - can this now be merged too?

Bart

maintainer
#16 lore raw

Re: [PATCH v2 5/6] devres: provide devm_krealloc()

Quoted 32 lines · Bartosz Golaszewski
Quoted 31 lines · Bartosz Golaszewski
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by Bartosz Golaszewski <[email protected]>

[snip!]

Quoted 4 lines

That's a lot of logic that does not seem to match up with the krealloc() logic in mm/slab_common.c, are you sure we need to do all of that?

What are you referring to exactly? The check for rodata? It's because devm_kfree() handles this case, while regular kfree() (or krealloc()) doesn't - there's kfree_const() but no devm_kfree_const().

Quoted 1 line

Who wants this?

The hwmon commit I mentioned in my response to patch 6/6 explicitly mentions the lack of this helper.

Quoted 6 lines
Quoted 1 line

Bartosz

Hi Greg,

As we've established in the discussion under the iio patch that there will in fact be more users of this - can this now be merged too?

Sorry, it's still in my queue, along with 500+ other patches :)

it's not lost...

thanks,

greg k-h

maintainer
#17 lore raw

Re: [PATCH v2 5/6] devres: provide devm_krealloc()

In reply to Bartosz Golaszewski
Quoted 32 lines · Bartosz Golaszewski
Quoted 31 lines · Bartosz Golaszewski
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).

Managed realloc'ed chunks can be manually released with devm_kfree().

Signed-off-by Bartosz Golaszewski <[email protected]>

[snip!]

Quoted 4 lines

That's a lot of logic that does not seem to match up with the krealloc() logic in mm/slab_common.c, are you sure we need to do all of that?

What are you referring to exactly? The check for rodata? It's because devm_kfree() handles this case, while regular kfree() (or krealloc()) doesn't - there's kfree_const() but no devm_kfree_const().

Quoted 1 line

Who wants this?

The hwmon commit I mentioned in my response to patch 6/6 explicitly mentions the lack of this helper.

Quoted 6 lines
Quoted 1 line

Bartosz

Hi Greg,

As we've established in the discussion under the iio patch that there will in fact be more users of this - can this now be merged too?

Can you resend the remaining patches, along with the users, and I'll review it? I just worry about having to duplicate all of that mm code in there that it will grow stale over time...

thanks,

greg k-h

author
#18 lore raw

PATCH v2 6/6 hwmon: pmbus: use more devres helpers

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Shrink pmbus code by using devm_hwmon_device_register_with_groups() and devm_krealloc() instead of their non-managed variants.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/hwmon/pmbus/pmbus_core.c +9 -19
index a420877ba533..225d0ac162c7 100644
@@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
 		int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
-		void *new_attrs = krealloc(data->group.attrs,
-					   new_max_attrs * sizeof(void *),
-					   GFP_KERNEL);
+		void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
+						new_max_attrs * sizeof(void *),
+						GFP_KERNEL);
 		if (!new_attrs)
 			return -ENOMEM;
 		data->group.attrs = new_attrs;

@@ -2538,7 +2538,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 
 	ret = pmbus_find_attributes(client, data);
 	if (ret)
-		goto out_kfree;
+		return ret;
 
 	/*
 	 * If there are no attributes, something is wrong.

@@ -2546,35 +2546,27 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	 */
 	if (!data->num_attributes) {
 		dev_err(dev, "No attributes found\n");
-		ret = -ENODEV;
-		goto out_kfree;
+		return -ENODEV;
 	}
 
 	data->groups[0] = &data->group;
 	memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
-	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
-							    data, data->groups);
+	data->hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+					client->name, data, data->groups);
 	if (IS_ERR(data->hwmon_dev)) {
-		ret = PTR_ERR(data->hwmon_dev);
 		dev_err(dev, "Failed to register hwmon device\n");
-		goto out_kfree;
+		return PTR_ERR(data->hwmon_dev);
 	}
 
 	ret = pmbus_regulator_register(data);
 	if (ret)
-		goto out_unregister;
+		return ret;
 
 	ret = pmbus_init_debugfs(client, data);
 	if (ret)
 		dev_warn(dev, "Failed to register debugfs\n");
 
 	return 0;
-
-out_unregister:
-	hwmon_device_unregister(data->hwmon_dev);
-out_kfree:
-	kfree(data->group.attrs);
-	return ret;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_probe);
 

@@ -2584,8 +2576,6 @@ int pmbus_do_remove(struct i2c_client *client)
 
 	debugfs_remove_recursive(data->debugfs);
 
-	hwmon_device_unregister(data->hwmon_dev);
-	kfree(data->group.attrs);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_remove);
Signature · 1 line
2.26.1
maintainer
#20 lore raw

Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers

In reply to Bartosz Golaszewski
Quoted 15 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Shrink pmbus code by using devm_hwmon_device_register_with_groups() and devm_krealloc() instead of their non-managed variants.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/hwmon/pmbus/pmbus_core.c +0 -0
index a420877ba533..225d0ac162c7 100644

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) · line 1027

Quoted 9 lines
drivers/hwmon/pmbus/pmbus_core.c +3 -3
@@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
 		int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
-		void *new_attrs = krealloc(data->group.attrs,
-					   new_max_attrs * sizeof(void *),
-					   GFP_KERNEL);
+		void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
+						new_max_attrs * sizeof(void *),
+						GFP_KERNEL);

dynamic sysfs attributes in a devm-allocated chunk of memory? What could go wrong...

Anyway, is this the only in-kernel user that you could find for this function? If so, it feels like it's a lot of extra work for no real gain.

thanks,

greg k-h

author
#21 lore raw

Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers

Quoted 21 lines · Greg Kroah-Hartman
Quoted 19 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Shrink pmbus code by using devm_hwmon_device_register_with_groups() and devm_krealloc() instead of their non-managed variants.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/hwmon/pmbus/pmbus_core.c +0 -1
index a420877ba533..225d0ac162c7 100644
@@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
      if (data->num_attributes >= data->max_attributes - 1) {
              int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
-             void *new_attrs = krealloc(data->group.attrs,
Quoted 9 lines

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) · line 1027

Quoted 5 lines
drivers/hwmon/pmbus/pmbus_core.c +3 -2
-                                        new_max_attrs * sizeof(void *),
-                                        GFP_KERNEL);
+             void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
+                                             new_max_attrs * sizeof(void *),
+                                             GFP_KERNEL);

dynamic sysfs attributes in a devm-allocated chunk of memory? What could go wrong...

So what *can* go wrong, which it couldn't before this patch? The drivers in this directory kfree() this memory anyway on driver detach. Using devm here is equivalent to the previous behavior - only that the memory is freed after remove() not inside it.

Quoted 4 lines

Anyway, is this the only in-kernel user that you could find for this function? If so, it feels like it's a lot of extra work for no real gain.

No. There are around 100 calls to krealloc() in drivers/. I assume that at least half of these are called with an attached struct device. I chose this driver, because it has a commit in its history that explicitly says that it would use devm_krealloc() if it were available (commit 85cfb3a83536 ("hwmon: (pmbus) Use krealloc to allocate attribute memory"). I didn't want to spend a lot of time on converting other users in case this patch gets rejected.

Bartosz

maintainer
#22 lore raw

Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers

In reply to Greg Kroah-Hartman
Quoted 20 lines · Greg Kroah-Hartman
Quoted 19 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Shrink pmbus code by using devm_hwmon_device_register_with_groups() and devm_krealloc() instead of their non-managed variants.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
drivers/hwmon/pmbus/pmbus_core.c +0 -1
index a420877ba533..225d0ac162c7 100644
@@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
 		int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE;
-		void *new_attrs = krealloc(data->group.attrs,
Quoted 9 lines

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) · line 1027

Quoted 5 lines
drivers/hwmon/pmbus/pmbus_core.c +3 -2
-					   new_max_attrs * sizeof(void *),
-					   GFP_KERNEL);
+		void *new_attrs = devm_krealloc(data->dev, data->group.attrs,
+						new_max_attrs * sizeof(void *),
+						GFP_KERNEL);

dynamic sysfs attributes in a devm-allocated chunk of memory? What could go wrong...

You mean that the memory might be removed before the attributes are removed ? Hmm, but that isn't different to the current implementation. The hwmon device is removed first (removing the sysfs attributes), followed by the kfree. Are you saying this is not safe ? Pretty much all code which allocates memory for struct attribute is doing the same, so that would be a problem throughout the kernel.

Quoted 4 lines

Anyway, is this the only in-kernel user that you could find for this function? If so, it feels like it's a lot of extra work for no real gain.

And I was so happy that I'd be able to get rid of pmbus_do_remove() subsequently. But then I can also use devm_add_action() and have it call kfree(), with the same result. Given that, if hwmon would really be the only user, we can live without it.

Guenter

maintainer
#23 lore raw
In reply to Bartosz Golaszewski
Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Regular krealloc() obviously can't work with managed memory. This series implements devm_krealloc() and adds the first user with hope that this helper will be adopted by other drivers currently using non-managed krealloc().

Some additional changes to the code modified by main patches are included.

I've applied the first 4, the other two I had questions on.

thanks,

greg k-h