From: Bartosz Golaszewski <[email protected]>
This series addresses a couple problems with memory management in nvmem
core.
First we fix a memory leak introduced in this release cycle. Next we extend
the GPIO framework to use reference counting for GPIO descriptors. We then
use it to fix the resource management problem with the write-protect pin.
Finally we add some readability tweaks and a comment clearing up some
confusion about resource management.
While the memory leak with wp-gpios is now in mainline - I'm not sure how
to go about applying the kref patch. This is theoretically a new feature
but it's also the cleanest way of fixing the problem.
v1 -> v2:
- make gpiod_ref() helper return
- reorganize the series for easier merging
- fix another memory leak
v2 -> v3:
- drop incorrect patches
- add a patch adding a comment about resource management
- extend the GPIO kref patch: only increment the reference count if the
descriptor is associated with a requested line
v3 -> v4:
- fixed the return value in error path in nvmem_register()
- dropped patches already applied to the nvmem tree
- dropped the patch adding the comment about resource management
Bartosz Golaszewski (3):
nvmem: fix memory leak in error path
gpiolib: use kref in gpio_desc
nvmem: increase the reference count of a gpio passed over config
Khouloud Touil (1):
nvmem: release the write-protect pin
drivers/gpio/gpiolib.c | 36 ++++++++++++++++++++++++++++++++---
drivers/gpio/gpiolib.h | 1 +
drivers/nvmem/core.c | 8 ++++++--
include/linux/gpio/consumer.h | 1 +
4 files changed, 41 insertions(+), 5 deletions(-)
--
2.25.0
From: Bartosz Golaszewski <[email protected]>
We need to free the ida mapping and nvmem struct if the write-protect
GPIO lookup fails.
Fixes: 2a127da461a9 ("nvmem: add support for the write-protect pin")
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 503da67dde06..948c7ebce340 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -353,8 +353,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
else
nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
GPIOD_OUT_HIGH);
- if (IS_ERR(nvmem->wp_gpio))
+ if (IS_ERR(nvmem->wp_gpio)) {
+ ida_simple_remove(&nvmem_ida, nvmem->id);
+ kfree(nvmem);
return ERR_CAST(nvmem->wp_gpio);
+ }
kref_init(&nvmem->refcnt);
INIT_LIST_HEAD(&nvmem->cells);
--
2.25.0
From: Bartosz Golaszewski <[email protected]>
We can obtain the write-protect GPIO in nvmem_register() by requesting
it ourselves or by storing the gpio_desc passed in nvmem_config. In the
latter case we need to increase the reference count so that it gets
freed correctly.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 948c7ebce340..071fb897394c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -349,7 +349,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
}
if (config->wp_gpio)
- nvmem->wp_gpio = config->wp_gpio;
+ nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
else
nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
GPIOD_OUT_HIGH);
--
2.25.0
From: Khouloud Touil <[email protected]>
Put the write-protect GPIO descriptor in nvmem_release() so that it can
be automatically released when the associated device's reference count
drops to 0.
Fixes: 2a127da461a9 ("nvmem: add support for the write-protect pin")
Reported-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Khouloud Touil <[email protected]>
[Bartosz: tweak the commit message]
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 071fb897394c..a82375f63d64 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -72,6 +72,7 @@ static void nvmem_release(struct device *dev)
struct nvmem_device *nvmem = to_nvmem_device(dev);
ida_simple_remove(&nvmem_ida, nvmem->id);
+ gpiod_put(nvmem->wp_gpio);
kfree(nvmem);
}
--
2.25.0
From: Bartosz Golaszewski <[email protected]>
GPIO descriptors are freed by consumers using gpiod_put(). The name of
this function suggests some reference counting is going on but it's not
true.
Use kref to actually introduce reference counting for gpio_desc objects.
Add a corresponding gpiod_get() helper for increasing the reference count.
This doesn't change anything for already existing (correct) drivers but
allows us to keep track of GPIO descs used by multiple users.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpiolib.c | 36 ++++++++++++++++++++++++++++++++---
drivers/gpio/gpiolib.h | 1 +
include/linux/gpio/consumer.h | 1 +
3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 753283486037..42f820356279 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2798,6 +2798,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
goto done;
}
+ kref_init(&desc->ref);
+
if (chip->request) {
/* chip->request may sleep */
spin_unlock_irqrestore(&gpio_lock, flags);
@@ -2933,6 +2935,13 @@ void gpiod_free(struct gpio_desc *desc)
}
}
+static void gpiod_free_kref(struct kref *ref)
+{
+ struct gpio_desc *desc = container_of(ref, struct gpio_desc, ref);
+
+ gpiod_free(desc);
+}
+
/**
* gpiochip_is_requested - return string iff signal was requested
* @chip: controller managing the signal
@@ -5047,18 +5056,39 @@ struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
/**
- * gpiod_put - dispose of a GPIO descriptor
- * @desc: GPIO descriptor to dispose of
+ * gpiod_put - decrease the reference count of a GPIO descriptor
+ * @desc: GPIO descriptor to unref
*
* No descriptor can be used after gpiod_put() has been called on it.
*/
void gpiod_put(struct gpio_desc *desc)
{
if (desc)
- gpiod_free(desc);
+ kref_put(&desc->ref, gpiod_free_kref);
}
EXPORT_SYMBOL_GPL(gpiod_put);
+/**
+ * gpiod_ref - increase the reference count of a GPIO descriptor
+ * @desc: GPIO descriptor to reference
+ *
+ * Returns the same gpio_desc after increasing the reference count.
+ */
+struct gpio_desc *gpiod_ref(struct gpio_desc *desc)
+{
+ if (!desc)
+ return NULL;
+
+ if (!test_bit(FLAG_REQUESTED, &desc->flags)) {
+ pr_warn("gpiolib: unable to increase the reference count of unrequested GPIO descriptor\n");
+ return desc;
+ }
+
+ kref_get(&desc->ref);
+ return desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_ref);
+
/**
* gpiod_put_array - dispose of multiple GPIO descriptors
* @descs: struct gpio_descs containing an array of descriptors
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3e0aab2945d8..51a92c43dd55 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -119,6 +119,7 @@ struct gpio_desc {
const char *label;
/* Name of the GPIO */
const char *name;
+ struct kref ref;
};
int gpiod_request(struct gpio_desc *desc, const char *label);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index bf2d017dd7b7..c7b5fb3d9d64 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -81,6 +81,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
const char *con_id,
enum gpiod_flags flags);
+struct gpio_desc *gpiod_ref(struct gpio_desc *desc);
void gpiod_put(struct gpio_desc *desc);
void gpiod_put_array(struct gpio_descs *descs);
--
2.25.0
On 20/02/2020 10:01, Bartosz Golaszewski wrote:
> - if (IS_ERR(nvmem->wp_gpio))
> + if (IS_ERR(nvmem->wp_gpio)) {
> + ida_simple_remove(&nvmem_ida, nvmem->id);
> + kfree(nvmem);
> return ERR_CAST(nvmem->wp_gpio);
You freed nvmem just before this statement, how can nvmem->wp_gpio be
still be valid?
Are you able to test this changes at your end?
Or
these are just compile tested?
--srini
> + }
>
On 20/02/2020 10:01, Bartosz Golaszewski wrote:
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2798,6 +2798,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> goto done;
> }
>
> + kref_init(&desc->ref);
> +
Should we not decrement refcount on the error path of this function?
--srini
> if (chip->request) {
> /* chip->request may sleep */
> spin_unlock_irqrestore(&gpio_lock, flags);
> @@ -2933,6 +2935,13 @@ void gpiod_free(struct gpio_desc *desc)
> }
> }
czw., 20 lut 2020 o 12:30 Srinivas Kandagatla
<[email protected]> napisał(a):
>
>
>
> On 20/02/2020 10:01, Bartosz Golaszewski wrote:
> > - if (IS_ERR(nvmem->wp_gpio))
> > + if (IS_ERR(nvmem->wp_gpio)) {
> > + ida_simple_remove(&nvmem_ida, nvmem->id);
> > + kfree(nvmem);
> > return ERR_CAST(nvmem->wp_gpio);
> You freed nvmem just before this statement, how can nvmem->wp_gpio be
> still be valid?
>
> Are you able to test this changes at your end?
> Or
> these are just compile tested?
>
Sorry this was rushed, I will have access to the HW for testing tomorrow.
Bartosz
czw., 20 lut 2020 o 13:05 Srinivas Kandagatla
<[email protected]> napisał(a):
>
>
>
> On 20/02/2020 10:01, Bartosz Golaszewski wrote:
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2798,6 +2798,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > goto done;
> > }
> >
> > + kref_init(&desc->ref);
> > +
>
> Should we not decrement refcount on the error path of this function?
>
On error the descriptor will still be unrequested so there's no point
in potentially calling gpiod_free(). Also: the next time someone
requests it and succeeds, we'll set it back to 1.
Bartosz
Hi Bartosz,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20200221]
[also build test ERROR on v5.6-rc2]
[cannot apply to gpio/for-next linus/master v5.6-rc2 v5.6-rc1 v5.5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/nvmem-gpio-fix-resource-management/20200222-054341
base: bee46b309a13ca158c99c325d0408fb2f0db207f
config: sparc-defconfig (attached as .config)
compiler: sparc-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/nvmem/core.c: In function 'nvmem_register':
>> drivers/nvmem/core.c:352:20: error: implicit declaration of function 'gpiod_ref'; did you mean 'gpiod_get'? [-Werror=implicit-function-declaration]
nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
^~~~~~~~~
gpiod_get
drivers/nvmem/core.c:352:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
^
cc1: some warnings being treated as errors
vim +352 drivers/nvmem/core.c
322
323 /**
324 * nvmem_register() - Register a nvmem device for given nvmem_config.
325 * Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
326 *
327 * @config: nvmem device configuration with which nvmem device is created.
328 *
329 * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device
330 * on success.
331 */
332
333 struct nvmem_device *nvmem_register(const struct nvmem_config *config)
334 {
335 struct nvmem_device *nvmem;
336 int rval;
337
338 if (!config->dev)
339 return ERR_PTR(-EINVAL);
340
341 nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
342 if (!nvmem)
343 return ERR_PTR(-ENOMEM);
344
345 rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
346 if (rval < 0) {
347 kfree(nvmem);
348 return ERR_PTR(rval);
349 }
350
351 if (config->wp_gpio)
> 352 nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
353 else
354 nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
355 GPIOD_OUT_HIGH);
356 if (IS_ERR(nvmem->wp_gpio)) {
357 ida_simple_remove(&nvmem_ida, nvmem->id);
358 kfree(nvmem);
359 return ERR_CAST(nvmem->wp_gpio);
360 }
361
362 kref_init(&nvmem->refcnt);
363 INIT_LIST_HEAD(&nvmem->cells);
364
365 nvmem->id = rval;
366 nvmem->owner = config->owner;
367 if (!nvmem->owner && config->dev->driver)
368 nvmem->owner = config->dev->driver->owner;
369 nvmem->stride = config->stride ?: 1;
370 nvmem->word_size = config->word_size ?: 1;
371 nvmem->size = config->size;
372 nvmem->dev.type = &nvmem_provider_type;
373 nvmem->dev.bus = &nvmem_bus_type;
374 nvmem->dev.parent = config->dev;
375 nvmem->priv = config->priv;
376 nvmem->type = config->type;
377 nvmem->reg_read = config->reg_read;
378 nvmem->reg_write = config->reg_write;
379 if (!config->no_of_node)
380 nvmem->dev.of_node = config->dev->of_node;
381
382 if (config->id == -1 && config->name) {
383 dev_set_name(&nvmem->dev, "%s", config->name);
384 } else {
385 dev_set_name(&nvmem->dev, "%s%d",
386 config->name ? : "nvmem",
387 config->name ? config->id : nvmem->id);
388 }
389
390 nvmem->read_only = device_property_present(config->dev, "read-only") ||
391 config->read_only || !nvmem->reg_write;
392
393 nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
394
395 device_initialize(&nvmem->dev);
396
397 dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
398
399 rval = device_add(&nvmem->dev);
400 if (rval)
401 goto err_put_device;
402
403 if (config->compat) {
404 rval = nvmem_sysfs_setup_compat(nvmem, config);
405 if (rval)
406 goto err_device_del;
407 }
408
409 if (config->cells) {
410 rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
411 if (rval)
412 goto err_teardown_compat;
413 }
414
415 rval = nvmem_add_cells_from_table(nvmem);
416 if (rval)
417 goto err_remove_cells;
418
419 rval = nvmem_add_cells_from_of(nvmem);
420 if (rval)
421 goto err_remove_cells;
422
423 blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
424
425 return nvmem;
426
427 err_remove_cells:
428 nvmem_device_remove_all_cells(nvmem);
429 err_teardown_compat:
430 if (config->compat)
431 nvmem_sysfs_remove_compat(nvmem, config);
432 err_device_del:
433 device_del(&nvmem->dev);
434 err_put_device:
435 put_device(&nvmem->dev);
436
437 return ERR_PTR(rval);
438 }
439 EXPORT_SYMBOL_GPL(nvmem_register);
440
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
sob., 22 lut 2020 o 12:54 kbuild test robot <[email protected]> napisał(a):
>
> Hi Bartosz,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on next-20200221]
> [also build test ERROR on v5.6-rc2]
> [cannot apply to gpio/for-next linus/master v5.6-rc2 v5.6-rc1 v5.5]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url: https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/nvmem-gpio-fix-resource-management/20200222-054341
> base: bee46b309a13ca158c99c325d0408fb2f0db207f
> config: sparc-defconfig (attached as .config)
> compiler: sparc-linux-gcc (GCC) 7.5.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.5.0 make.cross ARCH=sparc
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
>
> drivers/nvmem/core.c: In function 'nvmem_register':
> >> drivers/nvmem/core.c:352:20: error: implicit declaration of function 'gpiod_ref'; did you mean 'gpiod_get'? [-Werror=implicit-function-declaration]
> nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
> ^~~~~~~~~
> gpiod_get
> drivers/nvmem/core.c:352:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
> nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
> ^
> cc1: some warnings being treated as errors
>
> vim +352 drivers/nvmem/core.c
>
Of course I forgot to add the stub...
Will fix in next iteration.
Bart