From d6b9862907037f109bfc45bf5004f40bfca2f8d3 Mon Sep 17 00:00:00 2001 From: Jagadeesh Kona Date: Fri, 26 Feb 2021 21:14:14 +0530 Subject: [PATCH] clk: qcom: gdsc-regulator: Remove regulator_lock() call from GDSC driver Remove regulator_lock() and regulator_is_enabled() calls in GDSC driver as they are resulting in deadlock cases when lockdep is enabled. But it is critical to check if parent is enabled or not before performing any GDSC operation, as GDSC register access requires parent to be enabled. Use the parent's use_count to check if parent is enabled or not. For gdsc_disable(), in most cases, parent's mutex is locked by regulator framework before calling the gdsc_disable() callback, hence it is safe to read use_count. But in code paths where parent lock is not acquired by regulator framework, acquire parent's mutex before reading use_count to avoid parent getting disabled while in the middle of GDSC operations. For gdsc_set_mode(), parent locking is needed to avoid parent getting disabled in the middle of set mode operation. But it does not need the per-process reentrancy provided by regulator_lock() and regulator_unlock(). Therefore replace the calls with ww_mutex_lock() and ww_mutex_unlock(). Change-Id: Ic20f9a7478b47c9fde2e686b68d636342a4b1d48 Signed-off-by: Jagadeesh Kona --- drivers/clk/qcom/gdsc-regulator.c | 48 +++++++++++++++++-------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/drivers/clk/qcom/gdsc-regulator.c b/drivers/clk/qcom/gdsc-regulator.c index abbe34609675..158849549a18 100644 --- a/drivers/clk/qcom/gdsc-regulator.c +++ b/drivers/clk/qcom/gdsc-regulator.c @@ -344,20 +344,27 @@ static int gdsc_enable(struct regulator_dev *rdev) static int gdsc_disable(struct regulator_dev *rdev) { struct gdsc *sc = rdev_get_drvdata(rdev); + struct regulator_dev *parent_rdev; uint32_t regval; - int i, ret = 0, parent_enabled; + int i, ret = 0; + bool lock = false; if (rdev->supply) { - regulator_lock(rdev->supply->rdev); - parent_enabled = regulator_is_enabled(rdev->supply); - if (parent_enabled < 0) { - ret = parent_enabled; - dev_err(&rdev->dev, "%s unable to check parent enable state, ret=%d\n", - sc->rdesc.name, ret); - goto done; - } + parent_rdev = rdev->supply->rdev; - if (!parent_enabled) { + /* + * At this point, it can be assumed that parent supply's mutex is always + * locked by regulator framework before calling this callback but there + * are code paths where it isn't locked (e.g regulator_late_cleanup()). + * + * If parent supply is not locked, lock the parent supply mutex before + * checking it's enable count, so it won't get disabled while in the + * middle of GDSC operations + */ + if (ww_mutex_trylock(&parent_rdev->mutex)) + lock = true; + + if (!parent_rdev->use_count) { dev_err(&rdev->dev, "%s cannot disable GDSC while parent is disabled\n", sc->rdesc.name); ret = -EIO; @@ -435,8 +442,8 @@ static int gdsc_disable(struct regulator_dev *rdev) sc->is_gdsc_enabled = false; done: - if (rdev->supply) - regulator_unlock(rdev->supply->rdev); + if (rdev->supply && lock) + ww_mutex_unlock(&parent_rdev->mutex); return ret; } @@ -466,10 +473,12 @@ static unsigned int gdsc_get_mode(struct regulator_dev *rdev) static int gdsc_set_mode(struct regulator_dev *rdev, unsigned int mode) { struct gdsc *sc = rdev_get_drvdata(rdev); + struct regulator_dev *parent_rdev; uint32_t regval; int ret = 0; if (rdev->supply) { + parent_rdev = rdev->supply->rdev; /* * Ensure that the GDSC parent supply is enabled before * continuing. This is needed to avoid an unclocked access @@ -479,15 +488,12 @@ static int gdsc_set_mode(struct regulator_dev *rdev, unsigned int mode) * state cannot change after checking due to a race with another * consumer. */ - regulator_lock(rdev->supply->rdev); - ret = regulator_is_enabled(rdev->supply); - if (ret < 0) { - dev_err(&rdev->dev, "%s unable to check parent enable state, ret=%d\n", - sc->rdesc.name, ret); - goto done; - } else if (WARN(!ret, + ww_mutex_lock(&parent_rdev->mutex, NULL); + + if (!parent_rdev->use_count) { + dev_err(&rdev->dev, "%s cannot change GDSC HW/SW control mode while parent is disabled\n", - sc->rdesc.name)) { + sc->rdesc.name); ret = -EIO; goto done; } @@ -554,7 +560,7 @@ static int gdsc_set_mode(struct regulator_dev *rdev, unsigned int mode) done: if (rdev->supply) - regulator_unlock(rdev->supply->rdev); + ww_mutex_unlock(&parent_rdev->mutex); return ret; }