On Fri, 17 Jun 2022 at 21:58, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Hi Bjorn and Dmitry,
>
> Yu reported a lockdep warning coming from the gdsc driver. It looks like
> the runtime PM usage in gdsc.c is causing lockdep to see an AA deadlock
> possibility with 'genpd->mlock'. I suspect this is because we have
> commit 1b771839de05 ("clk: qcom: gdsc: enable optional power domain
> support"), and that is now calling runtime PM code from within the genpd
> code. I think genpd already has nested lock support, so the only
> solution is to not use runtime PM from within genpd code and start
> expressing genpd parent relationships in genpd itself?
Not sure exactly what you mean here, but yes, expressing the
parent/child domain relationship is always needed.
Having gdsc_disable() to do runtime PM calls (gdsc_pm_runtime_put())
seems awkward to me. Why is that needed, more exactly?
> Or maybe genpd
> needs to drop locks while calling down into gdsc_disable() and reacquire
> them after that?
Nope, that doesn't work. This internals of genpd relies on this
behaviour, as it allows it to properly deal with power-on|off for
parent/child domains, for example.
>
> ============================================
> 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
Kind regards
Uffe
