The regulator core takes over managing the lifetime of the enable GPIO
once the regulator is registered. As such we shouldn't register the
enable GPIO using devm, or it will be freed twice.
Reported-by: Marek Szyprowski <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
Again only build tested.
Thanks,
Charles
drivers/regulator/wm8994-regulator.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index d7fec533c4032..46e6b4ee1491a 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -147,11 +147,14 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
config.regmap = wm8994->regmap;
config.init_data = &ldo->init_data;
- /* Look up LDO enable GPIO from the parent device node */
- gpiod = devm_gpiod_get_optional(pdev->dev.parent,
- id ? "wlf,ldo2ena" : "wlf,ldo1ena",
- GPIOD_OUT_LOW |
- GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+ /*
+ * Look up LDO enable GPIO from the parent device node, we don't
+ * use devm because the regulator core will free the GPIO
+ */
+ gpiod = gpiod_get_optional(pdev->dev.parent,
+ id ? "wlf,ldo2ena" : "wlf,ldo1ena",
+ GPIOD_OUT_LOW |
+ GPIOD_FLAGS_BIT_NONEXCLUSIVE);
if (IS_ERR(gpiod))
return PTR_ERR(gpiod);
config.ena_gpiod = gpiod;
@@ -184,6 +187,7 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
return 0;
err:
+ gpiod_put(gpiod);
return ret;
}
--
2.11.0
On Wed, Nov 21, 2018 at 11:42:06AM +0100, Marek Szyprowski wrote:
> On 2018-11-21 11:13, Charles Keepax wrote:
> Linus, Mark: Similar issue is probably in the other regulator drivers,
> which use enable GPIO allocated by devm_gpio_get*(). This driver is
> simply the first one, which we observed it. It would be great if one
> would take a look into regulator_register() error path, because for some
> cases the GPIO will be freed, for the other - not.
>
Yeah I have managed to reproduce the issue on another on of our
boards now, so it definitely affects other drivers we have. Its a
bit sneaky since the GPIO core only seems to warn on the additional
free if you have some additional debug turned on which masks the
issue most of the time.
I will have a bit of a look and see what else might be affected.
Thanks,
Charles
Hi Charles,
On 2018-11-21 11:13, Charles Keepax wrote:
> The regulator core takes over managing the lifetime of the enable GPIO
> once the regulator is registered. As such we shouldn't register the
> enable GPIO using devm, or it will be freed twice.
>
> Reported-by: Marek Szyprowski <[email protected]>
> Signed-off-by: Charles Keepax <[email protected]>
> ---
>
> Again only build tested.
Thanks for the patch. It fixes the observed GPIOlib warning.
Tested-by: Marek Szyprowski <[email protected]>
It might sense to add:
Fixes: 1d2f46814d20 ("regulator: wm8994: Pass descriptor instead of GPIO
number")
Linus, Mark: Similar issue is probably in the other regulator drivers,
which use enable GPIO allocated by devm_gpio_get*(). This driver is
simply the first one, which we observed it. It would be great if one
would take a look into regulator_register() error path, because for some
cases the GPIO will be freed, for the other - not.
> Thanks,
> Charles
>
> drivers/regulator/wm8994-regulator.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
> index d7fec533c4032..46e6b4ee1491a 100644
> --- a/drivers/regulator/wm8994-regulator.c
> +++ b/drivers/regulator/wm8994-regulator.c
> @@ -147,11 +147,14 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
> config.regmap = wm8994->regmap;
> config.init_data = &ldo->init_data;
>
> - /* Look up LDO enable GPIO from the parent device node */
> - gpiod = devm_gpiod_get_optional(pdev->dev.parent,
> - id ? "wlf,ldo2ena" : "wlf,ldo1ena",
> - GPIOD_OUT_LOW |
> - GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> + /*
> + * Look up LDO enable GPIO from the parent device node, we don't
> + * use devm because the regulator core will free the GPIO
> + */
> + gpiod = gpiod_get_optional(pdev->dev.parent,
> + id ? "wlf,ldo2ena" : "wlf,ldo1ena",
> + GPIOD_OUT_LOW |
> + GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> if (IS_ERR(gpiod))
> return PTR_ERR(gpiod);
> config.ena_gpiod = gpiod;
> @@ -184,6 +187,7 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
> return 0;
>
> err:
> + gpiod_put(gpiod);
> return ret;
> }
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Wed, Nov 21, 2018 at 11:13 AM Charles Keepax
<[email protected]> wrote:
> The regulator core takes over managing the lifetime of the enable GPIO
> once the regulator is registered. As such we shouldn't register the
> enable GPIO using devm, or it will be freed twice.
>
> Reported-by: Marek Szyprowski <[email protected]>
> Signed-off-by: Charles Keepax <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Thanks for fixing everything I break, I owe you big time Charles!
Now as Marek points out, I need to discern the pattern in this so
I can see if I broke something else the same way.
Yours,
Linus Walleij
On Wed, Nov 21, 2018 at 11:42 AM Marek Szyprowski
<[email protected]> wrote:
> Linus, Mark: Similar issue is probably in the other regulator drivers,
> which use enable GPIO allocated by devm_gpio_get*(). This driver is
> simply the first one, which we observed it. It would be great if one
> would take a look into regulator_register() error path, because for some
> cases the GPIO will be freed, for the other - not.
OK I am onto it!
Yours,
Linus Walleij
On Thu, Nov 22, 2018 at 3:19 PM Linus Walleij <[email protected]> wrote:
> On Wed, Nov 21, 2018 at 11:13 AM Charles Keepax
> <[email protected]> wrote:
>
> > The regulator core takes over managing the lifetime of the enable GPIO
> > once the regulator is registered. As such we shouldn't register the
> > enable GPIO using devm, or it will be freed twice.
> >
> > Reported-by: Marek Szyprowski <[email protected]>
> > Signed-off-by: Charles Keepax <[email protected]>
>
> Reviewed-by: Linus Walleij <[email protected]>
Oh no this is not the right solution I think.
All drivers passing a descriptor (config->ena_gpiod) do their
own refcounting, including some using a function that has no
non-devm* counterpart.
It is better if we teach the core to not gpiod_put() those.
The other patch series I am floating to get rid of the legacy
GPIO handling from the core will do away with all the
legacy GPIO handling anyway, so let me introduce a bit
of ugliness (that can be backported) and then delete that
ugliness with an updated series v8 of my legacy GPIO
cleanup.
Sorry for the inconvenience.
Will send a patch soon.
Yours,
Linus Walleij
On Thu, Nov 22, 2018 at 04:47:20PM +0100, Linus Walleij wrote:
> On Thu, Nov 22, 2018 at 3:19 PM Linus Walleij <[email protected]> wrote:
> > On Wed, Nov 21, 2018 at 11:13 AM Charles Keepax
> > <[email protected]> wrote:
> >
> > > The regulator core takes over managing the lifetime of the enable GPIO
> > > once the regulator is registered. As such we shouldn't register the
> > > enable GPIO using devm, or it will be freed twice.
> > >
> > > Reported-by: Marek Szyprowski <[email protected]>
> > > Signed-off-by: Charles Keepax <[email protected]>
> >
> > Reviewed-by: Linus Walleij <[email protected]>
>
> Oh no this is not the right solution I think.
>
Yes I agree I actually am just about to send another series, I
guess I will send that and we can look at that and any
suggestions you have.
> All drivers passing a descriptor (config->ena_gpiod) do their
> own refcounting, including some using a function that has no
> non-devm* counterpart.
>
> It is better if we teach the core to not gpiod_put() those.
>
Yeah that is exactly what my patch chain is doing.
> The other patch series I am floating to get rid of the legacy
> GPIO handling from the core will do away with all the
> legacy GPIO handling anyway, so let me introduce a bit
> of ugliness (that can be backported) and then delete that
> ugliness with an updated series v8 of my legacy GPIO
> cleanup.
>
> Sorry for the inconvenience.
>
> Will send a patch soon.
>
> Yours,
> Linus Walleij
Thanks,
Charles