diff mbox

[RFC] regulator: Fix recursive mutex lockdep warning

Message ID 1438790528-4435-1-git-send-email-srinivas.kandagatla@linaro.org
State New
Headers show

Commit Message

Srinivas Kandagatla Aug. 5, 2015, 4:02 p.m. UTC
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(-)

Comments

Srinivas Kandagatla Aug. 6, 2015, 7:29 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Srinivas Kandagatla Aug. 6, 2015, 11:01 a.m. UTC | #2
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.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Srinivas Kandagatla Aug. 6, 2015, 11:49 a.m. UTC | #3
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!"



>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

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;
 	}