LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] regulator: core: Fix enable GPIO reference counting
@ 2015-02-27 19:41 Doug Anderson
  2015-02-27 20:08 ` Greg KH
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Doug Anderson @ 2015-02-27 19:41 UTC (permalink / raw)
  To: Mark Brown
  Cc: milo.kim, axel.lin, Dmitry Torokhov, olof, javier.martinez,
	Paul Stewart, Doug Anderson, stable, lgirdwood, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-02-27 19:41 [PATCH] regulator: core: Fix enable GPIO reference counting Doug Anderson
@ 2015-02-27 20:08 ` Greg KH
  2015-02-27 21:01 ` Javier Martinez Canillas
  2015-03-02 18:47 ` Mark Brown
  2 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2015-02-27 20:08 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Mark Brown, milo.kim, axel.lin, Dmitry Torokhov, olof,
	javier.martinez, Paul Stewart, stable, lgirdwood, linux-kernel

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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-02-27 19:41 [PATCH] regulator: core: Fix enable GPIO reference counting Doug Anderson
  2015-02-27 20:08 ` Greg KH
@ 2015-02-27 21:01 ` Javier Martinez Canillas
  2015-03-02 18:57   ` Mark Brown
  2015-03-02 18:47 ` Mark Brown
  2 siblings, 1 reply; 12+ messages in thread
From: Javier Martinez Canillas @ 2015-02-27 21:01 UTC (permalink / raw)
  To: Doug Anderson, Mark Brown
  Cc: milo.kim, axel.lin, Dmitry Torokhov, olof, Paul Stewart, stable,
	lgirdwood, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-02-27 19:41 [PATCH] regulator: core: Fix enable GPIO reference counting Doug Anderson
  2015-02-27 20:08 ` Greg KH
  2015-02-27 21:01 ` Javier Martinez Canillas
@ 2015-03-02 18:47 ` Mark Brown
  2015-03-02 21:13   ` Doug Anderson
  2 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2015-03-02 18:47 UTC (permalink / raw)
  To: Doug Anderson
  Cc: milo.kim, axel.lin, Dmitry Torokhov, olof, javier.martinez,
	Paul Stewart, stable, lgirdwood, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-02-27 21:01 ` Javier Martinez Canillas
@ 2015-03-02 18:57   ` Mark Brown
  2015-03-02 20:21     ` Javier Martinez Canillas
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2015-03-02 18:57 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Doug Anderson, milo.kim, axel.lin, Dmitry Torokhov, olof,
	Paul Stewart, stable, lgirdwood, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-02 18:57   ` Mark Brown
@ 2015-03-02 20:21     ` Javier Martinez Canillas
  2015-03-03 14:24       ` Mark Brown
  0 siblings, 1 reply; 12+ messages in thread
From: Javier Martinez Canillas @ 2015-03-02 20:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Doug Anderson, milo.kim, axel.lin, Dmitry Torokhov, olof,
	Paul Stewart, stable, lgirdwood, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-02 18:47 ` Mark Brown
@ 2015-03-02 21:13   ` Doug Anderson
  2015-03-03 14:23     ` Mark Brown
  0 siblings, 1 reply; 12+ messages in thread
From: Doug Anderson @ 2015-03-02 21:13 UTC (permalink / raw)
  To: Mark Brown
  Cc: milo.kim, Axel Lin, Dmitry Torokhov, Olof Johansson,
	Javier Martinez Canillas, Paul Stewart, stable, Liam Girdwood,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-02 21:13   ` Doug Anderson
@ 2015-03-03 14:23     ` Mark Brown
  2015-03-03 23:21       ` Doug Anderson
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2015-03-03 14:23 UTC (permalink / raw)
  To: Doug Anderson
  Cc: milo.kim, Axel Lin, Dmitry Torokhov, Olof Johansson,
	Javier Martinez Canillas, Paul Stewart, stable, Liam Girdwood,
	linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-02 20:21     ` Javier Martinez Canillas
@ 2015-03-03 14:24       ` Mark Brown
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2015-03-03 14:24 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Doug Anderson, milo.kim, axel.lin, Dmitry Torokhov, olof,
	Paul Stewart, stable, lgirdwood, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-03 14:23     ` Mark Brown
@ 2015-03-03 23:21       ` Doug Anderson
  2015-03-04 11:27         ` Mark Brown
  0 siblings, 1 reply; 12+ messages in thread
From: Doug Anderson @ 2015-03-03 23:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: milo.kim, Axel Lin, Dmitry Torokhov, Olof Johansson,
	Javier Martinez Canillas, Paul Stewart, stable, Liam Girdwood,
	linux-kernel

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-03 23:21       ` Doug Anderson
@ 2015-03-04 11:27         ` Mark Brown
  2015-03-04 17:13           ` Doug Anderson
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2015-03-04 11:27 UTC (permalink / raw)
  To: Doug Anderson
  Cc: milo.kim, Axel Lin, Dmitry Torokhov, Olof Johansson,
	Javier Martinez Canillas, Paul Stewart, stable, Liam Girdwood,
	linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] regulator: core: Fix enable GPIO reference counting
  2015-03-04 11:27         ` Mark Brown
@ 2015-03-04 17:13           ` Doug Anderson
  0 siblings, 0 replies; 12+ messages in thread
From: Doug Anderson @ 2015-03-04 17:13 UTC (permalink / raw)
  To: Mark Brown
  Cc: milo.kim, Axel Lin, Dmitry Torokhov, Olof Johansson,
	Javier Martinez Canillas, Paul Stewart, stable, Liam Girdwood,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-03-04 17:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-27 19:41 [PATCH] regulator: core: Fix enable GPIO reference counting Doug Anderson
2015-02-27 20:08 ` Greg KH
2015-02-27 21:01 ` Javier Martinez Canillas
2015-03-02 18:57   ` Mark Brown
2015-03-02 20:21     ` Javier Martinez Canillas
2015-03-03 14:24       ` Mark Brown
2015-03-02 18:47 ` Mark Brown
2015-03-02 21:13   ` Doug Anderson
2015-03-03 14:23     ` Mark Brown
2015-03-03 23:21       ` Doug Anderson
2015-03-04 11:27         ` Mark Brown
2015-03-04 17:13           ` Doug Anderson

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).