Re: [PATCH] clk: qcom: gdsc: Remove direct runtime PM calls

[Johan Hovold <johan@kernel.org>]

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@linaro.org>
> Cc: Johan Hovold <johan+linaro@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Satya Priya <quic_c_skakit@quicinc.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Reported-by: Stephen Boyd <swboyd@chromium.org>

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@chromium.org>
> ---
>  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