* [PATCH v2 1/7] nvmem: fix memory leak in error path
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
` (5 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
We need to remove the ida mapping when returning from nvmem_register()
with an error.
Cc: <stable@vger.kernel.org>
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 1/7] nvmem: fix memory leak in error path Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
2020-02-18 9:47 ` Srinivas Kandagatla
2020-02-18 9:56 ` Srinivas Kandagatla
2020-02-18 9:42 ` [PATCH v2 3/7] gpiolib: use kref in gpio_desc Bartosz Golaszewski
` (4 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
The nvmem struct is only freed on the first error check after its
allocation and leaked after that. Fix it with a new label.
Cc: <stable@vger.kernel.org>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/nvmem/core.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b0be03d5f240..c9b3f4047154 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
return ERR_PTR(-ENOMEM);
rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
- if (rval < 0) {
- kfree(nvmem);
- return ERR_PTR(rval);
- }
+ if (rval < 0)
+ goto err_free_nvmem;
if (config->wp_gpio)
nvmem->wp_gpio = config->wp_gpio;
else
@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
put_device(&nvmem->dev);
err_ida_remove:
ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+ kfree(nvmem);
return ERR_PTR(rval);
}
--
2.25.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
@ 2020-02-18 9:47 ` Srinivas Kandagatla
2020-02-18 9:50 ` Bartosz Golaszewski
2020-02-18 9:56 ` Srinivas Kandagatla
1 sibling, 1 reply; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18 9:47 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable
On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> The nvmem struct is only freed on the first error check after its
> allocation and leaked after that. Fix it with a new label.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
looks like 1/7 introduced the bug and 2/7 fixes it.
IMO, you should 1/7 and 2/7 should be single patch.
--srini
> ---
> drivers/nvmem/core.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b0be03d5f240..c9b3f4047154 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> return ERR_PTR(-ENOMEM);
>
> rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> - if (rval < 0) {
> - kfree(nvmem);
> - return ERR_PTR(rval);
> - }
> + if (rval < 0)
> + goto err_free_nvmem;
> if (config->wp_gpio)
> nvmem->wp_gpio = config->wp_gpio;
> else
> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> put_device(&nvmem->dev);
> err_ida_remove:
> ida_simple_remove(&nvmem_ida, nvmem->id);
> +err_free_nvmem:
> + kfree(nvmem);
>
> return ERR_PTR(rval);
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 9:47 ` Srinivas Kandagatla
@ 2020-02-18 9:50 ` Bartosz Golaszewski
0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:50 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Bartosz Golaszewski, Linus Walleij, Khouloud Touil,
Geert Uytterhoeven, linux-gpio, LKML, Stable # 4 . 20+
wt., 18 lut 2020 o 10:47 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The nvmem struct is only freed on the first error check after its
> > allocation and leaked after that. Fix it with a new label.
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> looks like 1/7 introduced the bug and 2/7 fixes it.
> IMO, you should 1/7 and 2/7 should be single patch.
>
The bug already exists in mainline - the nvmem object is only freed if
ida_simple_get() fails, but any subsequent error leads to a memory
leak. In other words: it doesn't matter if it's a single patch or two.
Bart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
2020-02-18 9:47 ` Srinivas Kandagatla
@ 2020-02-18 9:56 ` Srinivas Kandagatla
2020-02-18 10:05 ` Bartosz Golaszewski
1 sibling, 1 reply; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18 9:56 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable
On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> The nvmem struct is only freed on the first error check after its
> allocation and leaked after that. Fix it with a new label.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/nvmem/core.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b0be03d5f240..c9b3f4047154 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> return ERR_PTR(-ENOMEM);
>
> rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> - if (rval < 0) {
> - kfree(nvmem);
> - return ERR_PTR(rval);
> - }
> + if (rval < 0)
> + goto err_free_nvmem;
> if (config->wp_gpio)
> nvmem->wp_gpio = config->wp_gpio;
> else
> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> put_device(&nvmem->dev);
> err_ida_remove:
> ida_simple_remove(&nvmem_ida, nvmem->id);
> +err_free_nvmem:
> + kfree(nvmem);
This is not correct fix to start with, if the device has already been
intialized before jumping here then nvmem would be freed as part of
nvmem_release().
So the bug was actually introduced by adding err_ida_remove label.
You can free nvmem at that point but not at any point after that as
device core would be holding a reference to it.
--srini
>
> return ERR_PTR(rval);
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 9:56 ` Srinivas Kandagatla
@ 2020-02-18 10:05 ` Bartosz Golaszewski
2020-02-18 10:11 ` Srinivas Kandagatla
0 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 10:05 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Linus Walleij, Khouloud Touil, Geert Uytterhoeven,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Bartosz Golaszewski, stable
wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The nvmem struct is only freed on the first error check after its
> > allocation and leaked after that. Fix it with a new label.
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> > drivers/nvmem/core.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index b0be03d5f240..c9b3f4047154 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > return ERR_PTR(-ENOMEM);
> >
> > rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> > - if (rval < 0) {
> > - kfree(nvmem);
> > - return ERR_PTR(rval);
> > - }
> > + if (rval < 0)
> > + goto err_free_nvmem;
> > if (config->wp_gpio)
> > nvmem->wp_gpio = config->wp_gpio;
> > else
> > @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > put_device(&nvmem->dev);
> > err_ida_remove:
> > ida_simple_remove(&nvmem_ida, nvmem->id);
> > +err_free_nvmem:
> > + kfree(nvmem);
>
> This is not correct fix to start with, if the device has already been
> intialized before jumping here then nvmem would be freed as part of
> nvmem_release().
>
> So the bug was actually introduced by adding err_ida_remove label.
>
> You can free nvmem at that point but not at any point after that as
> device core would be holding a reference to it.
>
OK I see - I missed the release() callback assignment. Frankly: I find
this split of resource management responsibility confusing. Since the
users are expected to call nvmem_unregister() anyway - wouldn't it be
more clear to just free all resources there? What is the advantage of
defining the release() callback for device type here?
Bartosz
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 10:05 ` Bartosz Golaszewski
@ 2020-02-18 10:11 ` Srinivas Kandagatla
2020-02-18 10:22 ` Bartosz Golaszewski
0 siblings, 1 reply; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18 10:11 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Khouloud Touil, Geert Uytterhoeven,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
Bartosz Golaszewski, stable
On 18/02/2020 10:05, Bartosz Golaszewski wrote:
> wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> napisał(a):
>>
>>
>>
>> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> The nvmem struct is only freed on the first error check after its
>>> allocation and leaked after that. Fix it with a new label.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> ---
>>> drivers/nvmem/core.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index b0be03d5f240..c9b3f4047154 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>> return ERR_PTR(-ENOMEM);
>>>
>>> rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
>>> - if (rval < 0) {
>>> - kfree(nvmem);
>>> - return ERR_PTR(rval);
>>> - }
>>> + if (rval < 0)
>>> + goto err_free_nvmem;
>>> if (config->wp_gpio)
>>> nvmem->wp_gpio = config->wp_gpio;
>>> else
>>> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>> put_device(&nvmem->dev);
>>> err_ida_remove:
>>> ida_simple_remove(&nvmem_ida, nvmem->id);
>>> +err_free_nvmem:
>>> + kfree(nvmem);
>>
>> This is not correct fix to start with, if the device has already been
>> intialized before jumping here then nvmem would be freed as part of
>> nvmem_release().
>>
>> So the bug was actually introduced by adding err_ida_remove label.
>>
>> You can free nvmem at that point but not at any point after that as
>> device core would be holding a reference to it.
>>
>
> OK I see - I missed the release() callback assignment. Frankly: I find
> this split of resource management responsibility confusing. Since the
> users are expected to call nvmem_unregister() anyway - wouldn't it be
> more clear to just free all resources there? What is the advantage of
> defining the release() callback for device type here?
Because we are using dev pointer from nvmem structure, and this dev
pointer should be valid until release callback is invoked.
Freeing nvmem at any early stage would make dev pointer invalid and
device core would dereference it!
--srini
>
> Bartosz
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
2020-02-18 10:11 ` Srinivas Kandagatla
@ 2020-02-18 10:22 ` Bartosz Golaszewski
0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 10:22 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Bartosz Golaszewski, Linus Walleij, Khouloud Touil,
Geert Uytterhoeven, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, stable
wt., 18 lut 2020 o 11:11 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 18/02/2020 10:05, Bartosz Golaszewski wrote:
> > wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> napisał(a):
> >>
> >>
> >>
> >> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>
> >>> The nvmem struct is only freed on the first error check after its
> >>> allocation and leaked after that. Fix it with a new label.
> >>>
> >>> Cc: <stable@vger.kernel.org>
> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>> ---
> >>> drivers/nvmem/core.c | 8 ++++----
> >>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >>> index b0be03d5f240..c9b3f4047154 100644
> >>> --- a/drivers/nvmem/core.c
> >>> +++ b/drivers/nvmem/core.c
> >>> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >>> return ERR_PTR(-ENOMEM);
> >>>
> >>> rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> >>> - if (rval < 0) {
> >>> - kfree(nvmem);
> >>> - return ERR_PTR(rval);
> >>> - }
> >>> + if (rval < 0)
> >>> + goto err_free_nvmem;
> >>> if (config->wp_gpio)
> >>> nvmem->wp_gpio = config->wp_gpio;
> >>> else
> >>> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >>> put_device(&nvmem->dev);
> >>> err_ida_remove:
> >>> ida_simple_remove(&nvmem_ida, nvmem->id);
> >>> +err_free_nvmem:
> >>> + kfree(nvmem);
> >>
> >> This is not correct fix to start with, if the device has already been
> >> intialized before jumping here then nvmem would be freed as part of
> >> nvmem_release().
> >>
> >> So the bug was actually introduced by adding err_ida_remove label.
> >>
> >> You can free nvmem at that point but not at any point after that as
> >> device core would be holding a reference to it.
> >>
> >
> > OK I see - I missed the release() callback assignment. Frankly: I find
> > this split of resource management responsibility confusing. Since the
> > users are expected to call nvmem_unregister() anyway - wouldn't it be
> > more clear to just free all resources there? What is the advantage of
> > defining the release() callback for device type here?
>
> Because we are using dev pointer from nvmem structure, and this dev
> pointer should be valid until release callback is invoked.
>
> Freeing nvmem at any early stage would make dev pointer invalid and
> device core would dereference it!
>
Ok, let me brew up a v3 with that in mind.
Bart
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/7] gpiolib: use kref in gpio_desc
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 1/7] nvmem: fix memory leak in error path Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, 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 | 30 +++++++++++++++++++++++++++---
drivers/gpio/gpiolib.h | 1 +
include/linux/gpio/consumer.h | 1 +
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4d0106ceeba7..582425e9b449 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,33 @@ 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)
+ 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
` (2 preceding siblings ...)
2020-02-18 9:42 ` [PATCH v2 3/7] gpiolib: use kref in gpio_desc Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
2020-02-20 3:03 ` kbuild test robot
2020-02-18 9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, 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 c9b3f4047154..c4a6c044b020 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -346,7 +346,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
if (rval < 0)
goto err_free_nvmem;
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config
2020-02-18 9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
@ 2020-02-20 3:03 ` kbuild test robot
0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2020-02-20 3:03 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: kbuild-all, Linus Walleij, Srinivas Kandagatla, Khouloud Touil,
Geert Uytterhoeven, linux-gpio, linux-kernel,
Bartosz Golaszewski
[-- Attachment #1: Type: text/plain, Size: 5590 bytes --]
Hi Bartosz,
I love your patch! Perhaps something to improve:
[auto build test WARNING on gpio/for-next]
[also build test WARNING on linus/master v5.6-rc2]
[cannot apply to next-20200219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/nvmem-gpio-fix-resource-management/20200220-045651
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: nios2-3c120_defconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/nvmem/core.c: In function 'nvmem_register':
drivers/nvmem/core.c:349:20: error: implicit declaration of function 'gpiod_ref'; did you mean 'gpiod_get'? [-Werror=implicit-function-declaration]
nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
^~~~~~~~~
gpiod_get
>> drivers/nvmem/core.c:349:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
^
cc1: some warnings being treated as errors
vim +349 drivers/nvmem/core.c
322
323 /**
324 * nvmem_register() - Register a nvmem device for given nvmem_config.
325 * Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
326 *
327 * @config: nvmem device configuration with which nvmem device is created.
328 *
329 * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device
330 * on success.
331 */
332
333 struct nvmem_device *nvmem_register(const struct nvmem_config *config)
334 {
335 struct nvmem_device *nvmem;
336 int rval;
337
338 if (!config->dev)
339 return ERR_PTR(-EINVAL);
340
341 nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
342 if (!nvmem)
343 return ERR_PTR(-ENOMEM);
344
345 rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
346 if (rval < 0)
347 goto err_free_nvmem;
348 if (config->wp_gpio)
> 349 nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
350 else
351 nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
352 GPIOD_OUT_HIGH);
353 if (IS_ERR(nvmem->wp_gpio))
354 goto err_ida_remove;
355
356
357 kref_init(&nvmem->refcnt);
358 INIT_LIST_HEAD(&nvmem->cells);
359
360 nvmem->id = rval;
361 nvmem->owner = config->owner;
362 if (!nvmem->owner && config->dev->driver)
363 nvmem->owner = config->dev->driver->owner;
364 nvmem->stride = config->stride ?: 1;
365 nvmem->word_size = config->word_size ?: 1;
366 nvmem->size = config->size;
367 nvmem->dev.type = &nvmem_provider_type;
368 nvmem->dev.bus = &nvmem_bus_type;
369 nvmem->dev.parent = config->dev;
370 nvmem->priv = config->priv;
371 nvmem->type = config->type;
372 nvmem->reg_read = config->reg_read;
373 nvmem->reg_write = config->reg_write;
374 if (!config->no_of_node)
375 nvmem->dev.of_node = config->dev->of_node;
376
377 if (config->id == -1 && config->name) {
378 dev_set_name(&nvmem->dev, "%s", config->name);
379 } else {
380 dev_set_name(&nvmem->dev, "%s%d",
381 config->name ? : "nvmem",
382 config->name ? config->id : nvmem->id);
383 }
384
385 nvmem->read_only = device_property_present(config->dev, "read-only") ||
386 config->read_only || !nvmem->reg_write;
387
388 nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
389
390 device_initialize(&nvmem->dev);
391
392 dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
393
394 rval = device_add(&nvmem->dev);
395 if (rval)
396 goto err_put_device;
397
398 if (config->compat) {
399 rval = nvmem_sysfs_setup_compat(nvmem, config);
400 if (rval)
401 goto err_device_del;
402 }
403
404 if (config->cells) {
405 rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
406 if (rval)
407 goto err_teardown_compat;
408 }
409
410 rval = nvmem_add_cells_from_table(nvmem);
411 if (rval)
412 goto err_remove_cells;
413
414 rval = nvmem_add_cells_from_of(nvmem);
415 if (rval)
416 goto err_remove_cells;
417
418 blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
419
420 return nvmem;
421
422 err_remove_cells:
423 nvmem_device_remove_all_cells(nvmem);
424 err_teardown_compat:
425 if (config->compat)
426 nvmem_sysfs_remove_compat(nvmem, config);
427 err_device_del:
428 device_del(&nvmem->dev);
429 err_put_device:
430 put_device(&nvmem->dev);
431 err_ida_remove:
432 ida_simple_remove(&nvmem_ida, nvmem->id);
433 err_free_nvmem:
434 kfree(nvmem);
435
436 return ERR_PTR(rval);
437 }
438 EXPORT_SYMBOL_GPL(nvmem_register);
439
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9676 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 5/7] nvmem: release the write-protect pin
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
` (3 preceding siblings ...)
2020-02-18 9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
2020-02-18 12:24 ` Srinivas Kandagatla
2020-02-18 9:42 ` [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register() Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 7/7] nvmem: add a newline for readability Bartosz Golaszewski
6 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, 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 c4a6c044b020..e47152e9db34 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);
}
@@ -428,6 +429,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);
err_free_nvmem:
--
2.25.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/7] nvmem: release the write-protect pin
2020-02-18 9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
@ 2020-02-18 12:24 ` Srinivas Kandagatla
0 siblings, 0 replies; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18 12:24 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> @@ -428,6 +429,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);
> err_free_nvmem:
This is also redundant as explained in my previous comments about release()
--srini
> --
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register()
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
` (4 preceding siblings ...)
2020-02-18 9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
2020-02-18 9:42 ` [PATCH v2 7/7] nvmem: add a newline for readability Bartosz Golaszewski
6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, 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 e47152e9db34..5e7d46eccaf6 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -354,7 +354,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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 7/7] nvmem: add a newline for readability
2020-02-18 9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
` (5 preceding siblings ...)
2020-02-18 9:42 ` [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register() Bartosz Golaszewski
@ 2020-02-18 9:42 ` Bartosz Golaszewski
6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 9:42 UTC (permalink / raw)
To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
Cc: linux-gpio, linux-kernel, 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 5e7d46eccaf6..ec955db4673c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -346,6 +346,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
rval = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
if (rval < 0)
goto err_free_nvmem;
+
if (config->wp_gpio)
nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
else
--
2.25.0
^ permalink raw reply related [flat|nested] 16+ messages in thread