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(-)
--
2.26.1
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(-)
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
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(-)
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
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(-)
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
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(-)
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
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(-)
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
On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote:
> 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(-)
>
> 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
On Mon, Jun 29, 2020 at 08:50:02AM +0200, Bartosz Golaszewski wrote:
> 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
On Thu, Jul 2, 2020 at 2:44 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote:
> > 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(-)
> >
> > 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
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 <[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(-)
>>
>> 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
Hi Bartosz,
On 29/06/2020 07:50, Bartosz Golaszewski wrote:
> 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(-)
>
> 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
On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <[email protected]> wrote:
>
> Hi Bartosz,
>
> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
> > 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(-)
> >
> > 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
On 10/07/2020 17:03, Bartosz Golaszewski wrote:
> On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <[email protected]> wrote:
>>
>> Hi Bartosz,
>>
>> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
>>> 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(-)
>>>
>>> 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
On Fri, Jul 10, 2020 at 6:11 PM Jon Hunter <[email protected]> wrote:
>
>
> On 10/07/2020 17:03, Bartosz Golaszewski wrote:
> > On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <[email protected]> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
> >>> 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(-)
> >>>
> >>> 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
On 10/07/2020 17:24, Bartosz Golaszewski wrote:
> On Fri, Jul 10, 2020 at 6:11 PM Jon Hunter <[email protected]> wrote:
>>
>>
>> On 10/07/2020 17:03, Bartosz Golaszewski wrote:
>>> On Fri, Jul 10, 2020 at 3:46 PM Jon Hunter <[email protected]> wrote:
>>>>
>>>> Hi Bartosz,
>>>>
>>>> On 29/06/2020 07:50, Bartosz Golaszewski wrote:
>>>>> 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(-)
>>>>>
>>>>> 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
Hi Bartosz,
On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski <[email protected]> wrote:
>
> 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.
--
Dmitry
On Sun, Apr 11, 2021 at 5:21 AM Dmitry Torokhov
<[email protected]> wrote:
>
> Hi Bartosz,
>
> On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > 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.
>
> --
> 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