[PATCH v5 0/5] nvmem/gpio: fix resource management

[PATCH v5 0/5] nvmem/gpio: fix resource management

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hi Srinivas,

sorry for the lack of proper QA last time. This time the code is tested
on real HW (Beagle Bone Black with an ACME cape) including the error
path.

--

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.

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

v4 -> v5:
- don't reference nvmem once it's freed
- add GPIO descriptor validation in gpiod_ref()

Bartosz Golaszewski (4):
  nvmem: fix memory leak in error path
  gpiolib: provide VALIDATE_DESC_PTR() macro
  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        | 46 ++++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          | 11 ++++++---
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 52 insertions(+), 7 deletions(-)

-- 
2.25.0

[PATCH v5 1/5] nvmem: fix memory leak in error path

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 503da67dde06..2758d90d63b7 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -353,8 +353,12 @@ 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);
+		rval = PTR_ERR(nvmem->wp_gpio);
+		kfree(nvmem);
+		return ERR_PTR(rval);
+	}
 
 	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);
-- 
2.25.0

[PATCH v5 2/5] gpiolib: provide VALIDATE_DESC_PTR() macro

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We're about to add a public GPIO function that takes a descriptor as
argument and returns a pointer. Add a corresponding macro wrapping the
validate_desc() function that returns an ERR_PTR() on error.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4d0106ceeba7..da8ffd40aa97 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2864,6 +2864,14 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return; \
 	} while (0)
 
+#define VALIDATE_DESC_PTR(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid < 0) \
+		return ERR_PTR(__valid); \
+	if (__valid == 0) \
+		return NULL; \
+	} while (0)
+
 int gpiod_request(struct gpio_desc *desc, const char *label)
 {
 	int ret = -EPROBE_DEFER;
-- 
2.25.0

[PATCH v5 3/5] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c        | 38 +++++++++++++++++++++++++++++++----
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index da8ffd40aa97..a00e28ca8a49 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);
@@ -2941,6 +2943,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
@@ -5075,18 +5084,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);
+	VALIDATE_DESC_VOID(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)
+{
+	VALIDATE_DESC_PTR(desc);
+
+	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

[PATCH v5 4/5] nvmem: increase the reference count of a gpio passed over config

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 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 2758d90d63b7..4a4ef2049022 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

[PATCH v5 5/5] nvmem: release the write-protect pin

[Bartosz Golaszewski]

From: Khouloud Touil <ktouil@baylibre.com>

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 <geert@linux-m68k.org>
Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
[Bartosz: tweak the commit message]
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4a4ef2049022..790ec9b5552e 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

Re: [PATCH v5 1/5] nvmem: fix memory leak in error path

[Srinivas Kandagatla]

On 21/02/2020 15:48, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> 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 <bgolaszewski@baylibre.com>


Applied this and 5/5

Thanks,
srini

> ---
>   drivers/nvmem/core.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 503da67dde06..2758d90d63b7 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -353,8 +353,12 @@ 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);
> +		rval = PTR_ERR(nvmem->wp_gpio);
> +		kfree(nvmem);
> +		return ERR_PTR(rval);
> +	}
>   
>   	kref_init(&nvmem->refcnt);
>   	INIT_LIST_HEAD(&nvmem->cells);
> 

Re: [PATCH v5 2/5] gpiolib: provide VALIDATE_DESC_PTR() macro

[Linus Walleij]

On Fri, Feb 21, 2020 at 4:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We're about to add a public GPIO function that takes a descriptor as
> argument and returns a pointer. Add a corresponding macro wrapping the
> validate_desc() function that returns an ERR_PTR() on error.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
(I expect to get this back as a pull request or something later.)

Yours,
Linus Walleij

Re: [PATCH v5 3/5] gpiolib: use kref in gpio_desc

[Linus Walleij]

On Fri, Feb 21, 2020 at 4:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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 <bgolaszewski@baylibre.com>

I'm having some trouble figuring out if we might be reinventing
a wheel here.

A while back there was a proposed patch to add device links
between GPIO producers and consumers, so that a GPIO
chip won't be dropped while there are active consumers.

(I don't remember who sent the patch.)

We have a similar functionality in pin control if the
.link_consumers property is set on the pincontrol device.
I was thinking about making that compulsory at one point.

The device links use a kref already existing in struct
device and would in this case be the kref in the struct
device for the struct gpio_device.

So if that existed, gpiod_ref could just grab another
device_link_add().

Maybe we should just add device links between all
GPIO consumers (devices) and struct gpio_device:s
struct device and implement it like this so we don't
have to back out of this later?

C.f. commit
commit 036f394dd77f8117346874151793ec38967d843f
pinctrl: Enable device link creation for pin control

(...)
> @@ -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);

You forgot to add a stub for the case where GPIOLIB is not
compiled in I think? (Lower in the same file.)

Yours,
Linus Walleij

Re: [PATCH v5 3/5] gpiolib: use kref in gpio_desc

[Bartosz Golaszewski]

pt., 28 lut 2020 o 23:33 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Fri, Feb 21, 2020 at 4:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > 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 <bgolaszewski@baylibre.com>
>
> I'm having some trouble figuring out if we might be reinventing
> a wheel here.
>
> A while back there was a proposed patch to add device links
> between GPIO producers and consumers, so that a GPIO
> chip won't be dropped while there are active consumers.
>
> (I don't remember who sent the patch.)
>
> We have a similar functionality in pin control if the
> .link_consumers property is set on the pincontrol device.
> I was thinking about making that compulsory at one point.
>
> The device links use a kref already existing in struct
> device and would in this case be the kref in the struct
> device for the struct gpio_device.
>
> So if that existed, gpiod_ref could just grab another
> device_link_add().
>

I was always under the impression that device links are aimed mostly
at runtime PM.

> Maybe we should just add device links between all
> GPIO consumers (devices) and struct gpio_device:s
> struct device and implement it like this so we don't
> have to back out of this later?
>
> C.f. commit
> commit 036f394dd77f8117346874151793ec38967d843f
> pinctrl: Enable device link creation for pin control
>

Yes, definitely looks like it's done with PM in mind. Maybe we should
do what nvmem does? Define a struct device_type for GPIO chips with an
appropriate release() callback and use get_device() and put_device()?
Although nvmem seems to use kref for cells and device reference
counting somewhat separately - maybe that's something to address too.

> (...)
> > @@ -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);
>
> You forgot to add a stub for the case where GPIOLIB is not
> compiled in I think? (Lower in the same file.)
>
> Yours,
> Linus Walleij

Yeah this is fixed in the next version (with a different subject since
it no longer concerns nvmem that much).

Bart