[PATCH v2 1/2] regulator: Remove possible deadlock from regulator_enable

[David Collins]

Assume a regulator supply chain in which S1 supplies L2 (S1 --> L2).
It is possible to achieve deadlock using two threads if one thread
calls regulator_enable on a regulator L2 while the other thread calls
regulator_disable on S1.  This happens because _regulator_enable(L2)
holds a lock for L2 and then takes out a lock on S1.
_regulator_disable(S1) on the other hand, is called with a lock taken
for S1 and then it calls _notifier_call_chain which attempts to take a
lock for L2.  Because these locks are taken in opposite orders, deadlock
will result.

Change regulator_enable and _regulator_enable so that only one lock is
held at a time to avoid this deadlock scenario.  Rename
_regulator_enable to regulator_dev_enable to avoid confusion with other
_* functions which assume that all locks are already taken.

Signed-off-by: David Collins <collinsd@xxxxxxxxxxxxxx>
---
 drivers/regulator/core.c |   82 +++++++++++++++++++++++++++++++---------------
 1 files changed, 55 insertions(+), 27 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9fa2095..05904be 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1279,38 +1279,54 @@ 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 are *not* held by regulator_enable(). */
+static int regulator_dev_enable(struct regulator_dev *rdev)
 {
-	int ret, delay;
+	struct regulator_dev *supply_rdev = NULL;
+	int ret = 0, delay;
 
+	mutex_lock(&rdev->mutex);
 	if (rdev->use_count == 0) {
+		/*
+		 * Incrementing here removes a race condition for simultaneous
+		 * regulator_enable calls with use_count == 0.
+		 */
+		rdev->use_count++;
+		mutex_unlock(&rdev->mutex);
+
 		/* 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);
+			ret = regulator_dev_enable(rdev->supply);
 			if (ret < 0) {
 				rdev_err(rdev, "failed to enable: %d\n", ret);
+				mutex_lock(&rdev->mutex);
+				rdev->use_count--;
+				mutex_unlock(&rdev->mutex);
 				return ret;
 			}
+			supply_rdev = rdev->supply;
 		}
-	}
 
-	/* check voltage and requested load before enabling */
-	if (rdev->constraints &&
-	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
-		drms_uA_update(rdev);
+		mutex_lock(&rdev->mutex);
+		rdev->use_count--;
+
+		/* check voltage and requested load before enabling */
+		if (rdev->constraints &&
+		    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+			drms_uA_update(rdev);
 
-	if (rdev->use_count == 0) {
-		/* The regulator may on if it's not switchable or left on */
+		/* The regulator may be on if it's not switchable or left on */
 		ret = _regulator_is_enabled(rdev);
 		if (ret == -EINVAL || ret == 0) {
-			if (!_regulator_can_change_status(rdev))
-				return -EPERM;
+			if (!_regulator_can_change_status(rdev)) {
+				ret = -EPERM;
+				goto out;
+			}
 
-			if (!rdev->desc->ops->enable)
-				return -EINVAL;
+			if (!rdev->desc->ops->enable) {
+				ret = -EINVAL;
+				goto out;
+			}
 
 			/* Query before enabling in case configuration
 			 * dependant.  */
@@ -1330,7 +1346,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
 			 * regulators can ramp together.  */
 			ret = rdev->desc->ops->enable(rdev);
 			if (ret < 0)
-				return ret;
+				goto out;
 
 			trace_regulator_enable_delay(rdev_get_name(rdev));
 
@@ -1345,14 +1361,32 @@ static int _regulator_enable(struct regulator_dev *rdev)
 
 		} else if (ret < 0) {
 			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
-			return ret;
+			goto out;
 		}
 		/* Fallthrough on positive return values - already enabled */
+	} else {
+		/* Check voltage and requested load. */
+		if (rdev->constraints &&
+		    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+			drms_uA_update(rdev);
 	}
 
 	rdev->use_count++;
+out:
+	mutex_unlock(&rdev->mutex);
 
-	return 0;
+	if (ret < 0) {
+		/* Enable has failed, disable supplies if necessary. */
+		while (supply_rdev != NULL) {
+			rdev = supply_rdev;
+
+			mutex_lock(&rdev->mutex);
+			_regulator_disable(rdev, &supply_rdev);
+			mutex_unlock(&rdev->mutex);
+		}
+	}
+
+	return ret;
 }
 
 /**
@@ -1368,13 +1402,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
  */
 int regulator_enable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
-	int ret = 0;
-
-	mutex_lock(&rdev->mutex);
-	ret = _regulator_enable(rdev);
-	mutex_unlock(&rdev->mutex);
-	return ret;
+	return regulator_dev_enable(regulator->rdev);
 }
 EXPORT_SYMBOL_GPL(regulator_enable);
 
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
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/