On Tue, Nov 01, 2022 at 04:34:21PM -0700, Stephen Boyd wrote:
> We shouldn't be calling runtime PM APIs from within the genpd
> enable/disable path for a couple reasons.
>
> First, this causes an AA lockdep splat because genpd can call into genpd
> code again while holding the genpd lock.
>
> WARNING: possible recursive locking detected
> 5.19.0-rc2-lockdep+ #7 Not tainted
> --------------------------------------------
> kworker/2:1/49 is trying to acquire lock:
> ffffffeea0370788 (&genpd->mlock){+.+.}-{3:3}, at: genpd_lock_mtx+0x24/0x30
>
> but task is already holding lock:
> ffffffeea03710a8 (&genpd->mlock){+.+.}-{3:3}, at: genpd_lock_mtx+0x24/0x30
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&genpd->mlock);
> lock(&genpd->mlock);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
I've seen this splat on sc8280xp as well but haven't had time to look
into it yet.
> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Cc: Johan Hovold <johan+linaro@xxxxxxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Cc: Taniya Das <quic_tdas@xxxxxxxxxxx>
> Cc: Satya Priya <quic_c_skakit@xxxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> Reported-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
We typically don't add Reported-by tags for bugs we find and fix
ourselves.
> Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support")
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
> drivers/clk/qcom/gdsc.c | 64 ++++++-----------------------------------
> 1 file changed, 8 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 7cf5e130e92f..a775ce1b7d8a 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -495,14 +451,11 @@ static int gdsc_init(struct gdsc *sc)
> sc->pd.power_on = gdsc_enable;
>
> ret = pm_genpd_init(&sc->pd, NULL, !on);
> - if (ret)
> - goto err_put_rpm;
> + if (!ret)
> + goto err_disable_supply;
The logic should not be inverted here (and only happens to work
currently when you have no regulator or the gdsc was off).
> return 0;
>
> -err_put_rpm:
> - if (on)
> - gdsc_pm_runtime_put(sc);
> err_disable_supply:
> if (on && sc->rsupply)
> regulator_disable(sc->rsupply);
Johan
