devres: provide and use devm_krealloc()

PATCH Superseded Bartosz Golaszewski 3 participants 6 messages Aug 2, 2020 – Aug 3, 2020

Subsystems (5)

Documentation IIO Driver core Hardware monitoring +1 more subsystems

lore

Share & tools

lore

Bartosz Golaszewski

author

2020-08-02 08:35 UTC #0 lore raw

v6 cover devres: provide and use devm_krealloc()

From: Bartosz Golaszewski <[email protected]>

Hi Greg, Andy,

this is a reworked version of devm_krealloc(). This time without the risk of sleeping with spinlock taken. I noticed that devres_lock is only there to protect the integrity of the devres list - we can assume users are sane enough to not devm_kfree() chunks that they devm_krealloc() at the same time so we only take the lock when manipulating the list now.

I dropped Andy's review tag due to this change.

===

Regular krealloc() obviously can't work with managed memory. This series implements devm_krealloc() and adds the first users 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()

v2 -> v3:
- drop already applied patches
- collect Acks
- add an additional user in iio

v3 -> v4:
- add the kerneldoc for devm_krealloc()
- WARN() outside of spinlock
- rename local variable

v4 -> v5:
- tweak the kerneldoc

v5 -> v6:
- tweak the devres_lock handling in devm_krealloc()

Bartosz Golaszewski (3):
devres: provide devm_krealloc()
hwmon: pmbus: use more devres helpers
iio: adc: xilinx-xadc: use devm_krealloc()

 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 65 +++++++++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c              | 28 +++-----
 drivers/iio/adc/xilinx-xadc-core.c            | 16 ++---
 include/linux/device.h                        |  2 +
 5 files changed, 85 insertions(+), 27 deletions(-)

Signature · 1 line

2.26.1

Bartosz Golaszewski

author

2020-08-02 08:35 UTC #1 lore raw

PATCH v6 1/3 devres: provide devm_krealloc()

From: Bartosz Golaszewski <[email protected]>

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

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

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

---
 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/base/devres.c                         | 65 +++++++++++++++++++
 include/linux/device.h                        |  2 +
 3 files changed, 68 insertions(+)

Documentation/driver-api/driver-model/devres.rst +1 -0

index eaaaafc21134..f318a5c0033c 100644
@@ -354,6 +354,7 @@ MEM
   devm_kmalloc()
   devm_kmalloc_array()
   devm_kmemdup()
+  devm_krealloc()
   devm_kstrdup()
   devm_kvasprintf()
   devm_kzalloc()

drivers/base/devres.c +65 -0

71 lines not on this page · raw message

include/linux/device.h +2 -0

index 79ce404619e6..51fb33400d7a 100644
@@ -206,6 +206,8 @@ int devres_release_group(struct device *dev, void *id);
 
 /* managed devm_k.alloc/kfree for device drivers */
 void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
+void *devm_krealloc(struct device *dev, void *ptr, size_t size,
+		    gfp_t gfp) __must_check;
 __printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
 				     const char *fmt, va_list ap) __malloc;
 __printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,

Signature · 1 line

2.26.1

Andy Shevchenko

2020-08-02 10:42 UTC #2 lore raw

Re: [PATCH v6 1/3] devres: provide devm_krealloc()

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
Implement the managed variant of krealloc(). This function works with all memory allocated by devm_kmalloc() (or devres functions using it implicitly like devm_kmemdup(), devm_kstrdup() etc.).
Managed realloc'ed chunks can be manually released with devm_kfree().

Some thoughts below. You may ignore nit-picks, of course :-)

...

Quoted 4 lines

+ * Managed krealloc(). Resizes the memory chunk allocated with devm_kmalloc().
+ * Behaves similarly to regular krealloc(): if @ptr is NULL or ZERO_SIZE_PTR,
+ * it's the equivalent of devm_kmalloc(). If new_size is zero, it frees the

equivalent for

Quoted 5 lines

+ * previously allocated memory and returns ZERO_SIZE_PTR. This function doesn't
+ * change the order in which the release callback for the re-alloc'ed devres
+ * will be called (except when falling back to devm_kmalloc() or when freeing
+ * resources when new_size is zero). The contents of the memory are preserved
+ * up to the lesser of new and old sizes.

Might deserve to say about pointers to RO, but see below.

...

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

Quoted 6 lines

drivers/base/devres.c +6 -0

+       if (WARN_ON(is_kernel_rodata((unsigned long)ptr)))
+               /*
+                * We cannot reliably realloc a const string returned by
+                * devm_kstrdup_const().
+                */
+               return NULL;

I was thinking about this bit... Shouldn't we rather issue a simple dev_warn() and return the existing pointer?
For example in some cases we might want to have resources coming either from heap or from constant. Then, if at some circumstances we would like to extend that memory (only for non-constant cases) we would need to manage this ourselves. Otherwise we may simply call krealloc().
It seems that devm_kstrdup_const returns an initial pointer. Getting NULL is kinda inconvenient (and actually dev_warn() might also be quite a noise, however I would give a message to the user, because it's something worth checking).

...

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

Quoted 5 lines

drivers/base/devres.c +4 -0

+       spin_lock_irqsave(&dev->devres_lock, flags);
+       old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
+       spin_unlock_irqrestore(&dev->devres_lock, flags);
 
+       if (!old_dr) {

I would have this under spin lock b/c of below.

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

Quoted 5 lines

drivers/base/devres.c +4 -0

+               WARN(1, "Memory chunk not managed or managed by a different device.");
+               return NULL;
+       }
 
+       old_head = old_dr->node.entry;

This would be still better to be under spin lock.

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

Quoted 3 lines

drivers/base/devres.c +3 -0

+       new_dr = krealloc(old_dr, total_size, gfp);
+       if (!new_dr)
+               return NULL;

And perhaps spin lock taken already here.

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

Quoted 5 lines

drivers/base/devres.c +5 -0

+       if (new_dr != old_dr) {
+               spin_lock_irqsave(&dev->devres_lock, flags);
+               list_replace(&old_head, &new_dr->node.entry);
+               spin_unlock_irqrestore(&dev->devres_lock, flags);
+       }

Yes, I understand that covering more code under spin lock does not fix any potential race, but at least it minimizes scope of the code that is not under it to see exactly what is problematic.

I probably will think more about a better approach to avoid potential races.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

2020-08-03 19:31 UTC #3 lore raw

Re: [PATCH v6 1/3] devres: provide devm_krealloc()

Quoted 1 line · Andy Shevchenko

[snip]

Quoted 13 lines

I was thinking about this bit... Shouldn't we rather issue a simple dev_warn() and return the existing pointer?
For example in some cases we might want to have resources coming either from heap or from constant. Then, if at some circumstances we would like to extend that memory (only for non-constant cases) we would need to manage this ourselves. Otherwise we may simply call krealloc().
It seems that devm_kstrdup_const returns an initial pointer. Getting NULL is kinda inconvenient (and actually dev_warn() might also be quite a noise, however I would give a message to the user, because it's something worth checking).

But this is inconsistent behavior: if you pass a pointer to ro memory to devm_krealloc() it will not resize it but by returning a valid pointer it will make you think it did -> you end up writing to ro memory in good faith.

Quoted 30 lines

...
drivers/base/devres.c void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) · line 884
Quoted 5 lines
drivers/base/devres.c +4 -0
+       spin_lock_irqsave(&dev->devres_lock, flags);
+       old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
+       spin_unlock_irqrestore(&dev->devres_lock, flags);
 
+       if (!old_dr) {
I would have this under spin lock b/c of below.
drivers/base/devres.c void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) · line 889
Quoted 5 lines
drivers/base/devres.c +4 -0
+               WARN(1, "Memory chunk not managed or managed by a different device.");
+               return NULL;
+       }
 
+       old_head = old_dr->node.entry;
This would be still better to be under spin lock.
drivers/base/devres.c void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) · line 893
Quoted 3 lines
drivers/base/devres.c +3 -0
+       new_dr = krealloc(old_dr, total_size, gfp);
+       if (!new_dr)
+               return NULL;
And perhaps spin lock taken already here.
drivers/base/devres.c void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) · line 899
Quoted 5 lines
drivers/base/devres.c +5 -0
+       if (new_dr != old_dr) {
+               spin_lock_irqsave(&dev->devres_lock, flags);
+               list_replace(&old_head, &new_dr->node.entry);
+               spin_unlock_irqrestore(&dev->devres_lock, flags);
+       }

Quoted 5 lines

Yes, I understand that covering more code under spin lock does not fix any potential race, but at least it minimizes scope of the code that is not under it to see exactly what is problematic.
I probably will think more about a better approach to avoid potential races.

My thinking behind this was this: we already have users who call devres_find() and do something with the retrieved resources without holding the devres_lock - so it's assumed that users are sane enough to not be getting in each other's way. Now I see that the difference is that here we're accessing the list node and it can change if another thread is adding a different devres to the same device. So this should definitely be protected somehow.

I think that we may have to give up using real krealloc() and instead just reimplement its behavior in the following way:

Still outside of spinlock check if the new total size is smaller or equal to the previous one. If so: return the same pointer. If not:
allocate a new devres as if it were for devm_kmalloc() but don't add it to the list yet. Take the spinlock - check if we can find the devres - if not: kfree() the new and old chunk and return NULL. If
yes: copy the contents of the devres node into the new chunk as well as the memory contents. Replace the old one on the list and free it. Release spinlock and return.

Does that work?

Bart

Bartosz Golaszewski

author

2020-08-02 08:35 UTC #4 lore raw

PATCH v6 2/3 hwmon: pmbus: use more devres helpers

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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

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

Acked-by Guenter Roeck <[email protected]>

---
 drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

drivers/hwmon/pmbus/pmbus_core.c +9 ~~-19~~

index 44535add3a4a..91839979cf6c 100644
@@ -1018,9 +1018,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;

@@ -2534,7 +2534,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.

@@ -2542,35 +2542,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);

@@ -2580,8 +2572,6 @@ int pmbus_do_remove(struct i2c_client *client)
 
 	debugfs_remove_recursive(data->debugfs);
 
-	hwmon_device_unregister(data->hwmon_dev);
-	kfree(data->group.attrs);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_remove);

Signature · 1 line

2.26.1

Bartosz Golaszewski

author

2020-08-02 08:35 UTC #5 lore raw

PATCH v6 3/3 iio: adc: xilinx-xadc: use devm_krealloc()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Use the managed variant of krealloc() and shrink the code a bit.

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

Acked-by Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/xilinx-xadc-core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/iio/adc/xilinx-xadc-core.c +8 -8

index d0b7ef296afb..f93c34fe5873 100644
@@ -1092,6 +1092,7 @@ MODULE_DEVICE_TABLE(of, xadc_of_match_table);
 static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	unsigned int *conf)
 {
+	struct device *dev = indio_dev->dev.parent;
 	struct xadc *xadc = iio_priv(indio_dev);
 	struct iio_chan_spec *channels, *chan;
 	struct device_node *chan_node, *child;

@@ -1136,7 +1137,8 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 		*conf |= XADC_CONF0_MUX | XADC_CONF0_CHAN(ext_mux_chan);
 	}
 
-	channels = kmemdup(xadc_channels, sizeof(xadc_channels), GFP_KERNEL);
+	channels = devm_kmemdup(dev, xadc_channels,
+				sizeof(xadc_channels), GFP_KERNEL);
 	if (!channels)
 		return -ENOMEM;

@@ -1172,8 +1174,9 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	of_node_put(chan_node);
 
 	indio_dev->num_channels = num_channels;
-	indio_dev->channels = krealloc(channels, sizeof(*channels) *
-					num_channels, GFP_KERNEL);
+	indio_dev->channels = devm_krealloc(dev, channels,
+					    sizeof(*channels) * num_channels,
+					    GFP_KERNEL);
 	/* If we can't resize the channels array, just use the original */
 	if (!indio_dev->channels)
 		indio_dev->channels = channels;

@@ -1225,14 +1228,14 @@ static int xadc_probe(struct platform_device *pdev)
 
 	ret = xadc_parse_dt(indio_dev, pdev->dev.of_node, &conf0);
 	if (ret)
-		goto err_device_free;
+		return ret;
 
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
 		ret = iio_triggered_buffer_setup(indio_dev,
 			&iio_pollfunc_store_time, &xadc_trigger_handler,
 			&xadc_buffer_ops);
 		if (ret)
-			goto err_device_free;
+			return ret;
 
 		xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
 		if (IS_ERR(xadc->convst_trigger)) {

@@ -1350,8 +1353,6 @@ static int xadc_probe(struct platform_device *pdev)
 err_triggered_buffer_cleanup:
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
 		iio_triggered_buffer_cleanup(indio_dev);
-err_device_free:
-	kfree(indio_dev->channels);
 
 	return ret;
 }

@@ -1371,7 +1372,6 @@ static int xadc_remove(struct platform_device *pdev)
 	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
 	clk_disable_unprepare(xadc->clk);
 	kfree(xadc->data);
-	kfree(indio_dev->channels);
 
 	return 0;
 }

Signature · 1 line

2.26.1