[PATCH 0/6] nvmem/gpio: fix resource management

[PATCH 0/6] nvmem/gpio: fix resource management

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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(-)

-- 
2.25.0

[PATCH 1/6] 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        | 26 +++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4d0106ceeba7..1f781cb97437 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
@@ -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
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..b12bbd511c6e 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);
+void 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 2/6] nvmem: fix memory leak in error path

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ef326f243f36..b0be03d5f240 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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);
 }
-- 
2.25.0

[PATCH 3/6] nvmem: remove a stray newline in nvmem_register()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Two newlines are unnecessary - remove one.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b0be03d5f240..12e2991cdeef 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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);
 
-- 
2.25.0

[PATCH 4/6] nvmem: add a newline for readability

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

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 12e2991cdeef..07af9edea9cf 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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
-- 
2.25.0

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

[Bartosz Golaszewski]

From: Khouloud Touil <ktouil@baylibre.com>

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: geert@linux-m68k.org
Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
[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 <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 07af9edea9cf..096c7bae9e74 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);
 }
 
@@ -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);
 
-- 
2.25.0

[PATCH 6/6] 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 096c7bae9e74..b7b1e3194453 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -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;
 
-- 
2.25.0

Re: [PATCH 0/6] nvmem/gpio: fix resource management

[Bartosz Golaszewski]

pon., 17 lut 2020 o 20:54 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> 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(-)
>
> --
> 2.25.0
>

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

Bartosz

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

[Geert Uytterhoeven]

Hi Bartosz,

On Mon, Feb 17, 2020 at 8:56 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 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>

Thanks for your patch!

> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -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);

> +       } 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Bartosz Golaszewski]

pon., 17 lut 2020 o 22:13 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> Hi Bartosz,
>
> On Mon, Feb 17, 2020 at 8:56 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > 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>
>
> Thanks for your patch!
>
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -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);
>

Yes, makes perfect sense - v2 coming up tomorrow.

Bart

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

[Srinivas Kandagatla]

On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We need to remove the ida mapping when returning from nvmem_register()
> with an error.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---

Applied thanks,
--srini

Re: [PATCH 3/6] nvmem: remove a stray newline in nvmem_register()

[Srinivas Kandagatla]

On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Two newlines are unnecessary - remove one.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---


Applied thanks,
--srini

Re: [PATCH 4/6] nvmem: add a newline for readability

[Srinivas Kandagatla]

On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Visibly separate the GPIO request from the previous operation in the
> code with a newline.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---


Applied thanks,
--srini

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

[Srinivas Kandagatla]

On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We need to remove the ida mapping when returning from nvmem_register()
> with an error.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Was too quick in my last reply..

> ---
>   drivers/nvmem/core.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ef326f243f36..b0be03d5f240 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -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
>   
>   
>   	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);
>   }
> 

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

[Bartosz Golaszewski]

wt., 18 lut 2020 o 10:42 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We need to remove the ida mapping when returning from nvmem_register()
> > with an error.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Was too quick in my last reply..
>
> > ---
> >   drivers/nvmem/core.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index ef326f243f36..b0be03d5f240 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -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;
>
> }
>

Srinivas,

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

Bartosz