lkml.org 
[lkml]   [2022]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] clk: qcom: gdsc: Remove direct runtime PM calls
    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

    \
     
     \ /
      Last update: 2022-11-02 11:53    [W:2.555 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site