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(-)
--
2.25.0
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(-)
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
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(+)
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
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(-)
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
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(-)
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
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(-)
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
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(+)
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
pon., 17 lut 2020 o 20:54 Bartosz Golaszewski <[email protected]> napisał(a):
>
> 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(-)
>
> --
> 2.25.0
>
Cc'ing Geert - sorry I forgot you when submitting this.
Bartosz
Hi Bartosz,
On Mon, Feb 17, 2020 at 8:56 PM Bartosz Golaszewski <[email protected]> wrote:
> 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!
> --- 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 -- [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
pon., 17 lut 2020 o 22:13 Geert Uytterhoeven <[email protected]> napisał(a):
>
> Hi Bartosz,
>
> On Mon, Feb 17, 2020 at 8:56 PM Bartosz Golaszewski <[email protected]> wrote:
> > 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!
>
> > --- 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
On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> 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]>
> ---
Applied thanks,
--srini
On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Two newlines are unnecessary - remove one.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
Applied thanks,
--srini
On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> 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]>
> ---
Applied thanks,
--srini
On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> 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..
> ---
> 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);
> }
>
wt., 18 lut 2020 o 10:42 Srinivas Kandagatla
<[email protected]> napisał(a):
>
>
>
> On 17/02/2020 19:54, Bartosz Golaszewski wrote:
> > 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..
>
> > ---
> > 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