• Stephen Boyd's avatar
    clk: qcom: gdsc: Remove direct runtime PM calls · 4cc47e8a
    Stephen Boyd authored
    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[1] 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
    
    3 locks held by kworker/2:1/49:
     #0: 74ffff80811a5748 ((wq_completion)pm){+.+.}-{0:0}, at: process_one_work+0x320/0x5fc
     #1: ffffffc008537cf8 ((work_completion)(&genpd->power_off_work)){+.+.}-{0:0}, at: process_one_work+0x354/0x5fc
     #2: ffffffeea03710a8 (&genpd->mlock){+.+.}-{3:3}, at: genpd_lock_mtx+0x24/0x30
    
    stack backtrace:
    CPU: 2 PID: 49 Comm: kworker/2:1 Not tainted 5.19.0-rc2-lockdep+ #7
    Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
    Workqueue: pm genpd_power_off_work_fn
    Call trace:
     dump_backtrace+0x1a0/0x200
     show_stack+0x24/0x30
     dump_stack_lvl+0x7c/0xa0
     dump_stack+0x18/0x44
     __lock_acquire+0xb38/0x3634
     lock_acquire+0x180/0x2d4
     __mutex_lock_common+0x118/0xe30
     mutex_lock_nested+0x70/0x7c
     genpd_lock_mtx+0x24/0x30
     genpd_runtime_suspend+0x2f0/0x414
     __rpm_callback+0xdc/0x1b8
     rpm_callback+0x4c/0xcc
     rpm_suspend+0x21c/0x5f0
     rpm_idle+0x17c/0x1e0
     __pm_runtime_idle+0x78/0xcc
     gdsc_disable+0x24c/0x26c
     _genpd_power_off+0xd4/0x1c4
     genpd_power_off+0x2d8/0x41c
     genpd_power_off_work_fn+0x60/0x94
     process_one_work+0x398/0x5fc
     worker_thread+0x42c/0x6c4
     kthread+0x194/0x1b4
     ret_from_fork+0x10/0x20
    
    Second, this confuses runtime PM on CoachZ for the camera devices by
    causing the camera clock controller's runtime PM usage_count to go
    negative after resuming from suspend. This is because runtime PM is
    being used on the clock controller while runtime PM is disabled for the
    device.
    
    The reason for the negative count is because a GDSC is represented as a
    genpd and each genpd that is attached to a device is resumed during the
    noirq phase of system wide suspend/resume (see the noirq suspend ops
    assignment in pm_genpd_init() for more details). The camera GDSCs are
    attached to camera devices with the 'power-domains' property in DT.
    Every device has runtime PM disabled in the late system suspend phase
    via __device_suspend_late(). Runtime PM is not usable until runtime PM
    is enabled in device_resume_early(). The noirq phases run after the
    'late' and before the 'early' phase of suspend/resume. When the genpds
    are resumed in genpd_resume_noirq(), we call down into gdsc_enable()
    that calls pm_runtime_resume_and_get() and that returns -EACCES to
    indicate failure to resume because runtime PM is disabled for all
    devices.
    
    Upon closer inspection, calling runtime PM APIs like this in the GDSC
    driver doesn't make sense. It was intended to make sure the GDSC for the
    clock controller providing other GDSCs was enabled, specifically the
    MMCX GDSC for the display clk controller on SM8250 (sm8250-dispcc), so
    that GDSC register accesses succeeded. That will already happen because
    we make the 'dev->pm_domain' a parent domain of each GDSC we register in
    gdsc_register() via pm_genpd_add_subdomain(). When any of these GDSCs
    are accessed, we'll enable the parent domain (in this specific case
    MMCX).
    
    We also remove any getting of runtime PM during registration, because
    when a genpd is registered it increments the count on the parent if the
    genpd itself is already enabled.
    
    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>
    Reviewed-by: default avatarDouglas Anderson <dianders@chromium.org>
    Tested-by: default avatarDouglas Anderson <dianders@chromium.org>
    Cc: Matthias Kaehlcke <mka@chromium.org>
    Reported-by: default avatarStephen Boyd <swboyd@chromium.org>
    Link: https://lore.kernel.org/r/CAE-0n52xbZeJ66RaKwggeRB57fUAwjvxGxfFMKOKJMKVyFTe+w@mail.gmail.com [1]
    Fixes: 1b771839 ("clk: qcom: gdsc: enable optional power domain support")
    Signed-off-by: default avatarStephen Boyd <swboyd@chromium.org>
    Link: https://lore.kernel.org/r/20221103183030.3594899-1-swboyd@chromium.orgTested-by: default avatarJohan Hovold <johan+linaro@kernel.org>
    Reviewed-by: default avatarJohan Hovold <johan+linaro@kernel.org>
    Signed-off-by: default avatarStephen Boyd <sboyd@kernel.org>
    4cc47e8a
gdsc.h 2.77 KB