Deadlock scenario in regulator core

The locks for S1 and L2 are taken in opposite orders in the two threads; therefore, it is possible to achieve deadlock. I am not sure about the best way to resolve this situation. Is there a correctness requirement that regulator_enable holds the child regulator's lock when it attempts to enable the parent regulator? Likewise, is the lock around _notifier_call_chain required?

No need to hold the child lock, when we take the reference on the supply we own the reference. It's just that the systems which need to use daisychained regulators (mostly a DCDC to power LDOs for better efficiency) are moderately rare and tend to not bother representing the supply relationship as the parent regulator tends to be always on.

In fact it looks rather like the refcounting for supplies is wrong anyway, regulator_disable() unconditionally drops references to supplies but regulator_enable() only enables them if the refcount was previously zero, and it appears we don't clean up supplies after failed enables. The below patch (which I've not even compile tested) should resolve both issues, could you give it a spin and let me know if it works for you please?

drivers/regulator/core.c +0 -13

index 3ffc697..0a7fbde 100644
@@ -1284,19 +1284,6 @@ static int _regulator_enable(struct regulator_dev *rdev)
 {
 	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);
-			mutex_unlock(&rdev->supply->mutex);
-			if (ret < 0) {
-				rdev_err(rdev, "failed to enable: %d\n", ret);
-				return ret;
-			}
-		}
-	}
-
 	/* check voltage and requested load before enabling */
 	if (rdev->constraints &&
 	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))

drivers/regulator/core.c int regulator_enable(struct regulator *regulator) · line 1363

Quoted 4 lines · Mark Brown

drivers/regulator/core.c +4 -0

+	int disret;
+
+	if (rdev->supply) {
+		ret = regulator_enable(rdev->supply);

This should be _regulator_enable instead of regulator_enable. There will

also need to be a mutex lock and unlock around it for rdev->supply->mutex.

I think that it needs to iterate through all supplies in the chain
similar to how it is done in regulator_disable.

This should be _regulator_disable instead of regulator_disable. There will also need to be a mutex lock and unlock around it for rdev->supply->mutex. Additionally, a while loop is needed to disable all supplies in the chain (same as in regulator_disable).

Assume that A has already called regulator_enable for S1 some time in the past.

Consumer A thread execution:

	regulator_disable(S1)
	mutex_lock(S1)
	_regulator_disable(S1)
	_notifier_call_chain(S1)
	mutex_lock(L2)

Consumer B thread execution:

	regulator_enable(L2)
	mutex_lock(L2)
	_regulator_enable(L2)
	mutex_lock(S1)

The locks for S1 and L2 are taken in opposite orders in the two threads; therefore, it is possible to achieve deadlock. I am not sure about the best way to resolve this situation. Is there a correctness requirement that regulator_enable holds the child regulator's lock when it attempts to enable the parent regulator? Likewise, is the lock around _notifier_call_chain required?

Quoted 16 lines · David Collins

Assume that A has already called regulator_enable for S1 some time in the past.

Consumer A thread execution:

	regulator_disable(S1)
	mutex_lock(S1)
	_regulator_disable(S1)
	_notifier_call_chain(S1)
	mutex_lock(L2)

Consumer B thread execution:

	regulator_enable(L2)
	mutex_lock(L2)
	_regulator_enable(L2)
	mutex_lock(S1)

Quoted 6 lines

The locks for S1 and L2 are taken in opposite orders in the two threads; therefore, it is possible to achieve deadlock. I am not sure about the best way to resolve this situation. Is there a correctness requirement that regulator_enable holds the child regulator's lock when it attempts to enable the parent regulator? Likewise, is the lock around _notifier_call_chain required?

I'm curious, if you had enabled lockdep, do you get a warning? If not, why not?

Thanks,

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 27 lines · Steven Rostedt

Quoted 16 lines · David Collins

Assume that A has already called regulator_enable for S1 some time in the past.

Consumer A thread execution:

	regulator_disable(S1)
	mutex_lock(S1)
	_regulator_disable(S1)
	_notifier_call_chain(S1)
	mutex_lock(L2)

Consumer B thread execution:

	regulator_enable(L2)
	mutex_lock(L2)
	_regulator_enable(L2)
	mutex_lock(S1)

Quoted 6 lines

The locks for S1 and L2 are taken in opposite orders in the two threads; therefore, it is possible to achieve deadlock. I am not sure about the best way to resolve this situation. Is there a correctness requirement that regulator_enable holds the child regulator's lock when it attempts to enable the parent regulator? Likewise, is the lock around _notifier_call_chain required?

I'm curious, if you had enabled lockdep, do you get a warning? If not, why not?

Quoted 8 lines

Thanks,

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I have tried running with lockdep enabled. It does not produce a warning about possible deadlock from locks being taken in opposite orders in two threads. I assume that this is because it can only keep track of locks taken in the current stack backtrace.

It does produce a warning for regulator_disable by itself though on a regulator with a non-empty supply_list:

=============================================
[ INFO: possible recursive locking detected ]
2.6.38-rc7+ #231
---------------------------------------------
sh/25 is trying to acquire lock:
(&rdev->mutex){+.+...}, at: [<c0137ae4>] _notifier_call_chain+0x28/0x6c

but task is already holding lock:
(&rdev->mutex){+.+...}, at: [<c0138410>] regulator_disable+0x24/0x74

The locks that it is noting are different; one is for the parent regulator and the other is for the child regulator. Any thoughts?

Looks to me that the mutex_lock() in _notifier_call_chain needs to be a mutex_lock_nested().

The "_nested()" versions are when you have the same type of mutex taken but belonging to two different instances. Like you have here:

	blocking_notifier_call_chain(&rdev->notifier, event, NULL);

	/* 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_unlock(&_rdev->mutex);
	}

The rdev->mutex is already held, so we don't need to take it to call the blocking_notifier_call_chain() with the rdev->notifier. But then the list of rdev's in the rdev->supply_list are different instances but we are still taking the same type of lock. lockdep treats all instances of the same lock the same, so to lockdep this looks like a deadlock. To teach lockdep that this is a different instance, simply use mutex_lock_nested() instead.

-- Steve

There seem to be very few uses of mutex_lock_nested() in the kernel. Most of them use subclass = SINGLE_DEPTH_NESTING. Would this be sufficient for usage in the regulator core in _notifier_call_chain (and perhaps other places) or should some other subclass be used?

Note, I do not know this code well enough to say. I'm assuming that an rdevA on a rdevB->supply_list never has rdevB on its own rdevA->supply_list.

If this is the case, and that you only ever have a lock nesting of one, then sure, use the SINGLE_DEPTH_NESTING.

Quoted 4 lines · David Collins

There seem to be very few uses of mutex_lock_nested() in the kernel. Most of them use subclass = SINGLE_DEPTH_NESTING. Would this be sufficient for usage in the regulator core in _notifier_call_chain (and perhaps other places) or should some other subclass be used?

Note, I do not know this code well enough to say. I'm assuming that an rdevA on a rdevB->supply_list never has rdevB on its own rdevA->supply_list.

If this is the case, and that you only ever have a lock nesting of one, then sure, use the SINGLE_DEPTH_NESTING.

Peter or Ingo could correct me if I'm wrong.

Looks to me that the mutex_lock() in _notifier_call_chain needs to be a mutex_lock_nested().

The "_nested()" versions are when you have the same type of mutex taken but belonging to two different instances. Like you have here:

Quoted 2 lines · Steven Rostedt

Looks to me that the mutex_lock() in _notifier_call_chain needs to be a mutex_lock_nested().

Quoted 2 lines

The "_nested()" versions are when you have the same type of mutex taken but belonging to two different instances. Like you have here:

What's a mutex type? I have to say this is the first time I've heard of mutex types and the documentation in mutex.c and mutex-design.txt isn't precisely verbose on what mutex_lock_nested() is for or how one would pick subclass.

Quoted 4 lines · Mark Brown

What's a mutex type? I have to say this is the first time I've heard of mutex types and the documentation in mutex.c and mutex-design.txt isn't precisely verbose on what mutex_lock_nested() is for or how one would pick subclass.

Sorry, I said "mutex type" as a synonym to "lock class". A lock class is pretty much how a lock is defined.

There's better documentation about this in the lockdep-design.txt.

That's helpful, though it doesn't really say anything about how one picks subclass?

enable the parent regulator? Likewise, is the lock around _notifier_call_chain required?