Skip to contentSkip to conversation

nvmem/gpio: fix resource management

PATCH Superseded Bartosz Golaszewski 4 participants 15 messages Feb 17, 2020 – Feb 18, 2020

Subsystems (2)
GPIO NVMEM framework

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

cover nvmem/gpio: fix resource management

From: Bartosz Golaszewski <[email protected]>

This series addresses a couple problems with memory management in nvmem core. We fix an earlier memory leak in error path in patch 2/6. Patches 1/6, 5/6 & 6/6 add reference counting to gpio_desc structure and use it to correctly free the write-protect GPIO. Patches 3/6 & 4/6 fix newline problems.

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

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

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

PATCH 1/6 gpiolib: use kref in gpio_desc

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        | 26 +++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 25 insertions(+), 3 deletions(-)
drivers/gpio/gpiolib.c +23 -3
index 4d0106ceeba7..1f781cb97437 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,29 @@ 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
+ */
+void gpiod_ref(struct gpio_desc *desc)
+{
+	if (desc)
+		kref_get(&desc->ref);
+}
+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..b12bbd511c6e 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);
+void 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
author
#2 lore raw

PATCH 2/6 nvmem: fix memory leak in error path

In reply to Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We need to remove the ida mapping when returning from nvmem_register() with an error.

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvmem/core.c +3 -1
index ef326f243f36..b0be03d5f240 100644
@@ -353,7 +353,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(nvmem->wp_gpio))
-		return ERR_CAST(nvmem->wp_gpio);
+		goto err_ida_remove;
 
 
 	kref_init(&nvmem->refcnt);

@@ -430,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	device_del(&nvmem->dev);
 err_put_device:
 	put_device(&nvmem->dev);
+err_ida_remove:
+	ida_simple_remove(&nvmem_ida, nvmem->id);
 
 	return ERR_PTR(rval);
 }
Signature · 1 line
2.25.0
#4 lore raw

Re: [PATCH 2/6] nvmem: fix memory leak in error path

In reply to Bartosz Golaszewski
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

We need to remove the ida mapping when returning from nvmem_register() with an error.

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

Was too quick in my last reply..

Quoted 9 lines
---
  drivers/nvmem/core.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvmem/core.c +0 -0
index ef326f243f36..b0be03d5f240 100644

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

Quoted 5 lines
drivers/nvmem/core.c +1 -1
@@ -353,7 +353,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
  						    GPIOD_OUT_HIGH);
  	if (IS_ERR(nvmem->wp_gpio))
-		return ERR_CAST(nvmem->wp_gpio);
+		goto err_ida_remove;

Looks like this is adding nvmem leak here.
May be something like this should help:

if (IS_ERR(nvmem->wp_gpio)) {
	rval =  ERR_CAST(nvmem->wp_gpio);
	ida_simple_remove(&nvmem_ida, nvmem->id);
	kfree(nvmem);
	return rval;

}

--srini

Quoted 13 lines
drivers/nvmem/core.c +2 -0
  	kref_init(&nvmem->refcnt);

@@ -430,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
  	device_del(&nvmem->dev);
  err_put_device:
  	put_device(&nvmem->dev);
+err_ida_remove:
+	ida_simple_remove(&nvmem_ida, nvmem->id);
 
  	return ERR_PTR(rval);
  }
#5 lore raw

Re: [PATCH 2/6] nvmem: fix memory leak in error path

wt., 18 lut 2020 o 10:42 Srinivas Kandagatla
<[email protected]> napisał(a):

Quoted 31 lines · Bartosz Golaszewski
Quoted 6 lines

From: Bartosz Golaszewski <[email protected]>

We need to remove the ida mapping when returning from nvmem_register() with an error.

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

Was too quick in my last reply..

Quoted 9 lines
---
  drivers/nvmem/core.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvmem/core.c +0 -0
index ef326f243f36..b0be03d5f240 100644

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

Quoted 5 lines
drivers/nvmem/core.c +1 -1
@@ -353,7 +353,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
              nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
                                                  GPIOD_OUT_HIGH);
      if (IS_ERR(nvmem->wp_gpio))
-             return ERR_CAST(nvmem->wp_gpio);
+             goto err_ida_remove;

Looks like this is adding nvmem leak here.
May be something like this should help:

Quoted 8 lines
if (IS_ERR(nvmem->wp_gpio)) {
        rval =  ERR_CAST(nvmem->wp_gpio);
        ida_simple_remove(&nvmem_ida, nvmem->id);
        kfree(nvmem);
        return rval;

}

Srinivas,

I just sent a v2 of this series that addresses it as well. Please don't apply v1 yet.

Bartosz

author
#6 lore raw

PATCH 3/6 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 b0be03d5f240..12e2991cdeef 100644
@@ -355,7 +355,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (IS_ERR(nvmem->wp_gpio))
 		goto err_ida_remove;
 
-
 	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);
Signature · 1 line
2.25.0
author
#8 lore raw

PATCH 4/6 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 12e2991cdeef..07af9edea9cf 100644
@@ -347,6 +347,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		kfree(nvmem);
 		return ERR_PTR(rval);
 	}
+
 	if (config->wp_gpio)
 		nvmem->wp_gpio = config->wp_gpio;
 	else
Signature · 1 line
2.25.0
author
#10 lore raw

PATCH 5/6 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() and in the error path in nvmem_register().

Fixes 2a127da461a9 ("nvmem: add support for the write-protect pin")
Reported-by [email protected]
Signed-off-by Khouloud Touil <[email protected]>

[Bartosz:
- also put the gpio in error path in nvmem_register(),
- tweak the commit message: the GPIO is not auto-released]

Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/nvmem/core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/nvmem/core.c +2 -0
index 07af9edea9cf..096c7bae9e74 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);
 }
 

@@ -430,6 +431,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	device_del(&nvmem->dev);
 err_put_device:
 	put_device(&nvmem->dev);
+	gpiod_put(nvmem->wp_gpio);
 err_ida_remove:
 	ida_simple_remove(&nvmem_ida, nvmem->id);
Signature · 1 line
2.25.0
author
#11 lore raw

PATCH 6/6 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/nvmem/core.c +4 -2
index 096c7bae9e74..b7b1e3194453 100644
@@ -349,11 +349,13 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		return ERR_PTR(rval);
 	}
 
-	if (config->wp_gpio)
+	if (config->wp_gpio) {
 		nvmem->wp_gpio = config->wp_gpio;
-	else
+		gpiod_ref(config->wp_gpio);
+	} else {
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
+	}
 	if (IS_ERR(nvmem->wp_gpio))
 		goto err_ida_remove;
Signature · 1 line
2.25.0
#12 lore raw

Re: [PATCH 6/6] nvmem: increase the reference count of a gpio passed over config

Hi Bartosz,

Quoted 8 lines · 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]>

Thanks for your patch!

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

Quoted 11 lines
drivers/nvmem/core.c +2 -2
@@ -349,11 +349,13 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
                return ERR_PTR(rval);
        }
 
-       if (config->wp_gpio)
+       if (config->wp_gpio) {
                nvmem->wp_gpio = config->wp_gpio;
-       else
+               gpiod_ref(config->wp_gpio);

If gpiod_ref() would return the passed GPIO, or an error code, you could write

        nvmem->wp_gpio = gpiod_ref(config->wp_gpio);

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

Quoted 7 lines
drivers/nvmem/core.c +2 -0
+       } else {
                nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
                                                    GPIOD_OUT_HIGH);
+       }
        if (IS_ERR(nvmem->wp_gpio))
                goto err_ida_remove;

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
author
#13 lore raw

Re: [PATCH 6/6] nvmem: increase the reference count of a gpio passed over config

pon., 17 lut 2020 o 22:13 Geert Uytterhoeven <[email protected]> napisał(a):

Quoted 29 lines

Hi Bartosz,

Quoted 8 lines · 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]>

Thanks for your patch!

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

Quoted 11 lines
drivers/nvmem/core.c +2 -2
@@ -349,11 +349,13 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
                return ERR_PTR(rval);
        }
 
-       if (config->wp_gpio)
+       if (config->wp_gpio) {
                nvmem->wp_gpio = config->wp_gpio;
-       else
+               gpiod_ref(config->wp_gpio);

If gpiod_ref() would return the passed GPIO, or an error code, you could write

Quoted 2 lines
        nvmem->wp_gpio = gpiod_ref(config->wp_gpio);

Yes, makes perfect sense - v2 coming up tomorrow.

Bart

author
#14 lore raw
In reply to Bartosz Golaszewski

pon., 17 lut 2020 o 20:54 Bartosz Golaszewski <[email protected]> napisał(a):

Quoted 19 lines

From: Bartosz Golaszewski <[email protected]>

This series addresses a couple problems with memory management in nvmem core. We fix an earlier memory leak in error path in patch 2/6. Patches 1/6, 5/6 & 6/6 add reference counting to gpio_desc structure and use it to correctly free the write-protect GPIO. Patches 3/6 & 4/6 fix newline problems.

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

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

Quoted 9 lines
 drivers/gpio/gpiolib.c        | 26 +++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          | 14 ++++++++++----
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 35 insertions(+), 7 deletions(-)

--
2.25.0

Cc'ing Geert - sorry I forgot you when submitting this.

Bartosz