All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] devres: provide and use devm_krealloc()
@ 2020-06-29  6:50 Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 1/6] devres: remove stray space from devm_kmalloc() definition Bartosz Golaszewski
                   ` (6 more replies)
  0 siblings, 7 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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(-)

-- 
2.26.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/6] devres: remove stray space from devm_kmalloc() definition
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
@ 2020-06-29  6:50 ` Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 2/6] devres: move the size check from alloc_dr() into a separate function Bartosz Golaszewski
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Use the preferred coding style for functions returning pointers.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/base/devres.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 0bbb328bd17f..c34327219c34 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -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;
 
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/6] devres: move the size check from alloc_dr() into a separate function
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 1/6] devres: remove stray space from devm_kmalloc() definition Bartosz Golaszewski
@ 2020-06-29  6:50 ` Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 3/6] device: remove 'extern' attribute from function prototypes in device.h Bartosz Golaszewski
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/base/devres.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index c34327219c34..1df1fb10b2d9 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -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);
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 3/6] device: remove 'extern' attribute from function prototypes in device.h
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 1/6] devres: remove stray space from devm_kmalloc() definition Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 2/6] devres: move the size check from alloc_dr() into a separate function Bartosz Golaszewski
@ 2020-06-29  6:50 ` Bartosz Golaszewski
  2020-06-29  6:50 ` [PATCH v2 4/6] devres: handle zero size in devm_kmalloc() Bartosz Golaszewski
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 include/linux/device.h | 223 ++++++++++++++++++++---------------------
 1 file changed, 107 insertions(+), 116 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..9cadb647cacc 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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,
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
                   ` (2 preceding siblings ...)
  2020-06-29  6:50 ` [PATCH v2 3/6] device: remove 'extern' attribute from function prototypes in device.h Bartosz Golaszewski
@ 2020-06-29  6:50 ` Bartosz Golaszewski
  2020-07-10 13:46   ` Jon Hunter
  2021-04-11  3:21   ` Dmitry Torokhov
  2020-06-29  6:50 ` [PATCH v2 5/6] devres: provide devm_krealloc() Bartosz Golaszewski
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/base/devres.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 1df1fb10b2d9..ed615d3b9cf1 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -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,
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 5/6] devres: provide devm_krealloc()
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
                   ` (3 preceding siblings ...)
  2020-06-29  6:50 ` [PATCH v2 4/6] devres: handle zero size in devm_kmalloc() Bartosz Golaszewski
@ 2020-06-29  6:50 ` Bartosz Golaszewski
  2020-07-02 12:42   ` Greg Kroah-Hartman
  2020-06-29  6:50 ` [PATCH v2 6/6] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
  2020-07-02 12:44 ` [PATCH v2 0/6] devres: provide and use devm_krealloc() Greg Kroah-Hartman
  6 siblings, 1 reply; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 50 +++++++++++++++++++
 include/linux/device.h                        |  2 +
 3 files changed, 53 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index e0b58c392e4f..0a2572c3813c 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -352,6 +352,7 @@ MEM
   devm_kfree()
   devm_kmalloc()
   devm_kmalloc_array()
+  devm_krealloc()
   devm_kmemdup()
   devm_kstrdup()
   devm_kvasprintf()
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index ed615d3b9cf1..4b8870ef6a3f 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -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;
+
+	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);
+
 /**
  * devm_kstrdup - Allocate resource managed space and
  *                copy an existing string into that.
diff --git a/include/linux/device.h b/include/linux/device.h
index 9cadb647cacc..228063c6392c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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,
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 6/6] hwmon: pmbus: use more devres helpers
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
                   ` (4 preceding siblings ...)
  2020-06-29  6:50 ` [PATCH v2 5/6] devres: provide devm_krealloc() Bartosz Golaszewski
@ 2020-06-29  6:50 ` Bartosz Golaszewski
  2020-06-29 16:32   ` Guenter Roeck
  2020-07-02 12:44   ` Greg Kroah-Hartman
  2020-07-02 12:44 ` [PATCH v2 0/6] devres: provide and use devm_krealloc() Greg Kroah-Hartman
  6 siblings, 2 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-06-29  6:50 UTC (permalink / raw)
  To: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index a420877ba533..225d0ac162c7 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -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);
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers
  2020-06-29  6:50 ` [PATCH v2 6/6] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
@ 2020-06-29 16:32   ` Guenter Roeck
  2020-07-02 12:44   ` Greg Kroah-Hartman
  1 sibling, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2020-06-29 16:32 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Jean Delvare, linux-doc, linux-kernel, linux-hwmon,
	Bartosz Golaszewski

On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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 <bgolaszewski@baylibre.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 5/6] devres: provide devm_krealloc()
  2020-06-29  6:50 ` [PATCH v2 5/6] devres: provide devm_krealloc() Bartosz Golaszewski
@ 2020-07-02 12:42   ` Greg Kroah-Hartman
  2020-07-02 13:11     ` Bartosz Golaszewski
  0 siblings, 1 reply; 24+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-02 12:42 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

On Mon, Jun 29, 2020 at 08:50:07AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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 <bgolaszewski@baylibre.com>
> ---
>  .../driver-api/driver-model/devres.rst        |  1 +
>  drivers/base/devres.c                         | 50 +++++++++++++++++++
>  include/linux/device.h                        |  2 +
>  3 files changed, 53 insertions(+)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index e0b58c392e4f..0a2572c3813c 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -352,6 +352,7 @@ MEM
>    devm_kfree()
>    devm_kmalloc()
>    devm_kmalloc_array()
> +  devm_krealloc()
>    devm_kmemdup()
>    devm_kstrdup()
>    devm_kvasprintf()
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index ed615d3b9cf1..4b8870ef6a3f 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -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;
> +
> +	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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers
  2020-06-29  6:50 ` [PATCH v2 6/6] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
  2020-06-29 16:32   ` Guenter Roeck
@ 2020-07-02 12:44   ` Greg Kroah-Hartman
  2020-07-02 13:06     ` Bartosz Golaszewski
  2020-07-02 14:29     ` Guenter Roeck
  1 sibling, 2 replies; 24+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-02 12:44 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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 <bgolaszewski@baylibre.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index a420877ba533..225d0ac162c7 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 0/6] devres: provide and use devm_krealloc()
  2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
                   ` (5 preceding siblings ...)
  2020-06-29  6:50 ` [PATCH v2 6/6] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
@ 2020-07-02 12:44 ` Greg Kroah-Hartman
  6 siblings, 0 replies; 24+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-02 12:44 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

On Mon, Jun 29, 2020 at 08:50:02AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers
  2020-07-02 12:44   ` Greg Kroah-Hartman
@ 2020-07-02 13:06     ` Bartosz Golaszewski
  2020-07-02 14:29     ` Guenter Roeck
  1 sibling, 0 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-07-02 13:06 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, Linux Kernel Mailing List, linux-hwmon,
	Bartosz Golaszewski

On Thu, Jul 2, 2020 at 2:44 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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 <bgolaszewski@baylibre.com>
> > ---
> >  drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
> >  1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index a420877ba533..225d0ac162c7 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -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...
>

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.

> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 5/6] devres: provide devm_krealloc()
  2020-07-02 12:42   ` Greg Kroah-Hartman
@ 2020-07-02 13:11     ` Bartosz Golaszewski
  2020-07-06 16:38       ` Bartosz Golaszewski
  0 siblings, 1 reply; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-07-02 13:11 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, Linux Kernel Mailing List, linux-hwmon,
	Bartosz Golaszewski

On Thu, Jul 2, 2020 at 2:42 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 29, 2020 at 08:50:07AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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 <bgolaszewski@baylibre.com>

[snip!]

>
> 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().

> Who wants this?

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

Bartosz

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 6/6] hwmon: pmbus: use more devres helpers
  2020-07-02 12:44   ` Greg Kroah-Hartman
  2020-07-02 13:06     ` Bartosz Golaszewski
@ 2020-07-02 14:29     ` Guenter Roeck
  1 sibling, 0 replies; 24+ messages in thread
From: Guenter Roeck @ 2020-07-02 14:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Bartosz Golaszewski
  Cc: Jonathan Corbet, Rafael J . Wysocki, Jean Delvare, linux-doc,
	linux-kernel, linux-hwmon, Bartosz Golaszewski

On 7/2/20 5:44 AM, Greg Kroah-Hartman wrote:
> On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> 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 <bgolaszewski@baylibre.com>
>> ---
>>  drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index a420877ba533..225d0ac162c7 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -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...
> 

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.

> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 5/6] devres: provide devm_krealloc()
  2020-07-02 13:11     ` Bartosz Golaszewski
@ 2020-07-06 16:38       ` Bartosz Golaszewski
  2020-07-06 16:41         ` Greg Kroah-Hartman
  2020-07-10 13:32         ` Greg Kroah-Hartman
  0 siblings, 2 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-07-06 16:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, Linux Kernel Mailing List, linux-hwmon,
	Bartosz Golaszewski

On Thu, Jul 2, 2020 at 3:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Jul 2, 2020 at 2:42 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jun 29, 2020 at 08:50:07AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > 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 <bgolaszewski@baylibre.com>
>
> [snip!]
>
> >
> > 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().
>
> > 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 5/6] devres: provide devm_krealloc()
  2020-07-06 16:38       ` Bartosz Golaszewski
@ 2020-07-06 16:41         ` Greg Kroah-Hartman
  2020-07-10 13:32         ` Greg Kroah-Hartman
  1 sibling, 0 replies; 24+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-06 16:41 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, Linux Kernel Mailing List, linux-hwmon,
	Bartosz Golaszewski

On Mon, Jul 06, 2020 at 06:38:10PM +0200, Bartosz Golaszewski wrote:
> On Thu, Jul 2, 2020 at 3:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Thu, Jul 2, 2020 at 2:42 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 08:50:07AM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > 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 <bgolaszewski@baylibre.com>
> >
> > [snip!]
> >
> > >
> > > 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().
> >
> > > 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?

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

it's not lost...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 5/6] devres: provide devm_krealloc()
  2020-07-06 16:38       ` Bartosz Golaszewski
  2020-07-06 16:41         ` Greg Kroah-Hartman
@ 2020-07-10 13:32         ` Greg Kroah-Hartman
  1 sibling, 0 replies; 24+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-10 13:32 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Rafael J . Wysocki, Guenter Roeck, Jean Delvare,
	linux-doc, Linux Kernel Mailing List, linux-hwmon,
	Bartosz Golaszewski

On Mon, Jul 06, 2020 at 06:38:10PM +0200, Bartosz Golaszewski wrote:
> On Thu, Jul 2, 2020 at 3:11 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Thu, Jul 2, 2020 at 2:42 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 08:50:07AM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > 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 <bgolaszewski@baylibre.com>
> >
> > [snip!]
> >
> > >
> > > 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().
> >
> > > 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?

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-06-29  6:50 ` [PATCH v2 4/6] devres: handle zero size in devm_kmalloc() Bartosz Golaszewski
@ 2020-07-10 13:46   ` Jon Hunter
  2020-07-10 16:03     ` Bartosz Golaszewski
  2021-04-11  3:21   ` Dmitry Torokhov
  1 sibling, 1 reply; 24+ messages in thread
From: Jon Hunter @ 2020-07-10 13:46 UTC (permalink / raw)
  To: Bartosz Golaszewski, Jonathan Corbet, Greg Kroah-Hartman,
	Rafael J . Wysocki, Guenter Roeck, Jean Delvare
  Cc: linux-doc, linux-kernel, linux-hwmon, Bartosz Golaszewski

Hi Bartosz,

On 29/06/2020 07:50, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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 <bgolaszewski@baylibre.com>
> ---
>  drivers/base/devres.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 1df1fb10b2d9..ed615d3b9cf1 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -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,


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

-- 
nvpublic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-07-10 13:46   ` Jon Hunter
@ 2020-07-10 16:03     ` Bartosz Golaszewski
  2020-07-10 16:11       ` Jon Hunter
  0 siblings, 1 reply; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-07-10 16:03 UTC (permalink / raw)
  To: Jon Hunter
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare, linux-doc,
	Linux Kernel Mailing List, linux-hwmon, Bartosz Golaszewski

On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Bartosz,
>
> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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 <bgolaszewski@baylibre.com>
> > ---
> >  drivers/base/devres.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index 1df1fb10b2d9..ed615d3b9cf1 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -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,
>
>
> 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
>
> --
> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-07-10 16:03     ` Bartosz Golaszewski
@ 2020-07-10 16:11       ` Jon Hunter
  2020-07-10 16:24         ` Bartosz Golaszewski
  0 siblings, 1 reply; 24+ messages in thread
From: Jon Hunter @ 2020-07-10 16:11 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare, linux-doc,
	Linux Kernel Mailing List, linux-hwmon, Bartosz Golaszewski


On 10/07/2020 17:03, Bartosz Golaszewski wrote:
> On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Hi Bartosz,
>>
>> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> 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 <bgolaszewski@baylibre.com>
>>> ---
>>>  drivers/base/devres.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>>> index 1df1fb10b2d9..ed615d3b9cf1 100644
>>> --- a/drivers/base/devres.c
>>> +++ b/drivers/base/devres.c
>>> @@ -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,
>>
>>
>> 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
>>
>> --
>> 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).


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

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-07-10 16:11       ` Jon Hunter
@ 2020-07-10 16:24         ` Bartosz Golaszewski
  2020-07-10 16:30           ` Jon Hunter
  0 siblings, 1 reply; 24+ messages in thread
From: Bartosz Golaszewski @ 2020-07-10 16:24 UTC (permalink / raw)
  To: Jon Hunter
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare, linux-doc,
	Linux Kernel Mailing List, linux-hwmon, Bartosz Golaszewski

On Fri, Jul 10, 2020 at 6:11 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 10/07/2020 17:03, Bartosz Golaszewski wrote:
> > On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>
> >>> 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 <bgolaszewski@baylibre.com>
> >>> ---
> >>>  drivers/base/devres.c | 9 ++++++---
> >>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> >>> index 1df1fb10b2d9..ed615d3b9cf1 100644
> >>> --- a/drivers/base/devres.c
> >>> +++ b/drivers/base/devres.c
> >>> @@ -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,
> >>
> >>
> >> 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
> >>
> >> --
> >> 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).
>
>
> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-07-10 16:24         ` Bartosz Golaszewski
@ 2020-07-10 16:30           ` Jon Hunter
  0 siblings, 0 replies; 24+ messages in thread
From: Jon Hunter @ 2020-07-10 16:30 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare, linux-doc,
	Linux Kernel Mailing List, linux-hwmon, Bartosz Golaszewski


On 10/07/2020 17:24, Bartosz Golaszewski wrote:
> On Fri, Jul 10, 2020 at 6:11 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 10/07/2020 17:03, Bartosz Golaszewski wrote:
>>> On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>
>>>> Hi Bartosz,
>>>>
>>>> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
>>>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>>>
>>>>> 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 <bgolaszewski@baylibre.com>
>>>>> ---
>>>>>  drivers/base/devres.c | 9 ++++++---
>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>>>>> index 1df1fb10b2d9..ed615d3b9cf1 100644
>>>>> --- a/drivers/base/devres.c
>>>>> +++ b/drivers/base/devres.c
>>>>> @@ -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,
>>>>
>>>>
>>>> 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
>>>>
>>>> --
>>>> 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).
>>
>>
>> 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?


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

-- 
nvpublic

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2020-06-29  6:50 ` [PATCH v2 4/6] devres: handle zero size in devm_kmalloc() Bartosz Golaszewski
  2020-07-10 13:46   ` Jon Hunter
@ 2021-04-11  3:21   ` Dmitry Torokhov
  2021-04-12 19:23     ` Bartosz Golaszewski
  1 sibling, 1 reply; 24+ messages in thread
From: Dmitry Torokhov @ 2021-04-11  3:21 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare, Linux Doc Mailing List, lkml,
	linux-hwmon, Bartosz Golaszewski

Hi Bartosz,

On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
  2021-04-11  3:21   ` Dmitry Torokhov
@ 2021-04-12 19:23     ` Bartosz Golaszewski
  0 siblings, 0 replies; 24+ messages in thread
From: Bartosz Golaszewski @ 2021-04-12 19:23 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jonathan Corbet, Greg Kroah-Hartman, Rafael J . Wysocki,
	Guenter Roeck, Jean Delvare, Linux Doc Mailing List, lkml,
	linux-hwmon, Bartosz Golaszewski

On Sun, Apr 11, 2021 at 5:21 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Bartosz,
>
> On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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.
>
> --
> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-04-12 19:23 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29  6:50 [PATCH v2 0/6] devres: provide and use devm_krealloc() Bartosz Golaszewski
2020-06-29  6:50 ` [PATCH v2 1/6] devres: remove stray space from devm_kmalloc() definition Bartosz Golaszewski
2020-06-29  6:50 ` [PATCH v2 2/6] devres: move the size check from alloc_dr() into a separate function Bartosz Golaszewski
2020-06-29  6:50 ` [PATCH v2 3/6] device: remove 'extern' attribute from function prototypes in device.h Bartosz Golaszewski
2020-06-29  6:50 ` [PATCH v2 4/6] devres: handle zero size in devm_kmalloc() Bartosz Golaszewski
2020-07-10 13:46   ` Jon Hunter
2020-07-10 16:03     ` Bartosz Golaszewski
2020-07-10 16:11       ` Jon Hunter
2020-07-10 16:24         ` Bartosz Golaszewski
2020-07-10 16:30           ` Jon Hunter
2021-04-11  3:21   ` Dmitry Torokhov
2021-04-12 19:23     ` Bartosz Golaszewski
2020-06-29  6:50 ` [PATCH v2 5/6] devres: provide devm_krealloc() Bartosz Golaszewski
2020-07-02 12:42   ` Greg Kroah-Hartman
2020-07-02 13:11     ` Bartosz Golaszewski
2020-07-06 16:38       ` Bartosz Golaszewski
2020-07-06 16:41         ` Greg Kroah-Hartman
2020-07-10 13:32         ` Greg Kroah-Hartman
2020-06-29  6:50 ` [PATCH v2 6/6] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
2020-06-29 16:32   ` Guenter Roeck
2020-07-02 12:44   ` Greg Kroah-Hartman
2020-07-02 13:06     ` Bartosz Golaszewski
2020-07-02 14:29     ` Guenter Roeck
2020-07-02 12:44 ` [PATCH v2 0/6] devres: provide and use devm_krealloc() Greg Kroah-Hartman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.