[00/10] Regulator ena_gpiod fixups
mbox series

Message ID 20181128104350.31902-1-linus.walleij@linaro.org
Headers show
Series
  • Regulator ena_gpiod fixups
Related show

Message

Linus Walleij Nov. 28, 2018, 10:43 a.m. UTC
We noticed a refcounting fight between the kernel device
core devm_* and the regulator core refcounting, pertaining
to enable GPIOd:s that may be shared between multiple
regulators.

Fix this with a series that hand it all over to the
regulator core and remove any devm_* stuff pertaining
to these GPIOs.

This includes a patch to gpiolib to make gpiod_get_from_node()
available. Just go ahead and apply this to the regulator
tree.

If these need to go for fixes or not is up to the
regulator maintainer, but all commits have a proper
Fixes: tag in case they would. I have noted in the
four last commits that the gpiolib patch is a
prerequisite.

Linus Walleij (10):
  regulator: fixed: Let core handle GPIO descriptor
  regulator: lm363x: Let core handle GPIO descriptor
  regulator: lp8788-ldo: Let core handle GPIO descriptor
  regulator: max8952: Let core handle GPIO descriptor
  regulator: max8973: Let core handle GPIO descriptor
  gpio: Export gpiod_get_from_of_node()
  regulator: da9211: Let core handle GPIO descriptors
  regulator: max77686: Let core handle GPIO descriptor
  regulator: s5m8767: Let core handle GPIO descriptors
  regulator: tps65090: Let core handle GPIO descriptors

 drivers/gpio/gpiolib.h                 |  6 -----
 drivers/regulator/da9211-regulator.c   |  4 +--
 drivers/regulator/fixed.c              |  4 ++-
 drivers/regulator/lm363x-regulator.c   |  8 ++++--
 drivers/regulator/lp8788-ldo.c         |  4 ++-
 drivers/regulator/max77686-regulator.c |  3 +--
 drivers/regulator/max8952.c            |  8 +++---
 drivers/regulator/max8973-regulator.c  | 12 ++++++---
 drivers/regulator/s5m8767.c            | 37 ++++++++++++++++++--------
 drivers/regulator/tps65090-regulator.c | 10 +++----
 include/linux/gpio/consumer.h          | 13 +++++++++
 11 files changed, 72 insertions(+), 37 deletions(-)

Comments

Charles Keepax Nov. 28, 2018, 3:22 p.m. UTC | #1
On Wed, Nov 28, 2018 at 11:43:40AM +0100, Linus Walleij wrote:
>  drivers/gpio/gpiolib.h                 |  6 -----
>  drivers/regulator/da9211-regulator.c   |  4 +--
>  drivers/regulator/fixed.c              |  4 ++-
>  drivers/regulator/lm363x-regulator.c   |  8 ++++--
>  drivers/regulator/lp8788-ldo.c         |  4 ++-
>  drivers/regulator/max77686-regulator.c |  3 +--
>  drivers/regulator/max8952.c            |  8 +++---
>  drivers/regulator/max8973-regulator.c  | 12 ++++++---
>  drivers/regulator/s5m8767.c            | 37 ++++++++++++++++++--------
>  drivers/regulator/tps65090-regulator.c | 10 +++----
>  include/linux/gpio/consumer.h          | 13 +++++++++
>  11 files changed, 72 insertions(+), 37 deletions(-)

It looks like the patches are assuming the regulator core,
doesn't free the GPIO on an error, however that is not true in
all cases. If only a single regulator has requested the GPIO then
all the error paths after the call to regulator_ena_gpio_request
in regulator_register will free the GPIO. Although this is not the
case if more than one regulator has requested the GPIO.

Thanks,
charles
Linus Walleij Nov. 29, 2018, 3:52 p.m. UTC | #2
On Wed, Nov 28, 2018 at 4:22 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> It looks like the patches are assuming the regulator core,
> doesn't free the GPIO on an error, however that is not true in
> all cases. If only a single regulator has requested the GPIO then
> all the error paths after the call to regulator_ena_gpio_request
> in regulator_register will free the GPIO.

I guess part of it is that I should make sure not to gpiod_put()
if the [devm_]regulator_register() fails, I will go over the
series with that in mind!

Essentially the semantic is that the  [devm_]regulator_register()
call will immediately take ownership of the descriptor
and place it in the regulator core.

I'll check!

> Although this is not the
> case if more than one regulator has requested the GPIO.

This should be fine since the regulator core refcounts it,
when all the other regulators drops it, gpiod_put() will be
called as the refcount goes down to 0.

Yours,
Linus Walleij
Mark Brown Nov. 29, 2018, 4:29 p.m. UTC | #3
On Thu, Nov 29, 2018 at 04:52:04PM +0100, Linus Walleij wrote:

> Essentially the semantic is that the  [devm_]regulator_register()
> call will immediately take ownership of the descriptor
> and place it in the regulator core.

I think that's going to be the easiest thing to work with, it's more
pain in the regulator core but simpler for all the users which is
probably a good balance.
Bartosz Golaszewski Nov. 29, 2018, 6:38 p.m. UTC | #4
śr., 28 lis 2018 o 11:43 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> We noticed a refcounting fight between the kernel device
> core devm_* and the regulator core refcounting, pertaining
> to enable GPIOd:s that may be shared between multiple
> regulators.
>
> Fix this with a series that hand it all over to the
> regulator core and remove any devm_* stuff pertaining
> to these GPIOs.
>
> This includes a patch to gpiolib to make gpiod_get_from_node()
> available. Just go ahead and apply this to the regulator
> tree.
>
> If these need to go for fixes or not is up to the
> regulator maintainer, but all commits have a proper
> Fixes: tag in case they would. I have noted in the
> four last commits that the gpiolib patch is a
> prerequisite.
>
> Linus Walleij (10):
>   regulator: fixed: Let core handle GPIO descriptor
>   regulator: lm363x: Let core handle GPIO descriptor
>   regulator: lp8788-ldo: Let core handle GPIO descriptor
>   regulator: max8952: Let core handle GPIO descriptor
>   regulator: max8973: Let core handle GPIO descriptor
>   gpio: Export gpiod_get_from_of_node()
>   regulator: da9211: Let core handle GPIO descriptors
>   regulator: max77686: Let core handle GPIO descriptor
>   regulator: s5m8767: Let core handle GPIO descriptors
>   regulator: tps65090: Let core handle GPIO descriptors
>
>  drivers/gpio/gpiolib.h                 |  6 -----
>  drivers/regulator/da9211-regulator.c   |  4 +--
>  drivers/regulator/fixed.c              |  4 ++-
>  drivers/regulator/lm363x-regulator.c   |  8 ++++--
>  drivers/regulator/lp8788-ldo.c         |  4 ++-
>  drivers/regulator/max77686-regulator.c |  3 +--
>  drivers/regulator/max8952.c            |  8 +++---
>  drivers/regulator/max8973-regulator.c  | 12 ++++++---
>  drivers/regulator/s5m8767.c            | 37 ++++++++++++++++++--------
>  drivers/regulator/tps65090-regulator.c | 10 +++----
>  include/linux/gpio/consumer.h          | 13 +++++++++
>  11 files changed, 72 insertions(+), 37 deletions(-)
>
> --
> 2.19.1
>

I'm wondering if instead of using the non-devm variants of
gpiod_get_*() routines, we shouldn't provide helpers in the regulator
framework that would be named accordingly, for example:
regulator_gpiod_get_optional() etc. even if all they do is call the
relevant gpiolib function. Those helpers could then be documented as
passing the control over GPIO lines over to the regulator subsystem.

The reason for that is that most driver developers will automatically
use devm functions whenever available and having a single non-devm
function without any comment used in a driver normally using devres
looks like a bug. Expect people sending "fixes" in a couple months.

Bart
Mark Brown Nov. 29, 2018, 7:01 p.m. UTC | #5
On Thu, Nov 29, 2018 at 07:38:20PM +0100, Bartosz Golaszewski wrote:

> I'm wondering if instead of using the non-devm variants of
> gpiod_get_*() routines, we shouldn't provide helpers in the regulator
> framework that would be named accordingly, for example:
> regulator_gpiod_get_optional() etc. even if all they do is call the
> relevant gpiolib function. Those helpers could then be documented as
> passing the control over GPIO lines over to the regulator subsystem.

> The reason for that is that most driver developers will automatically
> use devm functions whenever available and having a single non-devm
> function without any comment used in a driver normally using devres
> looks like a bug. Expect people sending "fixes" in a couple months.

I predict that people would then immediately start demanding devm_
variants of that function...
Bartosz Golaszewski Nov. 30, 2018, 8:35 a.m. UTC | #6
czw., 29 lis 2018 o 20:01 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Thu, Nov 29, 2018 at 07:38:20PM +0100, Bartosz Golaszewski wrote:
>
> > I'm wondering if instead of using the non-devm variants of
> > gpiod_get_*() routines, we shouldn't provide helpers in the regulator
> > framework that would be named accordingly, for example:
> > regulator_gpiod_get_optional() etc. even if all they do is call the
> > relevant gpiolib function. Those helpers could then be documented as
> > passing the control over GPIO lines over to the regulator subsystem.
>
> > The reason for that is that most driver developers will automatically
> > use devm functions whenever available and having a single non-devm
> > function without any comment used in a driver normally using devres
> > looks like a bug. Expect people sending "fixes" in a couple months.
>
> I predict that people would then immediately start demanding devm_
> variants of that function...

At least we could document it in the code.

If I wouldn't know about the reason for not using devm and saw a stray
gpiod_get() without a corresponding put() I'd probably send a patch to
fix it, but if I saw something like regulator_gpiod_get(), I'd look at
what this routine does.

Matter of taste I guess, but I'd prefer the latter. At the very least
we could add a comment to each call saying that the regulator
framework will take care of that resource.

Bart
Linus Walleij Nov. 30, 2018, 10:56 a.m. UTC | #7
On Fri, Nov 30, 2018 at 9:35 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> At least we could document it in the code.

I've put some comments in my patch set, I will check if I can add some
more in the less-than-obvious spots.

Yours,
Linus Walleij