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 <[email protected]>
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
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 <[email protected]>
> 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>
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 <[email protected]>
> 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 <[email protected]>
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 <[email protected]>
---
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, ®ulator_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