[PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Charles Keepax]

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 <m.szyprowski@samsung.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

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

Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>
> Again only build tested.

Thanks for the patch. It fixes the observed GPIOlib warning.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

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

Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Charles Keepax]

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

Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Linus Walleij]

On Wed, Nov 21, 2018 at 11:42 AM Marek Szyprowski
<m.szyprowski@samsung.com> 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

Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Linus Walleij]

On Wed, Nov 21, 2018 at 11:13 AM Charles Keepax
<ckeepax@opensource.cirrus.com> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

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

Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Linus Walleij]

On Thu, Nov 22, 2018 at 3:19 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 21, 2018 at 11:13 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> 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 <m.szyprowski@samsung.com>
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

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

Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs

[Charles Keepax]

On Thu, Nov 22, 2018 at 04:47:20PM +0100, Linus Walleij wrote:
> On Thu, Nov 22, 2018 at 3:19 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Nov 21, 2018 at 11:13 AM Charles Keepax
> > <ckeepax@opensource.cirrus.com> 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 <m.szyprowski@samsung.com>
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> >
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> 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