regulator: wm8994: Don't use devres for enable GPIOs
PATCH v2 1/1 regulator: wm8994: Don't use devres for enable GPIOs
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.
---
Again only build tested.
Thanks,
Charles
drivers/regulator/wm8994-regulator.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/regulator/wm8994-regulator.c +9 -5
@@ -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; }
Signature · 1 line
2.11.0
Hi Charles,
Quoted 9 lines · 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 <[email protected]>Signed-off-by Charles Keepax <[email protected]>---
Again only build tested.
Thanks for the patch. It fixes the observed GPIOlib warning.
It might sense to add:
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.
Quoted 31 lines
Thanks,
Charlesdrivers/regulator/wm8994-regulator.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)drivers/regulator/wm8994-regulator.c +8
-5@@ -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;
drivers/regulator/wm8994-regulator.c static int wm8994_ldo_probe(struct platform_device *pdev) · line 193
Quoted 7 lines
drivers/regulator/wm8994-regulator.c +1
-0@@ -184,6 +187,7 @@ static int wm8994_ldo_probe(struct platform_device *pdev)return 0 ; err: +gpiod_put (gpiod);return ret; }
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
Quoted 7 lines · Marek Szyprowski
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 intoregulator_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
Quoted 5 lines · Marek Szyprowski
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
Quoted 6 lines · 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 <[email protected]>Signed-off-by Charles Keepax <[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
Quoted 11 lines · Linus Walleij
Quoted 6 lines · 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 <[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
Quoted 8 lines · Linus Walleij
Quoted 7 lines · Linus Walleij
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 6 lines
Sorry for the inconvenience.
Will send a patch soon.
Yours,
Linus Walleij
Thanks,
Charles