[RFC PATCH] regulator: Fix recursive mutex lockdep warning

[RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Srinivas Kandagatla]

A recursive lockdep warning occurs if you call regulator_set_voltage()
on a load switches that are modelled as regulators with a parent supply as
there is no nesting annotation for the rdev->mutex.
To avoid this warning, use the unlocked version of the get_voltage().

wiithout this patch kernel throws up below warning:

 =============================================
 [ INFO: possible recursive locking detected ]
 4.2.0-rc5-dirty #132 Not tainted
 ---------------------------------------------
 swapper/0/1 is trying to acquire lock:
  (&rdev->mutex){+.+.+.}, at: [<c066b6e4>] regulator_get_voltage+0x44/0x68

 but task is already holding lock:
  (&rdev->mutex){+.+.+.}, at: [<c066d718>] regulator_set_voltage+0x50/0x168

 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

 3 locks held by swapper/0/1:
 #0:  (&dev->mutex){......}, at: [<c0756e34>] __driver_attach+0x58/0xa8
 #1:  (&dev->mutex){......}, at: [<c0756e44>] __driver_attach+0x68/0xa8
 #2:  (&rdev->mutex){+.+.+.}, at: [<c066d718>] regulator_set_voltage+0x50/0x168

 stack backtrace:
 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc5-dirty #132
 Hardware name: Qualcomm (Flattened Device Tree)
 [<c021af98>] (unwind_backtrace) from [<c021536c>] (show_stack+0x20/0x24)
 [<c021536c>] (show_stack) from [<c0ba6698>] (dump_stack+0x8c/0xc0)
 [<c0ba6698>] (dump_stack) from [<c02a8604>] (__lock_acquire+0x1bf4/0x1ec0)
 [<c02a8604>] (__lock_acquire) from [<c02a9020>] (lock_acquire+0xe0/0x300)
 [<c02a9020>] (lock_acquire) from [<c0baa7dc>] (mutex_lock_nested+0x88/0x45c)
 [<c0baa7dc>] (mutex_lock_nested) from [<c066b6e4>] (regulator_get_voltage+0x44/0x68)
 [<c066b6e4>] (regulator_get_voltage) from [<c066b5c8>] (_regulator_get_voltage+0xb8/0x190)
 [<c066b5c8>] (_regulator_get_voltage) from [<c066d7f4>] (regulator_set_voltage+0x12c/0x168)
 [<c066d7f4>] (regulator_set_voltage) from [<c09af5b4>] (mmci_sig_volt_switch+0x6c/0x110)
 [<c09af5b4>] (mmci_sig_volt_switch) from [<c099d8a4>] (mmc_power_up+0x78/0x114)
 [<c099d8a4>] (mmc_power_up) from [<c099e69c>] (mmc_start_host+0x54/0x7c)
 [<c099e69c>] (mmc_start_host) from [<c099f95c>] (mmc_add_host+0x6c/0x90)
 [<c099f95c>] (mmc_add_host) from [<c09b014c>] (mmci_probe+0x5ac/0x854)
 [<c09b014c>] (mmci_probe) from [<c063f5e4>] (amba_probe+0xdc/0x158)
 [<c063f5e4>] (amba_probe) from [<c0756d38>] (driver_probe_device+0x1dc/0x280)
 [<c0756d38>] (driver_probe_device) from [<c0756e80>] (__driver_attach+0xa4/0xa8)
 [<c0756e80>] (__driver_attach) from [<c0755120>] (bus_for_each_dev+0x64/0x98)
 [<c0755120>] (bus_for_each_dev) from [<c0756608>] (driver_attach+0x2c/0x30)
 [<c0756608>] (driver_attach) from [<c075634c>] (bus_add_driver+0xf8/0x204)
 [<c075634c>] (bus_add_driver) from [<c0757fb8>] (driver_register+0x88/0x104)
 [<c0757fb8>] (driver_register) from [<c063f418>] (amba_driver_register+0x50/0x64)
 [<c063f418>] (amba_driver_register) from [<c113e904>] (mmci_driver_init+0x14/0x1c)
 [<c113e904>] (mmci_driver_init) from [<c020adb4>] (do_one_initcall+0x90/0x1e4)
 [<c020adb4>] (do_one_initcall) from [<c10e4ea8>] (kernel_init_freeable+0x12c/0x1f4)
 [<c10e4ea8>] (kernel_init_freeable) from [<c0b9ec78>] (kernel_init+0x1c/0xfc)
 [<c0b9ec78>] (kernel_init) from [<c02114d8>] (ret_from_fork+0x14/0x3c)

Reported-by: Nicolas Dechesne <nicolas.dechesne@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3c3137a..7717b04 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2919,7 +2919,7 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 	} else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
 		ret = rdev->desc->fixed_uV;
 	} else if (rdev->supply) {
-		ret = regulator_get_voltage(rdev->supply);
+		ret = _regulator_get_voltage(rdev->supply->rdev);
 	} else {
 		return -EINVAL;
 	}
-- 
1.9.1

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Krzysztof Kozlowski]

2015-08-06 1:02 GMT+09:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
> A recursive lockdep warning occurs if you call regulator_set_voltage()
> on a load switches that are modelled as regulators with a parent supply as
> there is no nesting annotation for the rdev->mutex.
> To avoid this warning, use the unlocked version of the get_voltage().
>
> wiithout this patch kernel throws up below warning:
>
>  =============================================
>  [ INFO: possible recursive locking detected ]
>  4.2.0-rc5-dirty #132 Not tainted
>  ---------------------------------------------
>  swapper/0/1 is trying to acquire lock:
>   (&rdev->mutex){+.+.+.}, at: [<c066b6e4>] regulator_get_voltage+0x44/0x68
>
>  but task is already holding lock:
>   (&rdev->mutex){+.+.+.}, at: [<c066d718>] regulator_set_voltage+0x50/0x168
>
>  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
>
>  3 locks held by swapper/0/1:
>  #0:  (&dev->mutex){......}, at: [<c0756e34>] __driver_attach+0x58/0xa8
>  #1:  (&dev->mutex){......}, at: [<c0756e44>] __driver_attach+0x68/0xa8
>  #2:  (&rdev->mutex){+.+.+.}, at: [<c066d718>] regulator_set_voltage+0x50/0x168
>
>  stack backtrace:
>  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc5-dirty #132
>  Hardware name: Qualcomm (Flattened Device Tree)
>  [<c021af98>] (unwind_backtrace) from [<c021536c>] (show_stack+0x20/0x24)
>  [<c021536c>] (show_stack) from [<c0ba6698>] (dump_stack+0x8c/0xc0)
>  [<c0ba6698>] (dump_stack) from [<c02a8604>] (__lock_acquire+0x1bf4/0x1ec0)
>  [<c02a8604>] (__lock_acquire) from [<c02a9020>] (lock_acquire+0xe0/0x300)
>  [<c02a9020>] (lock_acquire) from [<c0baa7dc>] (mutex_lock_nested+0x88/0x45c)
>  [<c0baa7dc>] (mutex_lock_nested) from [<c066b6e4>] (regulator_get_voltage+0x44/0x68)
>  [<c066b6e4>] (regulator_get_voltage) from [<c066b5c8>] (_regulator_get_voltage+0xb8/0x190)
>  [<c066b5c8>] (_regulator_get_voltage) from [<c066d7f4>] (regulator_set_voltage+0x12c/0x168)
>  [<c066d7f4>] (regulator_set_voltage) from [<c09af5b4>] (mmci_sig_volt_switch+0x6c/0x110)
>  [<c09af5b4>] (mmci_sig_volt_switch) from [<c099d8a4>] (mmc_power_up+0x78/0x114)
>  [<c099d8a4>] (mmc_power_up) from [<c099e69c>] (mmc_start_host+0x54/0x7c)
>  [<c099e69c>] (mmc_start_host) from [<c099f95c>] (mmc_add_host+0x6c/0x90)
>  [<c099f95c>] (mmc_add_host) from [<c09b014c>] (mmci_probe+0x5ac/0x854)
>  [<c09b014c>] (mmci_probe) from [<c063f5e4>] (amba_probe+0xdc/0x158)
>  [<c063f5e4>] (amba_probe) from [<c0756d38>] (driver_probe_device+0x1dc/0x280)
>  [<c0756d38>] (driver_probe_device) from [<c0756e80>] (__driver_attach+0xa4/0xa8)
>  [<c0756e80>] (__driver_attach) from [<c0755120>] (bus_for_each_dev+0x64/0x98)
>  [<c0755120>] (bus_for_each_dev) from [<c0756608>] (driver_attach+0x2c/0x30)
>  [<c0756608>] (driver_attach) from [<c075634c>] (bus_add_driver+0xf8/0x204)
>  [<c075634c>] (bus_add_driver) from [<c0757fb8>] (driver_register+0x88/0x104)
>  [<c0757fb8>] (driver_register) from [<c063f418>] (amba_driver_register+0x50/0x64)
>  [<c063f418>] (amba_driver_register) from [<c113e904>] (mmci_driver_init+0x14/0x1c)
>  [<c113e904>] (mmci_driver_init) from [<c020adb4>] (do_one_initcall+0x90/0x1e4)
>  [<c020adb4>] (do_one_initcall) from [<c10e4ea8>] (kernel_init_freeable+0x12c/0x1f4)
>  [<c10e4ea8>] (kernel_init_freeable) from [<c0b9ec78>] (kernel_init+0x1c/0xfc)
>  [<c0b9ec78>] (kernel_init) from [<c02114d8>] (ret_from_fork+0x14/0x3c)
>
> Reported-by: Nicolas Dechesne <nicolas.dechesne@linaro.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/regulator/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 3c3137a..7717b04 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2919,7 +2919,7 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
>         } else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
>                 ret = rdev->desc->fixed_uV;
>         } else if (rdev->supply) {
> -               ret = regulator_get_voltage(rdev->supply);
> +               ret = _regulator_get_voltage(rdev->supply->rdev);

Is the 'rdev' and 'rdev->supply' same regulators? If not then you are
just hiding false warning by removing locks thus introducing real
issue...

Best regards,
Krzysztof

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Srinivas Kandagatla]

Thanks Krzysztof

On 06/08/15 02:39, Krzysztof Kozlowski wrote:
>> --- a/drivers/regulator/core.c
>> >+++ b/drivers/regulator/core.c
>> >@@ -2919,7 +2919,7 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
>> >         } else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages == 1)) {
>> >                 ret = rdev->desc->fixed_uV;
>> >         } else if (rdev->supply) {
>> >-               ret = regulator_get_voltage(rdev->supply);
>> >+               ret = _regulator_get_voltage(rdev->supply->rdev);
> Is the 'rdev' and 'rdev->supply' same regulators? If not then you are
> just hiding false warning by removing locks thus introducing real
> issue...
They are the not the same regulators, and hence they are not locking the 
same mutex, looks like this is a false positive warning from lockdep. I 
can't think of any use case which could result in ABBA type lockup too, 
so we can ignore this patch for now.

Not sure why did the lockdep think that this is same lock :-)

--srini
>
> Best regards,
> Krzysztof

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Krzysztof Kozlowski]

On 06.08.2015 16:29, Srinivas Kandagatla wrote:
> Thanks Krzysztof
> 
> On 06/08/15 02:39, Krzysztof Kozlowski wrote:
>>> --- a/drivers/regulator/core.c
>>> >+++ b/drivers/regulator/core.c
>>> >@@ -2919,7 +2919,7 @@ static int _regulator_get_voltage(struct
>>> regulator_dev *rdev)
>>> >         } else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages
>>> == 1)) {
>>> >                 ret = rdev->desc->fixed_uV;
>>> >         } else if (rdev->supply) {
>>> >-               ret = regulator_get_voltage(rdev->supply);
>>> >+               ret = _regulator_get_voltage(rdev->supply->rdev);
>> Is the 'rdev' and 'rdev->supply' same regulators? If not then you are
>> just hiding false warning by removing locks thus introducing real
>> issue...
> They are the not the same regulators, and hence they are not locking the
> same mutex, looks like this is a false positive warning from lockdep. I
> can't think of any use case which could result in ABBA type lockup too,
> so we can ignore this patch for now.
> 
> Not sure why did the lockdep think that this is same lock :-)

I think the warning appears because the class of lock is the same but
there is nesting information:
	"May be due to missing lock nesting notation"
Fixing this would require adding the nesting information.

Best regards,
Krzysztof

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Krzysztof Kozlowski]

2015-08-06 16:35 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> On 06.08.2015 16:29, Srinivas Kandagatla wrote:
>> Thanks Krzysztof
>>
>> On 06/08/15 02:39, Krzysztof Kozlowski wrote:
>>>> --- a/drivers/regulator/core.c
>>>> >+++ b/drivers/regulator/core.c
>>>> >@@ -2919,7 +2919,7 @@ static int _regulator_get_voltage(struct
>>>> regulator_dev *rdev)
>>>> >         } else if (rdev->desc->fixed_uV && (rdev->desc->n_voltages
>>>> == 1)) {
>>>> >                 ret = rdev->desc->fixed_uV;
>>>> >         } else if (rdev->supply) {
>>>> >-               ret = regulator_get_voltage(rdev->supply);
>>>> >+               ret = _regulator_get_voltage(rdev->supply->rdev);
>>> Is the 'rdev' and 'rdev->supply' same regulators? If not then you are
>>> just hiding false warning by removing locks thus introducing real
>>> issue...
>> They are the not the same regulators, and hence they are not locking the
>> same mutex, looks like this is a false positive warning from lockdep. I
>> can't think of any use case which could result in ABBA type lockup too,
>> so we can ignore this patch for now.
>>
>> Not sure why did the lockdep think that this is same lock :-)
>
> I think the warning appears because the class of lock is the same but
> there is nesting information:

Crap... s/there is nesting/there is no nesting/

Maybe it is worth to add it... or remove the lockdep warning if it hits often.

>         "May be due to missing lock nesting notation"
> Fixing this would require adding the nesting information.

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Mark Brown]

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

On Wed, Aug 05, 2015 at 05:02:08PM +0100, Srinivas Kandagatla wrote:
> A recursive lockdep warning occurs if you call regulator_set_voltage()
> on a load switches that are modelled as regulators with a parent supply as
> there is no nesting annotation for the rdev->mutex.
> To avoid this warning, use the unlocked version of the get_voltage().

No, just completely removing the locking is broken - the locking is
there for a reason!  This needs some lockdep dance, either something
like what we have for regmaps with a class per regulator or something
more fancy but whatever's going on just hacking out locking to shut up
warnings from lockdep is clearly not a good idea.

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

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Srinivas Kandagatla]

On 06/08/15 10:43, Mark Brown wrote:
> On Wed, Aug 05, 2015 at 05:02:08PM +0100, Srinivas Kandagatla wrote:
>> A recursive lockdep warning occurs if you call regulator_set_voltage()
>> on a load switches that are modelled as regulators with a parent supply as
>> there is no nesting annotation for the rdev->mutex.
>> To avoid this warning, use the unlocked version of the get_voltage().
>
> No, just completely removing the locking is broken - the locking is
> there for a reason!  This needs some lockdep dance, either something

Yes, I totally agree, removing locking would have more regressions.

> like what we have for regmaps with a class per regulator or something

lock_class per regulator makes more sense, I will try to cookup an RFC 
patch.
> more fancy but whatever's going on just hacking out locking to shut up
> warnings from lockdep is clearly not a good idea.
>

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Mark Brown]

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

On Thu, Aug 06, 2015 at 12:01:29PM +0100, Srinivas Kandagatla wrote:
> On 06/08/15 10:43, Mark Brown wrote:

> >like what we have for regmaps with a class per regulator or something

> lock_class per regulator makes more sense, I will try to cookup an RFC
> patch.

There's an issue there with all lock classes needing to be statically
allocated which makes things a bit tricky.

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

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Srinivas Kandagatla]

On 06/08/15 12:40, Mark Brown wrote:
> On Thu, Aug 06, 2015 at 12:01:29PM +0100, Srinivas Kandagatla wrote:
>> On 06/08/15 10:43, Mark Brown wrote:
>
>>> like what we have for regmaps with a class per regulator or something
>
>> lock_class per regulator makes more sense, I will try to cookup an RFC
>> patch.
>
> There's an issue there with all lock classes needing to be statically
> allocated which makes things a bit tricky.
Yes, just realized it with "BUG: key edadc07c not in .data!"



>

Re: [RFC PATCH] regulator: Fix recursive mutex lockdep warning

[Mark Brown]

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

On Thu, Aug 06, 2015 at 12:49:22PM +0100, Srinivas Kandagatla wrote:
> On 06/08/15 12:40, Mark Brown wrote:

> >>lock_class per regulator makes more sense, I will try to cookup an RFC
> >>patch.

> >There's an issue there with all lock classes needing to be statically
> >allocated which makes things a bit tricky.

> Yes, just realized it with "BUG: key edadc07c not in .data!"

Yeah, it's a bit nasty.  We went round this a bit with regmap and
unfortunately didn't manage to come up with any great ideas.  The regmap
workaround just landed in my tree, I'm not sure it's really great for
regulator though.

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