[PATCH 1/1] regulator: Only enable disabled regulators on resume

[PATCH 1/1] regulator: Only enable disabled regulators on resume

[Javier Martinez Canillas]

After leaving from system wide suspend state, regulator_suspend_finish()
turn on regulators that may 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..8551400d57e4 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 1/1] regulator: Only enable disabled regulators on resume

[Doug Anderson]

Javier,

On Mon, Mar 2, 2015 at 12:40 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> After leaving from system wide suspend state, regulator_suspend_finish()
> turn on regulators that may 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(-)

I've tested this and it also fixes the problem that my patch
(regulator: core: Fix enable GPIO reference counting -
https://patchwork.kernel.org/patch/5903071) fixes.

As I said in the other conversation I think both patches could land.
...but maybe change your commit message to something like:

The _regulator_do_enable() call ought to be a no-op when called on an
already-enabled regulator.  However, as an optimization
_regulator_enable() doesn't call _regulator_do_enable() on an already
enabled regulator.  That means we never test the case of calling
_regulator_do_enable() during normal usage and there may be hidden
bugs or warnings.  We have seen warnings issued by the tps65090 driver
and bugs when using the GPIO enable pin.

Let's match the same optimization that _regulator_enable() in
regulator_suspend_finish().  That may speed up suspend/resume and also
avoids exposing hidden bugs.

>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index f2452148c8da..8551400d57e4 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)) {

Looking at _regulator_enable() I see that _regulator_is_enabled()
could return an error.  Should we be checking?  Maybe we should have a
helper function called by both callers?


I have tested this on my system and it works.  Other than the error
check / updated commit message this looks good to me.


-Doug

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Javier Martinez Canillas]

Hello Doug,

On 03/03/2015 06:24 PM, Doug Anderson wrote:
> Javier,
> 
> On Mon, Mar 2, 2015 at 12:40 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> After leaving from system wide suspend state, regulator_suspend_finish()
>> turn on regulators that may 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(-)
> 
> I've tested this and it also fixes the problem that my patch
> (regulator: core: Fix enable GPIO reference counting -
> https://patchwork.kernel.org/patch/5903071) fixes.
>

Thanks a lot for testing.
 
> As I said in the other conversation I think both patches could land.

Agreed that both patches should land.

> ...but maybe change your commit message to something like:
> 
> The _regulator_do_enable() call ought to be a no-op when called on an
> already-enabled regulator.  However, as an optimization
> _regulator_enable() doesn't call _regulator_do_enable() on an already
> enabled regulator.  That means we never test the case of calling
> _regulator_do_enable() during normal usage and there may be hidden
> bugs or warnings.  We have seen warnings issued by the tps65090 driver
> and bugs when using the GPIO enable pin.
>
> Let's match the same optimization that _regulator_enable() in
> regulator_suspend_finish().  That may speed up suspend/resume and also
> avoids exposing hidden bugs.
>

Right, I'll change the commit message since your suggestion is more clear.

>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index f2452148c8da..8551400d57e4 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)) {
> 
> Looking at _regulator_enable() I see that _regulator_is_enabled()
> could return an error.  Should we be checking?  Maybe we should have a
> helper function called by both callers?
>

Thanks for pointing that out. I'll change it on v2 as well.

> 
> I have tested this on my system and it works.  Other than the error
> check / updated commit message this looks good to me.
> 
>

I guess that means that I can include your Tested-by tag when
doing a re-spin? Please let me know otherwise.

> -Doug
> 

Best regards,
Javier

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Javier Martinez Canillas]

Hello Doug,

On 03/03/2015 08:05 PM, Javier Martinez Canillas wrote:
> On 03/03/2015 06:24 PM, Doug Anderson wrote:
>>>
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index f2452148c8da..8551400d57e4 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)) {
>> 
>> Looking at _regulator_enable() I see that _regulator_is_enabled()
>> could return an error.  Should we be checking?  Maybe we should have a
>> helper function called by both callers?
>>
> 
> Thanks for pointing that out. I'll change it on v2 as well.
> 

Looking at the code now I remember why I didn't checked for an error
in _regulator_is_enable(), sorry I wrote the patch months ago.

The thing is that _regulator_is_enabled() used to return -EINVAL if
the rdev didn't have an .is_enabled callback but that changed in
commit 9a7f6a4c6edc8 ("regulator: Assume regulators are enabled if
they don't report anything") and now returns 1 in that case. But
_regulator_enable() was not changed and is still checking for -EINVAL
which seems to me like a left over after the mentioned commit.

Also, _regulator_enable() is the only place that has a check for a
negative errno value returned by _regulator_is_enabled().

All other functions calling _regulator_is_enabled() seems to assume
that a return value != 0 means that the regulator is enabled.

Is true though that the rdev .is_enabled callback function may return
an error so I don't know if all those callers are missing a check or
if it's a design decision to assume that a regulator should be enabled
if the actual hardware state can't be obtained.

Best regards,
Javier

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Mark Brown]

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

On Wed, Mar 04, 2015 at 02:45:00PM +0100, Javier Martinez Canillas wrote:

> The thing is that _regulator_is_enabled() used to return -EINVAL if
> the rdev didn't have an .is_enabled callback but that changed in
> commit 9a7f6a4c6edc8 ("regulator: Assume regulators are enabled if
> they don't report anything") and now returns 1 in that case. But
> _regulator_enable() was not changed and is still checking for -EINVAL
> which seems to me like a left over after the mentioned commit.

You mean _do_enable(), not _enable() here.  It's not really a leftover
as the two operations are doing somewhat different things and the
changes are a bit separate, _is_enabled() is reporting the current state
while _do_enable() is making a change so it should fail if it can't do
that.  

A better way of writing it in the _do_enable() case is that it possibly
ought to be checking if the regulator is enabled before it does
anything, though for uncached regulator operations that then means an
extra I/O which isn't great.  Given that I think rather than ignoring
the missing op it should instead fall back to checking _is_enabled() -
that way if we can read the state but not change it the right thing will
happen.  I'll do a patch, probably tomorrow.

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

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Mark Brown]

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

On Mon, Mar 02, 2015 at 09:40:39PM +0100, Javier Martinez Canillas wrote:
> After leaving from system wide suspend state, regulator_suspend_finish()
> turn on regulators that may 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.

Applied, with Doug's commit message.  Thanks.

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

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Javier Martinez Canillas]

On 03/08/2015 08:38 PM, Mark Brown wrote:
> On Wed, Mar 04, 2015 at 02:45:00PM +0100, Javier Martinez Canillas wrote:
> 
>> The thing is that _regulator_is_enabled() used to return -EINVAL if
>> the rdev didn't have an .is_enabled callback but that changed in
>> commit 9a7f6a4c6edc8 ("regulator: Assume regulators are enabled if
>> they don't report anything") and now returns 1 in that case. But
>> _regulator_enable() was not changed and is still checking for -EINVAL
>> which seems to me like a left over after the mentioned commit.
> 
> You mean _do_enable(), not _enable() here.  It's not really a leftover

No, I meant _enable() here. What I said is that _enable() is checking
if -EINVAL was returned by _is_enabled():

static int _regulator_enable(struct regulator_dev *rdev)
{
...
		ret = _regulator_is_enabled(rdev);
		if (ret == -EINVAL || ret == 0) {
			if (!_regulator_can_change_status(rdev))
				return -EPERM;

			ret = _regulator_do_enable(rdev);
			if (ret < 0)
				return ret;

		} else if (ret < 0) {
			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
			return ret;
		}
...
}

and my point was that it is checking because _is_enabled() used to return
-EINVAL if the regulator driver didn't provide a .is_enabled callback:

static int _regulator_is_enabled(struct regulator_dev *rdev)
{
...
        if (!rdev->desc->ops->is_enabled)
               return -EINVAL;

        return rdev->desc->ops->is_enabled(rdev);
...
}

so, if a driver didn't provide a way to query if the regulator was enabled,
it was assumed that it was disabled. But after the mentioned commit, the
assumption was changed and now not having .is_enabled means that it's enabled:

static int _regulator_is_enabled(struct regulator_dev *rdev)
{
...
        if (!rdev->desc->ops->is_enabled)
               return 1;

        return rdev->desc->ops->is_enabled(rdev);
...
}

So my question was if _is_enabled() returning -EINVAL should still mean
that the regulator has to be enabled or the error has to be propagated
since now -EINVAL will be returned by the driver .is_enabled callback.

> as the two operations are doing somewhat different things and the
> changes are a bit separate, _is_enabled() is reporting the current state
> while _do_enable() is making a change so it should fail if it can't do
> that.  
>

Yes, I understand that.

> A better way of writing it in the _do_enable() case is that it possibly
> ought to be checking if the regulator is enabled before it does
> anything, though for uncached regulator operations that then means an
> extra I/O which isn't great.  Given that I think rather than ignoring
> the missing op it should instead fall back to checking _is_enabled() -
> that way if we can read the state but not change it the right thing will
> happen.  I'll do a patch, probably tomorrow.
> 

Best regards,
Javier

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Mark Brown]

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

On Mon, Mar 09, 2015 at 08:40:20AM +0100, Javier Martinez Canillas wrote:
> On 03/08/2015 08:38 PM, Mark Brown wrote:
> > On Wed, Mar 04, 2015 at 02:45:00PM +0100, Javier Martinez Canillas wrote:

> >> The thing is that _regulator_is_enabled() used to return -EINVAL if
> >> the rdev didn't have an .is_enabled callback but that changed in
> >> commit 9a7f6a4c6edc8 ("regulator: Assume regulators are enabled if
> >> they don't report anything") and now returns 1 in that case. But
> >> _regulator_enable() was not changed and is still checking for -EINVAL
> >> which seems to me like a left over after the mentioned commit.

> > You mean _do_enable(), not _enable() here.  It's not really a leftover

> No, I meant _enable() here. What I said is that _enable() is checking
> if -EINVAL was returned by _is_enabled():

Then we have an abstraction problem if we're trying to do things in
plain _enable() - _do_enable() is supposed to be hiding all this stuff.

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

Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

[Javier Martinez Canillas]

On 03/11/2015 11:57 AM, Mark Brown wrote:
> On Mon, Mar 09, 2015 at 08:40:20AM +0100, Javier Martinez Canillas wrote:
>> On 03/08/2015 08:38 PM, Mark Brown wrote:
>> > You mean _do_enable(), not _enable() here.  It's not really a leftover
> 
>> No, I meant _enable() here. What I said is that _enable() is checking
>> if -EINVAL was returned by _is_enabled():
> 
> Then we have an abstraction problem if we're trying to do things in
> plain _enable() - _do_enable() is supposed to be hiding all this stuff.
> 

Agreed.

Best regards,
Javier