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 <[email protected]> --- 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-arm-msm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
