LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] regulator: recursive locking detected
@ 2011-04-01 10:04 Robert Rosengren
  2011-04-02  1:12 ` Mark Brown
  0 siblings, 1 reply; 4+ messages in thread
From: Robert Rosengren @ 2011-04-01 10:04 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, linux-kernel
  Cc: Bengt Jonsson, Linus Walleij, Robert Rosengren

"possible recursive locking detected"-warnings are issued when a
regulator has specified supply regulator. Both when enabling and
disabling regulators uses recursive call chains for notify the supply
regulators. This is due to locking mutexes of the same lock class,
i.e. the locks reside in the regulator_dev struct.

Since this is valid behavior for the regulators, this patch changes the
mutex lock into nested, as suggested in lockdep-design.txt.

Signed-off-by: Robert Rosengren <robert.rosengren@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/core.c |   35 ++++++++++++++++++++++-------------
 1 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3ffc697..e9536ff 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -87,7 +87,7 @@ static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
 static void _notifier_call_chain(struct regulator_dev *rdev,
-				  unsigned long event, void *data);
+				  unsigned long event, void *data, int lock_sublevel);
 static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 				     int min_uV, int max_uV);
 
@@ -1279,16 +1279,21 @@ static int _regulator_can_change_status(struct regulator_dev *rdev)
 		return 0;
 }
 
-/* locks held by regulator_enable() */
-static int _regulator_enable(struct regulator_dev *rdev)
+/* locks held by regulator_enable()
+ * lock_sublevel should always be 0, only used for recursive calls.
+ */
+static int _regulator_enable(struct regulator_dev *rdev,
+				int lock_sublevel)
 {
 	int ret, delay;
 
 	if (rdev->use_count == 0) {
 		/* do we need to enable the supply regulator first */
 		if (rdev->supply) {
-			mutex_lock(&rdev->supply->mutex);
-			ret = _regulator_enable(rdev->supply);
+			/* increase sublevel before stepping into nested regulators */
+			lock_sublevel++;
+			mutex_lock_nested(&rdev->supply->mutex, lock_sublevel);
+			ret = _regulator_enable(rdev->supply, lock_sublevel);
 			mutex_unlock(&rdev->supply->mutex);
 			if (ret < 0) {
 				rdev_err(rdev, "failed to enable: %d\n", ret);
@@ -1372,7 +1377,7 @@ int regulator_enable(struct regulator *regulator)
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	ret = _regulator_enable(rdev);
+	ret = _regulator_enable(rdev, 0);
 	mutex_unlock(&rdev->mutex);
 	return ret;
 }
@@ -1407,7 +1412,7 @@ static int _regulator_disable(struct regulator_dev *rdev,
 			trace_regulator_disable_complete(rdev_get_name(rdev));
 
 			_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
-					     NULL);
+					     NULL, 0);
 		}
 
 		/* decrease our supplies ref count and disable if required */
@@ -1477,7 +1482,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev,
 		}
 		/* notify other consumers that power has been forced off */
 		_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
-			REGULATOR_EVENT_DISABLE, NULL);
+			REGULATOR_EVENT_DISABLE, NULL, 0);
 	}
 
 	/* decrease our supplies ref count and disable if required */
@@ -1699,7 +1704,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 
 	if (ret == 0)
 		_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE,
-				     NULL);
+				     NULL, 0);
 
 	trace_regulator_set_voltage_complete(rdev_get_name(rdev), selector);
 
@@ -2167,19 +2172,23 @@ EXPORT_SYMBOL_GPL(regulator_unregister_notifier);
 
 /* notify regulator consumers and downstream regulator consumers.
  * Note mutex must be held by caller.
+ * lock_sublevel should always be 0, only used for recursive calls.
  */
 static void _notifier_call_chain(struct regulator_dev *rdev,
-				  unsigned long event, void *data)
+				  unsigned long event, void *data, int lock_sublevel)
 {
 	struct regulator_dev *_rdev;
 
 	/* call rdev chain first */
 	blocking_notifier_call_chain(&rdev->notifier, event, NULL);
 
+	/* increase sublevel before stepping into nested regulators */
+	lock_sublevel++;
+
 	/* now notify regulator we supply */
 	list_for_each_entry(_rdev, &rdev->supply_list, slist) {
-		mutex_lock(&_rdev->mutex);
-		_notifier_call_chain(_rdev, event, data);
+		mutex_lock_nested(&_rdev->mutex, lock_sublevel);
+		_notifier_call_chain(_rdev, event, data, lock_sublevel);
 		mutex_unlock(&_rdev->mutex);
 	}
 }
@@ -2333,7 +2342,7 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
-	_notifier_call_chain(rdev, event, data);
+	_notifier_call_chain(rdev, event, data, 0);
 	return NOTIFY_DONE;
 
 }
-- 
1.7.4.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] regulator: recursive locking detected
  2011-04-01 10:04 [PATCH] regulator: recursive locking detected Robert Rosengren
@ 2011-04-02  1:12 ` Mark Brown
  2011-04-07  6:31   ` Robert ROSENGREN
  0 siblings, 1 reply; 4+ messages in thread
From: Mark Brown @ 2011-04-02  1:12 UTC (permalink / raw)
  To: Robert Rosengren
  Cc: Liam Girdwood, linux-kernel, Bengt Jonsson, Linus Walleij

On Fri, Apr 01, 2011 at 12:04:17PM +0200, Robert Rosengren wrote:
> "possible recursive locking detected"-warnings are issued when a
> regulator has specified supply regulator. Both when enabling and
> disabling regulators uses recursive call chains for notify the supply
> regulators. This is due to locking mutexes of the same lock class,
> i.e. the locks reside in the regulator_dev struct.

There's actually a race here reported by David Brown in the past week
when working with supplies so the lock warning is probably a real issue
which should be fixed rather than overriding the warning.  Search the
list for the past week or so for the details.

> +/* locks held by regulator_enable()
> + * lock_sublevel should always be 0, only used for recursive calls.
> + */
> +static int _regulator_enable(struct regulator_dev *rdev,
> +				int lock_sublevel)

This comment is inaccurate (and if it were then obviously sublevel
wouldn't be needed).

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] regulator: recursive locking detected
  2011-04-02  1:12 ` Mark Brown
@ 2011-04-07  6:31   ` Robert ROSENGREN
  0 siblings, 0 replies; 4+ messages in thread
From: Robert ROSENGREN @ 2011-04-07  6:31 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, linux-kernel, Bengt JONSSON, Linus Walleij

On Sat, 2011-04-02 at 10:12 +0900, Mark Brown wrote:
> On Fri, Apr 01, 2011 at 12:04:17PM +0200, Robert Rosengren wrote:
> > "possible recursive locking detected"-warnings are issued when a
> > regulator has specified supply regulator. Both when enabling and
> > disabling regulators uses recursive call chains for notify the supply
> > regulators. This is due to locking mutexes of the same lock class,
> > i.e. the locks reside in the regulator_dev struct.
>
> There's actually a race here reported by David Brown in the past week
> when working with supplies so the lock warning is probably a real issue
> which should be fixed rather than overriding the warning.  Search the
> list for the past week or so for the details.

Thanks for letting me know, sorry I missed that discussion. Having
checked that out, few comments regarding the mutex_lock_nested
discussion.

If going for silencing the mutex warning in _notifier_call_chain, it
will not be sufficient to use SINGLE_DEPTH_NESTING as there is no limit
of just using one supply regulator, the next one may also have a supply
so for each one we need to increase the subclass in mutex_lock_nested.
Don't know if we have real use cases here, but at least the code admits
it. 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] regulator: recursive locking detected
@ 2011-04-01  9:33 Robert Rosengren
  0 siblings, 0 replies; 4+ messages in thread
From: Robert Rosengren @ 2011-04-01  9:33 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, linux-kernel
  Cc: Bengt Jonsson, Linus Walleij, Robert Rosengren

"possible recursive locking detected"-warnings are issued when a
regulator has specified supply regulator. Both when enabling and
disabling regulators uses recursive call chains for notify the supply
regulators. This is due to locking mutexes of the same lock class,
i.e. the locks reside in the regulator_dev struct.

Since this is valid behavior for the regulators, this patch changes the
mutex lock into nested, as suggested in lockdep-design.txt.

Signed-off-by: Robert Rosengren <robert.rosengren@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/core.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3ffc697..2a66c6b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -87,7 +87,7 @@ static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
 static void _notifier_call_chain(struct regulator_dev *rdev,
-				  unsigned long event, void *data);
+				  unsigned long event, void *data, int lock_sublevel);
 static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 				     int min_uV, int max_uV);
 
@@ -1407,7 +1407,7 @@ static int _regulator_disable(struct regulator_dev *rdev,
 			trace_regulator_disable_complete(rdev_get_name(rdev));
 
 			_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
-					     NULL);
+					     NULL, 0);
 		}
 
 		/* decrease our supplies ref count and disable if required */
@@ -1477,7 +1477,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev,
 		}
 		/* notify other consumers that power has been forced off */
 		_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
-			REGULATOR_EVENT_DISABLE, NULL);
+			REGULATOR_EVENT_DISABLE, NULL, 0);
 	}
 
 	/* decrease our supplies ref count and disable if required */
@@ -1699,7 +1699,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 
 	if (ret == 0)
 		_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE,
-				     NULL);
+				     NULL, 0);
 
 	trace_regulator_set_voltage_complete(rdev_get_name(rdev), selector);
 
@@ -2167,19 +2167,23 @@ EXPORT_SYMBOL_GPL(regulator_unregister_notifier);
 
 /* notify regulator consumers and downstream regulator consumers.
  * Note mutex must be held by caller.
+ * lock_sublevel should always be 0, only used for recursive calls.
  */
 static void _notifier_call_chain(struct regulator_dev *rdev,
-				  unsigned long event, void *data)
+				  unsigned long event, void *data, int lock_sublevel)
 {
 	struct regulator_dev *_rdev;
 
 	/* call rdev chain first */
 	blocking_notifier_call_chain(&rdev->notifier, event, NULL);
 
+	/* increase sublevel before stepping into nested regulators */
+	lock_sublevel++;
+
 	/* now notify regulator we supply */
 	list_for_each_entry(_rdev, &rdev->supply_list, slist) {
-		mutex_lock(&_rdev->mutex);
-		_notifier_call_chain(_rdev, event, data);
+		mutex_lock_nested(&_rdev->mutex, lock_sublevel);
+		_notifier_call_chain(_rdev, event, data, lock_sublevel);
 		mutex_unlock(&_rdev->mutex);
 	}
 }
@@ -2333,7 +2337,7 @@ EXPORT_SYMBOL_GPL(regulator_bulk_free);
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
 				  unsigned long event, void *data)
 {
-	_notifier_call_chain(rdev, event, data);
+	_notifier_call_chain(rdev, event, data, 0);
 	return NOTIFY_DONE;
 
 }
-- 
1.7.4.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-04-07  6:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-01 10:04 [PATCH] regulator: recursive locking detected Robert Rosengren
2011-04-02  1:12 ` Mark Brown
2011-04-07  6:31   ` Robert ROSENGREN
  -- strict thread matches above, loose matches on Subject: below --
2011-04-01  9:33 Robert Rosengren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).