Skip to contentSkip to conversation

nvmem/gpio: fix resource management

PATCH Superseded Bartosz Golaszewski 4 participants 13 messages Feb 19, 2020 – Feb 21, 2020

Subsystems (2)
GPIO NVMEM framework

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

v3 cover nvmem/gpio: fix resource management

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

Bartosz Golaszewski (6):
nvmem: fix memory leak in error path
gpiolib: use kref in gpio_desc
nvmem: increase the reference count of a gpio passed over config
nvmem: remove a stray newline in nvmem_register()
nvmem: add a newline for readability
nvmem: add a comment about resource management

Khouloud Touil (1):
nvmem: release the write-protect pin

 drivers/gpio/gpiolib.c        | 36 ++++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          | 17 +++++++++++++----
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 48 insertions(+), 7 deletions(-)
Signature · 1 line
2.25.0
author
#1 lore raw

PATCH v3 1/7 nvmem: fix memory leak in error path

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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/nvmem/core.c +5 -2
index ef326f243f36..89974e40d250 100644
@@ -352,8 +352,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))
-		return ERR_CAST(nvmem->wp_gpio);
+	if (IS_ERR(nvmem->wp_gpio)) {
+		ida_simple_remove(&nvmem_ida, nvmem->id);
+		kfree(nvmem);
+		return ERR_PTR(rval);
+	}
 
 
 	kref_init(&nvmem->refcnt);
Signature · 1 line
2.25.0
#2 lore raw

Re: [PATCH v3 1/7] nvmem: fix memory leak in error path

Quoted 16 lines · Bartosz Golaszewski

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 | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
drivers/nvmem/core.c +0 -0
index ef326f243f36..89974e40d250 100644

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 358

Quoted 9 lines
drivers/nvmem/core.c +4 -2
@@ -352,8 +352,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))
-		return ERR_CAST(nvmem->wp_gpio);
+	if (IS_ERR(nvmem->wp_gpio)) {
+		ida_simple_remove(&nvmem_ida, nvmem->id);
+		kfree(nvmem);
+		return ERR_PTR(rval);

Why are you returning rval here?
This points return value of ida_simple_get and not the actual error code from wp_gpio.

--srini

Quoted 1 line
drivers/nvmem/core.c +1 -0
+	}
#3 lore raw

Re: [PATCH v3 1/7] nvmem: fix memory leak in error path

śr., 19 lut 2020 o 11:52 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 25 lines · Bartosz Golaszewski
Quoted 21 lines

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 | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
drivers/nvmem/core.c +0 -2
index ef326f243f36..89974e40d250 100644
@@ -352,8 +352,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))
-             return ERR_CAST(nvmem->wp_gpio);
Quoted 9 lines

drivers/nvmem/core.c struct nvmem_device *nvmem_register(const struct nvmem_config *config) · line 358

Quoted 4 lines
drivers/nvmem/core.c +4 -0
+     if (IS_ERR(nvmem->wp_gpio)) {
+             ida_simple_remove(&nvmem_ida, nvmem->id);
+             kfree(nvmem);
+             return ERR_PTR(rval);

Why are you returning rval here?
This points return value of ida_simple_get and not the actual error code from wp_gpio.

Duh! Thanks for catching this.

Bart

author
#4 lore raw

PATCH v3 2/7 gpiolib: use kref in gpio_desc

In reply to Bartosz Golaszewski

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(-)
drivers/gpio/gpiolib.c +33 -3
index 4d0106ceeba7..78220e86b2bd 100644
@@ -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

@@ -5067,18 +5076,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
drivers/gpio/gpiolib.h +1 -0
index 3e0aab2945d8..51a92c43dd55 100644
@@ -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);
include/linux/gpio/consumer.h +1 -0
index bf2d017dd7b7..c7b5fb3d9d64 100644
@@ -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);
Signature · 1 line
2.25.0
#5 lore raw

Re: [PATCH v3 2/7] gpiolib: use kref in gpio_desc

Quoted 6 lines · Bartosz Golaszewski

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.

Quoted 7 lines

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]>

I really like the simplicity of this approach ... hm! It seems the only downside is a krefcount_t extra per gpio_desc which is an atomic_t so I guess 32 or 64 bits per desc depending on arch.

We discussed refcounts in gpiods in another context but it got really complicated there. This is a clearly cut usecase.

Yours,
Linus Walleij

author
#6 lore raw

PATCH v3 3/7 nvmem: increase the reference count of a gpio passed over config

In reply to Bartosz Golaszewski

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(-)
drivers/nvmem/core.c +1 -1
index 89974e40d250..4e6daaa2b0f6 100644
@@ -348,7 +348,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		return ERR_PTR(rval);
 	}
 	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);
Signature · 1 line
2.25.0
author
#7 lore raw

PATCH v3 4/7 nvmem: release the write-protect pin

In reply to Bartosz Golaszewski

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(+)
drivers/nvmem/core.c +1 -0
index 4e6daaa2b0f6..c718ed7859e8 100644
@@ -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);
 }
Signature · 1 line
2.25.0
author
#8 lore raw

PATCH v3 5/7 nvmem: remove a stray newline in nvmem_register()

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Two newlines are unnecessary - remove one.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 1 -
 1 file changed, 1 deletion(-)
drivers/nvmem/core.c +0 -1
index c718ed7859e8..bbdf2fe076b9 100644
@@ -359,7 +359,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		return ERR_PTR(rval);
 	}
 
-
 	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);
Signature · 1 line
2.25.0
author
#9 lore raw

PATCH v3 6/7 nvmem: add a newline for readability

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Visibly separate the GPIO request from the previous operation in the code with a newline.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)
drivers/nvmem/core.c +1 -0
index bbdf2fe076b9..40fe5913c264 100644
@@ -348,6 +348,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		kfree(nvmem);
 		return ERR_PTR(rval);
 	}
+
 	if (config->wp_gpio)
 		nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
 	else
Signature · 1 line
2.25.0
author
#10 lore raw

PATCH v3 7/7 nvmem: add a comment about resource management

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The fact that part of the resources associated with the nvmem resources is freed by the release() callback in device_type caused me some confusion. Add a comment explaining that to nvmem_register().

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/nvmem/core.c +5 -0
index 40fe5913c264..6e28f3fddf53 100644
@@ -397,6 +397,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
 
+	/*
+	 * After calling device_add() we can stop freeing previously
+	 * allocated resources - they'll be freed from nvmem_release()
+	 * when the device's reference count drops to 0.
+	 */
 	rval = device_add(&nvmem->dev);
 	if (rval)
 		goto err_put_device;
Signature · 1 line
2.25.0
#11 lore raw

Re: [PATCH v3 7/7] nvmem: add a comment about resource management

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

The fact that part of the resources associated with the nvmem resources is freed by the release() callback in device_type caused me some confusion. Add a comment explaining that to nvmem_register().

I see this patch totally unnecessary!

I expect people creating patch in this area to understand what exactly they are doing. This is not going to help them in anyway, other than misleading and re-documenting device_add which is already available at

https://www.kernel.org/doc/html/v5.5/driver-api/infrastructure.html

thanks,
srini

Quoted 23 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
  drivers/nvmem/core.c | 5 +++++
  1 file changed, 5 insertions(+)
drivers/nvmem/core.c +5 -0
index 40fe5913c264..6e28f3fddf53 100644
@@ -397,6 +397,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
  	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
 
+	/*
+	 * After calling device_add() we can stop freeing previously
+	 * allocated resources - they'll be freed from nvmem_release()
+	 * when the device's reference count drops to 0.
+	 */
  	rval = device_add(&nvmem->dev);
  	if (rval)
  		goto err_put_device;