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

[Krzysztof Kozlowski]

2015-08-06 1:02 GMT+09:00 Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>:
> 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@xxxxxxxxxx>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/