* [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs @ 2018-11-21 10:13 ` Charles Keepax 2018-11-21 10:42 ` Marek Szyprowski 2018-11-22 14:19 ` Linus Walleij 0 siblings, 2 replies; 7+ messages in thread From: Charles Keepax @ 2018-11-21 10:13 UTC (permalink / raw) To: broonie; +Cc: linus.walleij, lgirdwood, m.szyprowski, linux-kernel, patches 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs 2018-11-21 10:13 ` [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs Charles Keepax @ 2018-11-21 10:42 ` Marek Szyprowski 2018-11-21 10:59 ` Charles Keepax 2018-11-22 14:20 ` Linus Walleij 2018-11-22 14:19 ` Linus Walleij 1 sibling, 2 replies; 7+ messages in thread From: Marek Szyprowski @ 2018-11-21 10:42 UTC (permalink / raw) To: Charles Keepax, broonie; +Cc: linus.walleij, lgirdwood, linux-kernel, patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs 2018-11-21 10:42 ` Marek Szyprowski @ 2018-11-21 10:59 ` Charles Keepax 2018-11-22 14:20 ` Linus Walleij 1 sibling, 0 replies; 7+ messages in thread From: Charles Keepax @ 2018-11-21 10:59 UTC (permalink / raw) To: Marek Szyprowski; +Cc: broonie, linus.walleij, lgirdwood, linux-kernel, patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs 2018-11-21 10:42 ` Marek Szyprowski 2018-11-21 10:59 ` Charles Keepax @ 2018-11-22 14:20 ` Linus Walleij 1 sibling, 0 replies; 7+ messages in thread From: Linus Walleij @ 2018-11-22 14:20 UTC (permalink / raw) To: Marek Szyprowski Cc: Charles Keepax, Mark Brown, Liam Girdwood, linux-kernel, patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs 2018-11-21 10:13 ` [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs Charles Keepax 2018-11-21 10:42 ` Marek Szyprowski @ 2018-11-22 14:19 ` Linus Walleij 2018-11-22 15:47 ` Linus Walleij 1 sibling, 1 reply; 7+ messages in thread From: Linus Walleij @ 2018-11-22 14:19 UTC (permalink / raw) To: Charles Keepax Cc: Mark Brown, Liam Girdwood, Marek Szyprowski, linux-kernel, patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs 2018-11-22 14:19 ` Linus Walleij @ 2018-11-22 15:47 ` Linus Walleij 2018-11-22 17:21 ` Charles Keepax 0 siblings, 1 reply; 7+ messages in thread From: Linus Walleij @ 2018-11-22 15:47 UTC (permalink / raw) To: Charles Keepax Cc: Mark Brown, Liam Girdwood, Marek Szyprowski, linux-kernel, patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs 2018-11-22 15:47 ` Linus Walleij @ 2018-11-22 17:21 ` Charles Keepax 0 siblings, 0 replies; 7+ messages in thread From: Charles Keepax @ 2018-11-22 17:21 UTC (permalink / raw) To: Linus Walleij Cc: Mark Brown, Liam Girdwood, Marek Szyprowski, linux-kernel, patches 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-22 17:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20181121101410epcas3p27a2a577c3b5a0608f7808ff733fab9d1@epcas3p2.samsung.com> 2018-11-21 10:13 ` [PATCH v2] regulator: wm8994: Don't use devres for enable GPIOs Charles Keepax 2018-11-21 10:42 ` Marek Szyprowski 2018-11-21 10:59 ` Charles Keepax 2018-11-22 14:20 ` Linus Walleij 2018-11-22 14:19 ` Linus Walleij 2018-11-22 15:47 ` Linus Walleij 2018-11-22 17:21 ` Charles Keepax
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).