[PATCH] regulator: Add lockdep asserts to help detecting locking misuse

[PATCH] regulator: Add lockdep asserts to help detecting locking misuse

[Krzysztof Kozlowski]

Add lockdep_assert_held_once() to functions explicitly mentioning that
rdev or regulator_list mutex must be held. Using WARN_ONCE shouldn't
pollute the dmesg to much.

The patch (if CONFIG_LOCKDEP enabled) will show warnings in certain
regulators calling regulator_notifier_call_chain() without rdev->mutex
held.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Warnings for missing locks when calling regulator_notifier_call_chain()
should appear on many regulators except wm8350-regulator.c, e.g.:
da9055-regulator.c, da9062-regulator.c, da9063-regulator.c,
da9211-regulator.c, wm831x-dcdc.c and few more.

The question is whether the lock during that call should be held?
---
 drivers/regulator/core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 443eaab933fc..2d11731db7cb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -640,6 +640,8 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	int current_uA = 0, output_uV, input_uV, err;
 	unsigned int mode;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	/*
 	 * first check to see if we can set modes at all, otherwise just
 	 * tell the consumer everything is OK.
@@ -758,6 +760,8 @@ static int suspend_set_state(struct regulator_dev *rdev,
 /* locks held by caller */
 static int suspend_prepare(struct regulator_dev *rdev, suspend_state_t state)
 {
+	lockdep_assert_held_once(&rdev->mutex);
+
 	if (!rdev->constraints)
 		return -EINVAL;
 
@@ -1555,6 +1559,8 @@ static void _regulator_put(struct regulator *regulator)
 	if (regulator == NULL || IS_ERR(regulator))
 		return;
 
+	lockdep_assert_held_once(&regulator_list_mutex);
+
 	rdev = regulator->rdev;
 
 	debugfs_remove_recursive(regulator->debugfs);
@@ -1933,6 +1939,8 @@ static int _regulator_enable(struct regulator_dev *rdev)
 {
 	int ret;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	/* check voltage and requested load before enabling */
 	if (rdev->constraints &&
 	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
@@ -2033,6 +2041,8 @@ static int _regulator_disable(struct regulator_dev *rdev)
 {
 	int ret = 0;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	if (WARN(rdev->use_count <= 0,
 		 "unbalanced disables for %s\n", rdev_get_name(rdev)))
 		return -EIO;
@@ -2111,6 +2121,8 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
 {
 	int ret = 0;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	ret = _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
 			REGULATOR_EVENT_PRE_DISABLE, NULL);
 	if (ret & NOTIFY_STOP_MASK)
@@ -3407,6 +3419,8 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
+	lockdep_assert_held_once(&rdev->mutex);
+
 	_notifier_call_chain(rdev, event, data);
 	return NOTIFY_DONE;
 
-- 
1.9.1

Re: [PATCH] regulator: Add lockdep asserts to help detecting locking misuse

[Mark Brown]

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

On Wed, Jun 10, 2015 at 03:23:12PM +0900, Krzysztof Kozlowski wrote:
> Add lockdep_assert_held_once() to functions explicitly mentioning that
> rdev or regulator_list mutex must be held. Using WARN_ONCE shouldn't
> pollute the dmesg to much.

Applied, thanks.

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

Applied "regulator: Add lockdep asserts to help detecting locking misuse" to the regulator tree

[Mark Brown]

The patch

   regulator: Add lockdep asserts to help detecting locking misuse

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 70cfef26267474f94ff4a988fb45ff78442b1cf4 Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Date: Mon, 29 Jun 2015 09:04:43 +0900
Subject: [PATCH] regulator: Add lockdep asserts to help detecting locking
 misuse

Add lockdep_assert_held_once() to functions explicitly mentioning that
rdev or regulator_list mutex must be held. Using WARN_ONCE shouldn't
pollute the dmesg to much.

The patch (if CONFIG_LOCKDEP enabled) will show warnings in certain
regulators calling regulator_notifier_call_chain() without rdev->mutex
held.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c9f72019bd68..68b616580533 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -640,6 +640,8 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	int current_uA = 0, output_uV, input_uV, err;
 	unsigned int mode;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	/*
 	 * first check to see if we can set modes at all, otherwise just
 	 * tell the consumer everything is OK.
@@ -760,6 +762,8 @@ static int suspend_set_state(struct regulator_dev *rdev,
 /* locks held by caller */
 static int suspend_prepare(struct regulator_dev *rdev, suspend_state_t state)
 {
+	lockdep_assert_held_once(&rdev->mutex);
+
 	if (!rdev->constraints)
 		return -EINVAL;
 
@@ -1587,6 +1591,8 @@ static void _regulator_put(struct regulator *regulator)
 	if (regulator == NULL || IS_ERR(regulator))
 		return;
 
+	lockdep_assert_held_once(&regulator_list_mutex);
+
 	rdev = regulator->rdev;
 
 	debugfs_remove_recursive(regulator->debugfs);
@@ -1965,6 +1971,8 @@ static int _regulator_enable(struct regulator_dev *rdev)
 {
 	int ret;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	/* check voltage and requested load before enabling */
 	if (rdev->constraints &&
 	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
@@ -2065,6 +2073,8 @@ static int _regulator_disable(struct regulator_dev *rdev)
 {
 	int ret = 0;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	if (WARN(rdev->use_count <= 0,
 		 "unbalanced disables for %s\n", rdev_get_name(rdev)))
 		return -EIO;
@@ -2143,6 +2153,8 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
 {
 	int ret = 0;
 
+	lockdep_assert_held_once(&rdev->mutex);
+
 	ret = _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
 			REGULATOR_EVENT_PRE_DISABLE, NULL);
 	if (ret & NOTIFY_STOP_MASK)
@@ -3439,6 +3451,8 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
+	lockdep_assert_held_once(&rdev->mutex);
+
 	_notifier_call_chain(rdev, event, data);
 	return NOTIFY_DONE;
 
-- 
2.1.4

Re: [PATCH] regulator: Add lockdep asserts to help detecting locking misuse

[Geert Uytterhoeven]

Hi Krzysztof,

On Wed, Jun 10, 2015 at 8:23 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> Add lockdep_assert_held_once() to functions explicitly mentioning that
> rdev or regulator_list mutex must be held. Using WARN_ONCE shouldn't
> pollute the dmesg to much.
>
> The patch (if CONFIG_LOCKDEP enabled) will show warnings in certain
> regulators calling regulator_notifier_call_chain() without rdev->mutex
> held.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
> ---
>
> Warnings for missing locks when calling regulator_notifier_call_chain()
> should appear on many regulators except wm8350-regulator.c, e.g.:
> da9055-regulator.c, da9062-regulator.c, da9063-regulator.c,
> da9211-regulator.c, wm831x-dcdc.c and few more.
>
> The question is whether the lock during that call should be held?

That was a (so far, not counting the "Applied, thanks!") unanswered question?

For the first time ever, I got:

        drivers/regulator/core.c:3480 regulator_notifier_call_chain+0x54/0x80()

due to da9210_irq_handler() not taking the mutex.

Drivers calling regulator_notifier_call_chain() from a threaded interrupt
handler should be OK calling mutex_lock().

Does anyone have plans to fix all affected drivers?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] regulator: Add lockdep asserts to help detecting locking misuse

[Mark Brown]

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

On Wed, Aug 19, 2015 at 03:08:34PM +0200, Geert Uytterhoeven wrote:

> Drivers calling regulator_notifier_call_chain() from a threaded interrupt
> handler should be OK calling mutex_lock().

> Does anyone have plans to fix all affected drivers?

I'm not aware of anyone who's shown an interest.

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

Re: [PATCH] regulator: Add lockdep asserts to help detecting locking misuse

[Krzysztof Kozlowski]

On 19.08.2015 22:08, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Wed, Jun 10, 2015 at 8:23 AM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> Add lockdep_assert_held_once() to functions explicitly mentioning that
>> rdev or regulator_list mutex must be held. Using WARN_ONCE shouldn't
>> pollute the dmesg to much.
>>
>> The patch (if CONFIG_LOCKDEP enabled) will show warnings in certain
>> regulators calling regulator_notifier_call_chain() without rdev->mutex
>> held.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
>> ---
>>
>> Warnings for missing locks when calling regulator_notifier_call_chain()
>> should appear on many regulators except wm8350-regulator.c, e.g.:
>> da9055-regulator.c, da9062-regulator.c, da9063-regulator.c,
>> da9211-regulator.c, wm831x-dcdc.c and few more.
>>
>> The question is whether the lock during that call should be held?
> 
> That was a (so far, not counting the "Applied, thanks!") unanswered question?
> 
> For the first time ever, I got:
> 
>         drivers/regulator/core.c:3480 regulator_notifier_call_chain+0x54/0x80()
> 
> due to da9210_irq_handler() not taking the mutex.
> 
> Drivers calling regulator_notifier_call_chain() from a threaded interrupt
> handler should be OK calling mutex_lock().
> 
> Does anyone have plans to fix all affected drivers?

Question is still unanswered. I don't have plans to fix the drivers
because I don't have necessary hardware. Blindly fixing such minor issue
could do more harm than good. I just polluted the dmesg with WARN hoping
that this will wake up someone :) .

Best regards,
Krzysztof