[PATCH] regulator: core: Fix enable GPIO reference counting

[PATCH] regulator: core: Fix enable GPIO reference counting

[Doug Anderson]

It is possible for _regulator_do_enable() to be called for an
already-enabled rdev, like in regulator_suspend_finish().  If we were
using an enable pin (rdev->ena_pin is set) then we'd end up
incrementing the reference count in regulator_ena_gpio_ctrl() over and
over again without a decrement.  That prevented the GPIO from going to
the "off" state even after all users were disabled.

Fix this by avoiding the call to regulator_ena_gpio_ctrl() when it's
not needed.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Fixes: 967cfb18c0e3 ("regulator: core: manage enable GPIO list")
---
FYI: this was developed and tested against a 3.14 kernel with
backports; I've done basic boot testing against upstream and sanity
checked the code but haven't done as extensive testing there.

 drivers/regulator/core.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b899947..9daccbb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1839,10 +1839,12 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 	}
 
 	if (rdev->ena_pin) {
-		ret = regulator_ena_gpio_ctrl(rdev, true);
-		if (ret < 0)
-			return ret;
-		rdev->ena_gpio_state = 1;
+		if (!rdev->ena_gpio_state) {
+			ret = regulator_ena_gpio_ctrl(rdev, true);
+			if (ret < 0)
+				return ret;
+			rdev->ena_gpio_state = 1;
+		}
 	} else if (rdev->desc->ops->enable) {
 		ret = rdev->desc->ops->enable(rdev);
 		if (ret < 0)
@@ -1939,10 +1941,12 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 	trace_regulator_disable(rdev_get_name(rdev));
 
 	if (rdev->ena_pin) {
-		ret = regulator_ena_gpio_ctrl(rdev, false);
-		if (ret < 0)
-			return ret;
-		rdev->ena_gpio_state = 0;
+		if (rdev->ena_gpio_state) {
+			ret = regulator_ena_gpio_ctrl(rdev, false);
+			if (ret < 0)
+				return ret;
+			rdev->ena_gpio_state = 0;
+		}
 
 	} else if (rdev->desc->ops->disable) {
 		ret = rdev->desc->ops->disable(rdev);
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Greg KH]

On Fri, Feb 27, 2015 at 11:41:03AM -0800, Doug Anderson wrote:
> It is possible for _regulator_do_enable() to be called for an
> already-enabled rdev, like in regulator_suspend_finish().  If we were
> using an enable pin (rdev->ena_pin is set) then we'd end up
> incrementing the reference count in regulator_ena_gpio_ctrl() over and
> over again without a decrement.  That prevented the GPIO from going to
> the "off" state even after all users were disabled.
> 
> Fix this by avoiding the call to regulator_ena_gpio_ctrl() when it's
> not needed.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Fixes: 967cfb18c0e3 ("regulator: core: manage enable GPIO list")
> ---
> FYI: this was developed and tested against a 3.14 kernel with
> backports; I've done basic boot testing against upstream and sanity
> checked the code but haven't done as extensive testing there.
> 
>  drivers/regulator/core.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Javier Martinez Canillas]

Hello Doug,

On 02/27/2015 08:41 PM, Doug Anderson wrote:
> It is possible for _regulator_do_enable() to be called for an
> already-enabled rdev, like in regulator_suspend_finish().  If we were
> using an enable pin (rdev->ena_pin is set) then we'd end up
> incrementing the reference count in regulator_ena_gpio_ctrl() over and
> over again without a decrement.  That prevented the GPIO from going to
> the "off" state even after all users were disabled.
> 
> Fix this by avoiding the call to regulator_ena_gpio_ctrl() when it's
> not needed.
>

I noticed the same problem in regulator_suspend_finish() when I was working
on S2R for Exynos a couple of months ago and had patch [0] on my local tree
but never found the time to do extensive testing so I never posted it.

In my case a tps65090 FET regulator was tried to be enabled twice and
tps65090_fet_enable() was failing printing a warning.
 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Fixes: 967cfb18c0e3 ("regulator: core: manage enable GPIO list")
> ---
> FYI: this was developed and tested against a 3.14 kernel with
> backports; I've done basic boot testing against upstream and sanity
> checked the code but haven't done as extensive testing there.
> 
>  drivers/regulator/core.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b899947..9daccbb 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1839,10 +1839,12 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>  	}
>  
>  	if (rdev->ena_pin) {
> -		ret = regulator_ena_gpio_ctrl(rdev, true);
> -		if (ret < 0)
> -			return ret;
> -		rdev->ena_gpio_state = 1;
> +		if (!rdev->ena_gpio_state) {
> +			ret = regulator_ena_gpio_ctrl(rdev, true);
> +			if (ret < 0)
> +				return ret;
> +			rdev->ena_gpio_state = 1;
> +		}
>  	} else if (rdev->desc->ops->enable) {
>  		ret = rdev->desc->ops->enable(rdev);
>  		if (ret < 0)

Your approach to check in _regulator_do_enable() is more generic though
since it covers future issues and not only regulator_suspend_finish()
but otoh it only cover the case for GPIO enabled regulators so you may
also check for other type of regulators using _regulator_is_enabled()?

I see that the check is already in _regulator_enable() so another option
is to call _regulator_enable() instead of _regulator_do_enable() in
regulator_suspend_finish().

Best regards,
Javier

[0]:
>From 71bb25eff5c2aac4a7b6878f205f3f8905c61363 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Thu, 16 Oct 2014 01:31:20 +0100
Subject: [PATCH 1/1] regulator: Only enable disabled regulators on resume

After leaving from system wide suspend state, regulator_suspend_finish()
turn on regulators that might be turned off by regulator_suspend_prepare
but it tries to enable all regulators that have an enable count > 0 or
that were marked as "always-on" regardless if those were disabled or not.

Trying to enable an already enabled regulator may cause issues so is
better to skip enabling regulators that were not disabled before suspend.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/regulator/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f2452148c8da..4f1932aa10bc 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3816,9 +3816,11 @@ int regulator_suspend_finish(void)
 	list_for_each_entry(rdev, &regulator_list, list) {
 		mutex_lock(&rdev->mutex);
 		if (rdev->use_count > 0  || rdev->constraints->always_on) {
-			error = _regulator_do_enable(rdev);
-			if (error)
-				ret = error;
+			if (!_regulator_is_enabled(rdev)) {
+			    error = _regulator_do_enable(rdev);
+			    if (error)
+				    ret = error;
+			}
 		} else {
 			if (!have_full_constraints())
 				goto unlock;
-- 
2.1.3

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Fri, Feb 27, 2015 at 11:41:03AM -0800, Doug Anderson wrote:
> It is possible for _regulator_do_enable() to be called for an
> already-enabled rdev, like in regulator_suspend_finish().  If we were
> using an enable pin (rdev->ena_pin is set) then we'd end up
> incrementing the reference count in regulator_ena_gpio_ctrl() over and
> over again without a decrement.  That prevented the GPIO from going to
> the "off" state even after all users were disabled.

> Fix this by avoiding the call to regulator_ena_gpio_ctrl() when it's
> not needed.

There's a big jump in this changelog where you assert that we're
avoiding the call "when it's not needed" without explaining the
situations in which this is the case or why.  

Looking at the code it seems that you're adding checks to skip calls in
the standard enable and disable paths but not touching other paths,
based on this patch by itself I can't tell if this is a good idea or
not.  It certainly doesn't feel robust - if we're missing reference
counting skipping operations seems likely to lead to other bugs popping
up elsewhere when the other user that isn't doing a disable currently
decides to start doing so.

> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Fixes: 967cfb18c0e3 ("regulator: core: manage enable GPIO list")

Fixes normally goes first.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Fri, Feb 27, 2015 at 10:01:23PM +0100, Javier Martinez Canillas wrote:

> I noticed the same problem in regulator_suspend_finish() when I was working
> on S2R for Exynos a couple of months ago and had patch [0] on my local tree
> but never found the time to do extensive testing so I never posted it.

Please don't bury patches in the middle of mails where they're hard to
apply if they're useful.

> I see that the check is already in _regulator_enable() so another option
> is to call _regulator_enable() instead of _regulator_do_enable() in
> regulator_suspend_finish().

I'm not entirely sure what "the check" is?

> Trying to enable an already enabled regulator may cause issues so is
> better to skip enabling regulators that were not disabled before suspend.

>  		mutex_lock(&rdev->mutex);
>  		if (rdev->use_count > 0  || rdev->constraints->always_on) {
> -			error = _regulator_do_enable(rdev);
> -			if (error)
> -				ret = error;
> +			if (!_regulator_is_enabled(rdev)) {
> +			    error = _regulator_do_enable(rdev);
> +			    if (error)
> +				    ret = error;
> +			}

This seems like a better fix or at least a better approach - essentially
the assumption in most of the code is that regulator enables are just
register writes so repeated updates don't have any effect.  We may need
a specific per client count here...  I've not looked at the code and I
only got back to the UK this morning so I'm not going to start now.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Javier Martinez Canillas]

Hello Mark,

On 03/02/2015 07:57 PM, Mark Brown wrote:
> On Fri, Feb 27, 2015 at 10:01:23PM +0100, Javier Martinez Canillas wrote:
> 
>> I noticed the same problem in regulator_suspend_finish() when I was working
>> on S2R for Exynos a couple of months ago and had patch [0] on my local tree
>> but never found the time to do extensive testing so I never posted it.
> 
> Please don't bury patches in the middle of mails where they're hard to
> apply if they're useful.
>

Sorry, if my intention was you to apply the patch then I would had posted
it properly. But what I wanted was to share that I had the same issue and
my approach to see if that also fixed Doug's issue.

Otherwise is hard to maintain a conversation across different threads.
 
>> I see that the check is already in _regulator_enable() so another option
>> is to call _regulator_enable() instead of _regulator_do_enable() in
>> regulator_suspend_finish().
> 
> I'm not entirely sure what "the check" is?
>

The check I was referring to is _regulator_is_enabled() but now looking again
I see that _regulator_enable() can't be used in regulator_suspend_finish()
because that will increment the reference counting which is wrong.

>> Trying to enable an already enabled regulator may cause issues so is
>> better to skip enabling regulators that were not disabled before suspend.
> 
>>  		mutex_lock(&rdev->mutex);
>>  		if (rdev->use_count > 0  || rdev->constraints->always_on) {
>> -			error = _regulator_do_enable(rdev);
>> -			if (error)
>> -				ret = error;
>> +			if (!_regulator_is_enabled(rdev)) {
>> +			    error = _regulator_do_enable(rdev);
>> +			    if (error)
>> +				    ret = error;
>> +			}
> 
> This seems like a better fix or at least a better approach - essentially
> the assumption in most of the code is that regulator enables are just
> register writes so repeated updates don't have any effect.  We may need

Which doesn't seem to be the case for all regulators since at least I got
failures when a FET in the tps65090 pmu was tried to be enabled twice.

> a specific per client count here...  I've not looked at the code and I

Sorry, I'm not sure I understood what you meant. The suspend path:

suspend_prepare() -> suspend_set_state() -> .set_suspend_*

doesn't decrement use_count so is correct to call _regulator_do_enable()
directly. The problem is the assumption that all regulators were either
disabled on suspend or that enabling an enabled regulator is a no-op.

I'll post as a proper patch so you can review it.

Best regards,
Javier

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Doug Anderson]

Mark,

On Mon, Mar 2, 2015 at 10:47 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 27, 2015 at 11:41:03AM -0800, Doug Anderson wrote:
>> It is possible for _regulator_do_enable() to be called for an
>> already-enabled rdev, like in regulator_suspend_finish().  If we were
>> using an enable pin (rdev->ena_pin is set) then we'd end up
>> incrementing the reference count in regulator_ena_gpio_ctrl() over and
>> over again without a decrement.  That prevented the GPIO from going to
>> the "off" state even after all users were disabled.
>
>> Fix this by avoiding the call to regulator_ena_gpio_ctrl() when it's
>> not needed.
>
> There's a big jump in this changelog where you assert that we're
> avoiding the call "when it's not needed" without explaining the
> situations in which this is the case or why.
>
> Looking at the code it seems that you're adding checks to skip calls in
> the standard enable and disable paths but not touching other paths,
> based on this patch by itself I can't tell if this is a good idea or
> not.  It certainly doesn't feel robust - if we're missing reference
> counting skipping operations seems likely to lead to other bugs popping
> up elsewhere when the other user that isn't doing a disable currently
> decides to start doing so.

I guess it depends on whether _regulator_do_enable() on an
already-enabled rdev is supposed to be a noop or not.  My assumption
was that it was supposed to be a noop with reference counting handled
by _regulator_enable().

My assumption is that regulator drivers themselves shouldn't do
reference counting.  That is: if you call
rdev->desc->ops->enable(rdev) twice you should not have to call
rdev->desc->ops->disable(rdev) twice to disable.  Right?  That means
my fix is making the "ena_pin" symmetric to how normal regulator
drivers work.

The refcounting being skipped by my patch is refcounting that's used
only when the same GPIO is shared by more than one regulator.  That
is, if "vcc_a" uses GPIO1 and "vcc_b" also uses "GPIO1" we need
refcounting.  GPIO1 will be in the "on" state if either vcc_a or vcc_b
is on.  The problem came in because _regulator_do_enable() was
incrementing the shared refcount every time it was called even if the
specific regulator was already on.


Anyway, I looked at Javier's patch and it's also fine / reasonable.
...and in fact I would argue that possibly we could take both patches.
Javier's patch eliminates the one known place where
_regulator_do_enable() is called for an already-enabled regulator and
my patch means that if someone else adds a new call we won't end up
back in this same subtle bug.  I'm happy to update the CL desc to make
it more obvious if you'd like.

-Doug

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

On Mon, Mar 02, 2015 at 01:13:56PM -0800, Doug Anderson wrote:
> On Mon, Mar 2, 2015 at 10:47 AM, Mark Brown <broonie@kernel.org> wrote:

> > Looking at the code it seems that you're adding checks to skip calls in
> > the standard enable and disable paths but not touching other paths,
> > based on this patch by itself I can't tell if this is a good idea or
> > not.  It certainly doesn't feel robust - if we're missing reference
> > counting skipping operations seems likely to lead to other bugs popping
> > up elsewhere when the other user that isn't doing a disable currently
> > decides to start doing so.

> I guess it depends on whether _regulator_do_enable() on an
> already-enabled rdev is supposed to be a noop or not.  My assumption
> was that it was supposed to be a noop with reference counting handled
> by _regulator_enable().

Yes, that's the point.

> My assumption is that regulator drivers themselves shouldn't do
> reference counting.  That is: if you call
> rdev->desc->ops->enable(rdev) twice you should not have to call
> rdev->desc->ops->disable(rdev) twice to disable.  Right?  That means
> my fix is making the "ena_pin" symmetric to how normal regulator
> drivers work.

> The refcounting being skipped by my patch is refcounting that's used
> only when the same GPIO is shared by more than one regulator.  That
> is, if "vcc_a" uses GPIO1 and "vcc_b" also uses "GPIO1" we need
> refcounting.  GPIO1 will be in the "on" state if either vcc_a or vcc_b
> is on.  The problem came in because _regulator_do_enable() was
> incrementing the shared refcount every time it was called even if the
> specific regulator was already on.

This is all analysis which should have been in the changelog...
possibly not quite so verbosely but it should be there.

> Anyway, I looked at Javier's patch and it's also fine / reasonable.
> ...and in fact I would argue that possibly we could take both patches.
> Javier's patch eliminates the one known place where
> _regulator_do_enable() is called for an already-enabled regulator and
> my patch means that if someone else adds a new call we won't end up
> back in this same subtle bug.  I'm happy to update the CL desc to make
> it more obvious if you'd like.

Yes, the changelog definitely needs to be *much* clearer.  Especially
for things like locking and reference counting the changelog needs to
explain what the fix is and why it's safe, without that working it is a
lot harder to do a review as the reviewer needs to go back and check
that everything has been thought through properly.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

On Mon, Mar 02, 2015 at 09:21:45PM +0100, Javier Martinez Canillas wrote:
> On 03/02/2015 07:57 PM, Mark Brown wrote:

> > a specific per client count here...  I've not looked at the code and I

> Sorry, I'm not sure I understood what you meant. The suspend path:

We need to check if this specific client has done an enable (which we
actually do record already).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Doug Anderson]

Hi,

On Tue, Mar 3, 2015 at 6:23 AM, Mark Brown <broonie@kernel.org> wrote:
>> My assumption is that regulator drivers themselves shouldn't do
>> reference counting.  That is: if you call
>> rdev->desc->ops->enable(rdev) twice you should not have to call
>> rdev->desc->ops->disable(rdev) twice to disable.  Right?  That means
>> my fix is making the "ena_pin" symmetric to how normal regulator
>> drivers work.
>
>> The refcounting being skipped by my patch is refcounting that's used
>> only when the same GPIO is shared by more than one regulator.  That
>> is, if "vcc_a" uses GPIO1 and "vcc_b" also uses "GPIO1" we need
>> refcounting.  GPIO1 will be in the "on" state if either vcc_a or vcc_b
>> is on.  The problem came in because _regulator_do_enable() was
>> incrementing the shared refcount every time it was called even if the
>> specific regulator was already on.
>
> This is all analysis which should have been in the changelog...
> possibly not quite so verbosely but it should be there.
>
>> Anyway, I looked at Javier's patch and it's also fine / reasonable.
>> ...and in fact I would argue that possibly we could take both patches.
>> Javier's patch eliminates the one known place where
>> _regulator_do_enable() is called for an already-enabled regulator and
>> my patch means that if someone else adds a new call we won't end up
>> back in this same subtle bug.  I'm happy to update the CL desc to make
>> it more obvious if you'd like.
>
> Yes, the changelog definitely needs to be *much* clearer.  Especially
> for things like locking and reference counting the changelog needs to
> explain what the fix is and why it's safe, without that working it is a
> lot harder to do a review as the reviewer needs to go back and check
> that everything has been thought through properly.

OK, so I started working on a nice clean changelog of this.  ...and
then I found a bug.  :(

It looks as if "ena_gpio_state" is not quite what I thought it was and
I think is not actually consistent in the regulator framework itself.
In _regulator_do_enable() and _regulator_do_disable() is clear that
ena_gpio_state is 1 when an "rdev" is enabled and 0 when the "rdev" is
disabled.  That was my assumption.  It's also clear in
_regulator_is_enabled().

...but then I looked in regulator_register().  There you can see that
ena_gpio_state could be set to 1 if you've got an active low GPIO that
is disabled at boot.  That totally throws my logic for a loop.  Also
with my patch the reference counting will be all messed up for active
high / boot on regulators.  :(


I'll fix up my patch to make "ena_gpio_state" just be the state of the
"rdev" and not the true state of the pin.  Without redoing the whole
shared GPIO infrastructure I think this is the best I can do.

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On Tue, Mar 03, 2015 at 03:21:21PM -0800, Doug Anderson wrote:

> It looks as if "ena_gpio_state" is not quite what I thought it was and
> I think is not actually consistent in the regulator framework itself.
> In _regulator_do_enable() and _regulator_do_disable() is clear that
> ena_gpio_state is 1 when an "rdev" is enabled and 0 when the "rdev" is
> disabled.  That was my assumption.  It's also clear in
> _regulator_is_enabled().

> ...but then I looked in regulator_register().  There you can see that
> ena_gpio_state could be set to 1 if you've got an active low GPIO that
> is disabled at boot.  That totally throws my logic for a loop.  Also
> with my patch the reference counting will be all messed up for active
> high / boot on regulators.  :(

Isn't that just a bug in the registration code?  I'd not be entirely
surprised if that were the case.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: Fix enable GPIO reference counting

[Doug Anderson]

Mark,

On Wed, Mar 4, 2015 at 3:27 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Mar 03, 2015 at 03:21:21PM -0800, Doug Anderson wrote:
>
>> It looks as if "ena_gpio_state" is not quite what I thought it was and
>> I think is not actually consistent in the regulator framework itself.
>> In _regulator_do_enable() and _regulator_do_disable() is clear that
>> ena_gpio_state is 1 when an "rdev" is enabled and 0 when the "rdev" is
>> disabled.  That was my assumption.  It's also clear in
>> _regulator_is_enabled().
>
>> ...but then I looked in regulator_register().  There you can see that
>> ena_gpio_state could be set to 1 if you've got an active low GPIO that
>> is disabled at boot.  That totally throws my logic for a loop.  Also
>> with my patch the reference counting will be all messed up for active
>> high / boot on regulators.  :(
>
> Isn't that just a bug in the registration code?  I'd not be entirely
> surprised if that were the case.

Yes, I'm pretty sure that's the case too and that's what I've assumed
in V2 of the patch that I sent up yesterday.  In my testing is caused
no problems, but of course my testing of my previous version also
showed no problems on my particular boards until I found the right
place to put printouts to show that the internal state was a bit
confused...

-Doug