[PATCH] regulator: disable supply regulator if it is enabled for boot-on

[PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

If supply regulator is enabled because of boot-on (not always-on)
then disable regulator need to be call if regulator have some
user or full constraint has been enabled.
This will make sure that reference count of supply regulator
is in sync with child regulator's state.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Reported-by: Rabin Vincent <rabin.vincent@gmail.com>
---
 drivers/regulator/core.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0fffeae..4632909 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3614,8 +3614,11 @@ static int __init regulator_init_complete(void)
 
 		mutex_lock(&rdev->mutex);
 
-		if (rdev->use_count)
+		if (rdev->use_count) {
+			if (rdev->supply && c->boot_on)
+				regulator_disable(rdev->supply);
 			goto unlock;
+		}
 
 		/* If we can't read the status assume it's on. */
 		if (ops->is_enabled)
@@ -3634,6 +3637,8 @@ static int __init regulator_init_complete(void)
 			if (ret != 0) {
 				rdev_err(rdev, "couldn't disable: %d\n", ret);
 			}
+			if (rdev->supply)
+				regulator_disable(rdev->supply);
 		} else {
 			/* The intention is that in future we will
 			 * assume that full constraints are provided
-- 
1.7.1.1

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Rabin Vincent]

2012/8/14 Laxman Dewangan <ldewangan@nvidia.com>:
> If supply regulator is enabled because of boot-on (not always-on)
> then disable regulator need to be call if regulator have some
> user or full constraint has been enabled.
> This will make sure that reference count of supply regulator
> is in sync with child regulator's state.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

 =============================================
 [ INFO: possible recursive locking detected ]
 3.4.0+ #71 Not tainted
 ---------------------------------------------
 swapper/0/1 is trying to acquire lock:
  (&rdev->mutex){+.+.+.}, at: [<c0306c60>] regulator_disable+0x2c/0x6c

 but task is already holding lock:
  (&rdev->mutex){+.+.+.}, at: [<c0a0d1f4>] regulator_init_complete+0x70/0x1c0

 other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(&rdev->mutex);
   lock(&rdev->mutex);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

 2 locks held by swapper/0/1:
  #0:  (regulator_list_mutex){+.+.+.}, at: [<c0a0d1a8>]
regulator_init_complete+0x24/0x1c0
  #1:  (&rdev->mutex){+.+.+.}, at: [<c0a0d1f4>]
regulator_init_complete+0x70/0x1c0

 stack backtrace:
 [<c0017ab4>] (unwind_backtrace+0x0/0x148) from [<c06e86a0>]
(dump_stack+0x20/0x24)
 [<c06e86a0>] (dump_stack+0x20/0x24) from [<c008e6d0>]
(check_deadlock+0x358/0x44c)
 [<c008e6d0>] (check_deadlock+0x358/0x44c) from [<c008f7b4>]
(validate_chain+0x410/0x710)
 [<c008f7b4>] (validate_chain+0x410/0x710) from [<c0090034>]
(__lock_acquire+0x580/0xb3c)
 [<c0090034>] (__lock_acquire+0x580/0xb3c) from [<c0090690>]
(lock_acquire+0xa0/0x138)
 [<c0090690>] (lock_acquire+0xa0/0x138) from [<c06ea0d0>]
(mutex_lock_nested+0x5c/0x364)
 [<c06ea0d0>] (mutex_lock_nested+0x5c/0x364) from [<c0306c60>]
(regulator_disable+0x2c/0x6c)
 [<c0306c60>] (regulator_disable+0x2c/0x6c) from [<c0a0d2cc>]
(regulator_init_complete+0x148/0x1c0)
 [<c0a0d2cc>] (regulator_init_complete+0x148/0x1c0) from [<c0008644>]
(do_one_initcall+0x50/0x1a8)
 [<c0008644>] (do_one_initcall+0x50/0x1a8) from [<c09ee3dc>]
(kernel_init+0xf4/0x1d4)
 [<c09ee3dc>] (kernel_init+0xf4/0x1d4) from [<c0010cfc>]
(kernel_thread_exit+0x0/0x8)

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

Hi Rabin,
Is it resolved the issue you mentioned?

Thanks,
Laxman

On Tuesday 14 August 2012 03:37 PM, Laxman Dewangan wrote:
> If supply regulator is enabled because of boot-on (not always-on)
> then disable regulator need to be call if regulator have some
> user or full constraint has been enabled.
> This will make sure that reference count of supply regulator
> is in sync with child regulator's state.
>
> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
> Reported-by: Rabin Vincent<rabin.vincent@gmail.com>
> ---
>   drivers/regulator/core.c |    7 ++++++-
>   1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 0fffeae..4632909 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3614,8 +3614,11 @@ static int __init regulator_init_complete(void)
>
>   		mutex_lock(&rdev->mutex);
>
> -		if (rdev->use_count)
> +		if (rdev->use_count) {
> +			if (rdev->supply&&  c->boot_on)
> +				regulator_disable(rdev->supply);
>   			goto unlock;
> +		}
>
>   		/* If we can't read the status assume it's on. */
>   		if (ops->is_enabled)
> @@ -3634,6 +3637,8 @@ static int __init regulator_init_complete(void)
>   			if (ret != 0) {
>   				rdev_err(rdev, "couldn't disable: %d\n", ret);
>   			}
> +			if (rdev->supply)
> +				regulator_disable(rdev->supply);
>   		} else {
>   			/* The intention is that in future we will
>   			 * assume that full constraints are provided

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Mark Brown]

On Wed, Aug 22, 2012 at 03:15:12PM +0530, Laxman Dewangan wrote:
> Hi Rabin,
> Is it resolved the issue you mentioned?

He replied to your mail pointing out that it generated a lockdep issue.

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

On Thursday 23 August 2012 08:23 PM, Mark Brown wrote:
> On Wed, Aug 22, 2012 at 03:15:12PM +0530, Laxman Dewangan wrote:
>> Hi Rabin,
>> Is it resolved the issue you mentioned?
> He replied to your mail pointing out that it generated a lockdep issue.
Thanks Mark for letting me know. The mail transfer to some other folder 
of my mail due to some issue in outlook.
Let me create some more scenario to reproduce this.

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

On Thursday 23 August 2012 08:13 PM, Laxman Dewangan wrote:
> On Thursday 23 August 2012 08:23 PM, Mark Brown wrote:
>> On Wed, Aug 22, 2012 at 03:15:12PM +0530, Laxman Dewangan wrote:
>>> Hi Rabin,
>>> Is it resolved the issue you mentioned?
>> He replied to your mail pointing out that it generated a lockdep issue.
> Thanks Mark for letting me know. The mail transfer to some other folder
> of my mail due to some issue in outlook.
> Let me create some more scenario to reproduce this.
>
>

Hi Rabin,
I tried to reproduce the issue but could not able to do this.
Can you please send me your board/dt files where you are porviding 
platform data for regulator?
This will help me to reproduce the issue.

Thanks,
Laxman

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Rabin Vincent]

On Fri, Aug 24, 2012 at 11:22:05PM +0530, Laxman Dewangan wrote:
> I tried to reproduce the issue but could not able to do this.
> Can you please send me your board/dt files where you are porviding
> platform data for regulator?
> This will help me to reproduce the issue.

Here's a dts patch:

diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index dba53fd..386eafa 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -207,5 +207,20 @@
 			regulator-max-microvolt = <3300000>;
 			regulator-always-on;
 		};
+
+		vbat: fixedregulator@1 {
+			compatible = "regulator-fixed";
+			regulator-name = "vbat";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		fixedregulator@2 {
+			compatible = "regulator-fixed";
+			regulator-name = "vtest1";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			vin-supply = <&vbat>;
+			regulator-boot-on;
+		};
 	};
 };

If you want to test it with fixed regulators, you'll need the hack below
to bypass the ops->disable check in regulator_init_complete(). 

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 185468c..05f3028 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -129,9 +129,16 @@ static int fixed_voltage_list_voltage(struct regulator_dev *dev,
 	return data->microvolts;
 }
 
+static int fixed_enable(struct regulator_dev *dev)
+{
+	return 0;
+}
+
 static struct regulator_ops fixed_voltage_ops = {
 	.get_voltage = fixed_voltage_get_voltage,
 	.list_voltage = fixed_voltage_list_voltage,
+	.disable = fixed_enable,
+	.enable = fixed_enable,
 };
 
 static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

On Saturday 25 August 2012 05:10 AM, Rabin Vincent wrote:
> On Fri, Aug 24, 2012 at 11:22:05PM +0530, Laxman Dewangan wrote:
>> I tried to reproduce the issue but could not able to do this.
>> Can you please send me your board/dt files where you are porviding
>> platform data for regulator?
>> This will help me to reproduce the issue.
> Here's a dts patch:
>
Hi Rabin,

I tried to reproduce the lockup issue with the following change but not 
seeing any lockup issue.
I implement enable/disable for fixed regulator and printed the string as 
enable/disable
During registration I got the log as
[    0.349955] Registering the regulator vbat
[    0.354352] vbat: 3300 mV
[    0.357260] Registering the regulator vtest1
[    0.361810] fixed_enable(): Enabling vtest1
[    0.366144] vtest1: 3300 mV
[    0.369177] vtest1: supplied by vbat
::::::::::::::::

and when init_complete() get called then it happened as

[    1.509855] vtest1: disabling
[    1.512813] fixed_disable(): Disabling vtest1
[    1.517158] fixed_disable(): Disabling vbat
[    1.521342] vbat: disabling
[    1.524124] fixed_disable(): Disabling vbat


Also reviewing the change, I am not seeing any call trace where the 
recursive locking happening.

Please help to reproduce/understand the issue.

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Rabin Vincent]

2012/8/28 Laxman Dewangan <ldewangan@nvidia.com>:
> I tried to reproduce the lockup issue with the following change but not
> seeing any lockup issue.

Did you enable CONFIG_PROVE_LOCKING?

> Also reviewing the change, I am not seeing any call trace where the
> recursive locking happening.

There's probably no actual recursive locking, but the lockdep warning
itself is a problem which must be eliminated.  You could perhaps do this
by doing the regulator_disable(rdev->supply); after you mutex unlock the
rdev->mutex.

Re: [PATCH] regulator: disable supply regulator if it is enabled for boot-on

[Laxman Dewangan]

On Thursday 23 August 2012 08:13 PM, Laxman Dewangan wrote:
> On Thursday 23 August 2012 08:23 PM, Mark Brown wrote:
>> On Wed, Aug 22, 2012 at 03:15:12PM +0530, Laxman Dewangan wrote:
>>> Hi Rabin,
>>> Is it resolved the issue you mentioned?
>> He replied to your mail pointing out that it generated a lockdep issue.
> Thanks Mark for letting me know. The mail transfer to some other folder
> of my mail due to some issue in outlook.
> Let me create some more scenario to reproduce this.
>
>

Hi Rabin,
I tried to reproduce the issue but could not able to do this.
Can you please send me your board/dt files where you are porviding 
platform data for regulator?
This will help me to reproduce the issue.

Thanks,
Laxman