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@xxxxxxxxxxxx>
> 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]: