| Message ID | 1438790528-4435-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
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/
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/
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 --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; }
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(-)