| Message ID | 20221024150947.10739-1-marex@denx.de (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] soc: imx: imx8m-blk-ctrl: Split clock prepare from clock enable in the domain | expand |
Hi Marek, Thank you for the patch. On Mon, Oct 24, 2022 at 05:09:47PM +0200, Marek Vasut wrote: > It is possible for clk_disable_unused() to trigger lockdep warning > regarding lock ordering in this driver. This happens in case of the > following conditions: > > A) clock core clk_disable_unused() triggers the following sequence in a > driver which also uses blkctrl domain: > - clk_prepare_lock() -> obtains clock core prepare_lock > - pm_runtime_get*() -> obtains &blk_ctrl_genpd_lock_class > > B) driver powers up a power domain and triggers the following sequence > in blkctrl: > - pm_runtime_get_sync() -> obtains &blk_ctrl_genpd_lock_class > - clk_bulk_prepare_enable() -> obtains clock core prepare_lock > > This can lead to a deadlock in case A and B runs on separate CPUs. > > To avoid the deadlock, split clk_*prepare() from clk_*enable() and > call the former before pm_runtime_get_sync(). This way, the blkctrl > driver always claims the prepare_lock before blk_ctrl_genpd_lock_class > and the deadlock is avoided. I'm afraid I'm still getting occasional warnings from lockdep :-( This one happened at boot time: [ 17.837243] [ 17.837254] ====================================================== [ 17.837256] WARNING: possible circular locking dependency detected [ 17.837260] 6.1.0-rc2-00199-gd504fb27855a-dirty #301 Not tainted [ 17.837265] ------------------------------------------------------ [ 17.837267] kworker/u8:6/152 is trying to acquire lock: [ 17.837273] ffff000000eab6f0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}, at: genpd_lock_mtx+0x20/0x30 [ 17.837300] [ 17.837300] but task is already holding lock: [ 17.837302] ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 [ 17.837321] [ 17.837321] which lock already depends on the new lock. [ 17.837321] [ 17.837323] [ 17.837323] the existing dependency chain (in reverse order) is: [ 17.837326] [ 17.837326] -> #1 (prepare_lock){+.+.}-{3:3}: [ 17.837337] lock_acquire+0x68/0x90 [ 17.837345] __mutex_lock+0x8c/0x420 [ 17.837354] mutex_lock_nested+0x34/0x40 [ 17.837360] clk_prepare_lock+0x54/0xb0 [ 17.837367] clk_unprepare+0x2c/0x50 [ 17.837373] clk_bulk_unprepare+0x44/0x60 [ 17.837380] imx8mp_blk_ctrl_power_off+0xb4/0x100 [ 17.837386] _genpd_power_off+0x9c/0x180 [ 17.837392] genpd_power_off.part.0.isra.0+0x130/0x2a0 [ 17.837399] genpd_power_off_work_fn+0x6c/0x90 [ 17.837405] process_one_work+0x294/0x6b0 [ 17.837412] worker_thread+0x7c/0x450 [ 17.837416] kthread+0x114/0x120 [ 17.837424] ret_from_fork+0x10/0x20 [ 17.837431] [ 17.837431] -> #0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}: [ 17.837442] __lock_acquire+0x1148/0x1910 [ 17.837448] lock_acquire.part.0+0xec/0x260 [ 17.837454] lock_acquire+0x68/0x90 [ 17.837459] __mutex_lock+0x8c/0x420 [ 17.837466] mutex_lock_nested+0x34/0x40 [ 17.837472] genpd_lock_mtx+0x20/0x30 [ 17.837478] genpd_runtime_resume+0xfc/0x2e0 [ 17.837484] __rpm_callback+0x50/0x1b0 [ 17.837492] rpm_callback+0x74/0x80 [ 17.837499] rpm_resume+0x434/0x6e0 [ 17.837505] __pm_runtime_resume+0x54/0xa0 [ 17.837512] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 [ 17.837518] clk_core_set_rate_nolock+0xbc/0x2b0 [ 17.837524] clk_set_rate+0x40/0xa0 [ 17.837529] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] [ 17.837544] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 [ 17.837551] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 [ 17.837558] commit_tail+0xac/0x1a0 [ 17.837563] drm_atomic_helper_commit+0x1a0/0x1c0 [ 17.837569] drm_atomic_commit+0x94/0x130 [ 17.837574] drm_client_modeset_commit_atomic+0x230/0x2a0 [ 17.837582] drm_client_modeset_commit_locked+0x68/0x1d0 [ 17.837588] drm_client_modeset_commit+0x38/0x60 [ 17.837594] drm_fb_helper_set_par+0xd4/0x130 [ 17.837600] fbcon_init+0x3d4/0x540 [ 17.837606] visual_init+0xb8/0x110 [ 17.837613] do_bind_con_driver.isra.0+0x1c8/0x3a0 [ 17.837620] do_take_over_console+0x148/0x200 [ 17.837627] do_fbcon_takeover+0x74/0xf0 [ 17.837632] fbcon_fb_registered+0x18c/0x200 [ 17.837637] register_framebuffer+0x1bc/0x2b0 [ 17.837645] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 [ 17.837650] drm_fbdev_client_hotplug+0xd8/0x1c0 [ 17.837656] drm_fbdev_generic_setup+0xbc/0x1b0 [ 17.837661] lcdif_probe+0x2b0/0x310 [imx_lcdif] [ 17.837672] platform_probe+0x70/0xf0 [ 17.837678] really_probe+0xc8/0x3e0 [ 17.837685] __driver_probe_device+0x84/0x190 [ 17.837692] driver_probe_device+0x44/0x120 [ 17.837699] __device_attach_driver+0xc4/0x160 [ 17.837707] bus_for_each_drv+0x80/0xe0 [ 17.837713] __device_attach+0xb0/0x1f0 [ 17.837720] device_initial_probe+0x1c/0x30 [ 17.837727] bus_probe_device+0xa4/0xb0 [ 17.837734] deferred_probe_work_func+0xac/0x100 [ 17.837740] process_one_work+0x294/0x6b0 [ 17.837746] worker_thread+0x7c/0x450 [ 17.837751] kthread+0x114/0x120 [ 17.837758] ret_from_fork+0x10/0x20 [ 17.837764] [ 17.837764] other info that might help us debug this: [ 17.837764] [ 17.837766] Possible unsafe locking scenario: [ 17.837766] [ 17.837768] CPU0 CPU1 [ 17.837770] ---- ---- [ 17.837772] lock(prepare_lock); [ 17.837779] lock(&blk_ctrl_genpd_lock_class); [ 17.837785] lock(prepare_lock); [ 17.837790] lock(&blk_ctrl_genpd_lock_class); [ 17.837796] [ 17.837796] *** DEADLOCK *** [ 17.837796] [ 17.837799] 11 locks held by kworker/u8:6/152: [ 17.837803] #0: ffff000000040938 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 [ 17.837821] #1: ffff80000b74bdd0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 [ 17.837838] #2: ffff0000003f58f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x40/0x1f0 [ 17.837856] #3: ffff800009c1ddc0 (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x4c/0x2b0 [ 17.837875] #4: ffff800009ae4e68 (console_lock){+.+.}-{0:0}, at: fbcon_fb_registered+0x1f8/0x200 [ 17.837892] #5: ffff000002f1ee78 (&helper->lock){+.+.}-{3:3}, at: drm_fb_helper_set_par+0x98/0x130 [ 17.837909] #6: ffff0000038611a8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x2c/0x70 [ 17.837928] #7: ffff000002f1ec98 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x34/0x1d0 [ 17.837947] #8: ffff80000b74b690 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x50/0x2a0 [ 17.837964] #9: ffff000003863138 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_lock+0x48/0x130 [ 17.837984] #10: ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 [ 17.838003] [ 17.838003] stack backtrace: [ 17.838007] CPU: 1 PID: 152 Comm: kworker/u8:6 Not tainted 6.1.0-rc2-00199-gd504fb27855a-dirty #301 [ 17.838013] Hardware name: Polyhex Debix Model A i.MX8MPlus board (DT) [ 17.838018] Workqueue: events_unbound deferred_probe_work_func [ 17.838029] Call trace: [ 17.838031] dump_backtrace+0xe4/0x140 [ 17.838042] show_stack+0x20/0x50 [ 17.838049] dump_stack_lvl+0x9c/0xd8 [ 17.838055] dump_stack+0x18/0x34 [ 17.838060] print_circular_bug+0x1f8/0x200 [ 17.838066] check_noncircular+0x160/0x180 [ 17.838072] __lock_acquire+0x1148/0x1910 [ 17.838078] lock_acquire.part.0+0xec/0x260 [ 17.838084] lock_acquire+0x68/0x90 [ 17.838089] __mutex_lock+0x8c/0x420 [ 17.838098] mutex_lock_nested+0x34/0x40 [ 17.838104] genpd_lock_mtx+0x20/0x30 [ 17.838110] genpd_runtime_resume+0xfc/0x2e0 [ 17.838117] __rpm_callback+0x50/0x1b0 [ 17.838125] rpm_callback+0x74/0x80 [ 17.838132] rpm_resume+0x434/0x6e0 [ 17.838138] __pm_runtime_resume+0x54/0xa0 [ 17.838145] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 [ 17.838150] clk_core_set_rate_nolock+0xbc/0x2b0 [ 17.838157] clk_set_rate+0x40/0xa0 [ 17.838162] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] [ 17.838175] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 [ 17.838182] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 [ 17.838188] commit_tail+0xac/0x1a0 [ 17.838194] drm_atomic_helper_commit+0x1a0/0x1c0 [ 17.838200] drm_atomic_commit+0x94/0x130 [ 17.838205] drm_client_modeset_commit_atomic+0x230/0x2a0 [ 17.838212] drm_client_modeset_commit_locked+0x68/0x1d0 [ 17.838218] drm_client_modeset_commit+0x38/0x60 [ 17.838224] drm_fb_helper_set_par+0xd4/0x130 [ 17.838230] fbcon_init+0x3d4/0x540 [ 17.838235] visual_init+0xb8/0x110 [ 17.838243] do_bind_con_driver.isra.0+0x1c8/0x3a0 [ 17.838250] do_take_over_console+0x148/0x200 [ 17.838257] do_fbcon_takeover+0x74/0xf0 [ 17.838262] fbcon_fb_registered+0x18c/0x200 [ 17.838268] register_framebuffer+0x1bc/0x2b0 [ 17.838275] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 [ 17.838282] drm_fbdev_client_hotplug+0xd8/0x1c0 [ 17.838288] drm_fbdev_generic_setup+0xbc/0x1b0 [ 17.838293] lcdif_probe+0x2b0/0x310 [imx_lcdif] [ 17.838304] platform_probe+0x70/0xf0 [ 17.838309] really_probe+0xc8/0x3e0 [ 17.838316] __driver_probe_device+0x84/0x190 [ 17.838323] driver_probe_device+0x44/0x120 [ 17.838330] __device_attach_driver+0xc4/0x160 [ 17.838338] bus_for_each_drv+0x80/0xe0 [ 17.838344] __device_attach+0xb0/0x1f0 [ 17.838351] device_initial_probe+0x1c/0x30 [ 17.838358] bus_probe_device+0xa4/0xb0 [ 17.838365] deferred_probe_work_func+0xac/0x100 [ 17.838372] process_one_work+0x294/0x6b0 [ 17.838378] worker_thread+0x7c/0x450 [ 17.838383] kthread+0x114/0x120 [ 17.838390] ret_from_fork+0x10/0x20 > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Fabio Estevam <festevam@denx.de> > Cc: Jacky Bai <ping.bai@nxp.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Shengjiu Wang <shengjiu.wang@nxp.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: linux-clk@vger.kernel.org > Cc: linux-imx@nxp.com > To: linux-arm-kernel@lists.infradead.org > --- > drivers/soc/imx/imx8mp-blk-ctrl.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c > index b7d4161fcda9c..571c98b5b005b 100644 > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > @@ -393,15 +393,21 @@ static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd) > struct imx8mp_blk_ctrl *bc = domain->bc; > int ret; > > + ret = clk_bulk_prepare(data->num_clks, domain->clks); > + if (ret) { > + dev_err(bc->dev, "failed to enable clocks\n"); > + return ret; > + } > + > /* make sure bus domain is awake */ > ret = pm_runtime_resume_and_get(bc->bus_power_dev); > if (ret < 0) { > dev_err(bc->dev, "failed to power up bus domain\n"); > - return ret; > + goto rpm_put; > } > > /* enable upstream clocks */ > - ret = clk_bulk_prepare_enable(data->num_clks, domain->clks); > + ret = clk_bulk_enable(data->num_clks, domain->clks); > if (ret) { > dev_err(bc->dev, "failed to enable clocks\n"); > goto bus_put; > @@ -426,9 +432,11 @@ static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd) > return 0; > > clk_disable: > - clk_bulk_disable_unprepare(data->num_clks, domain->clks); > + clk_bulk_disable(data->num_clks, domain->clks); > bus_put: > pm_runtime_put(bc->bus_power_dev); > +rpm_put: > + clk_bulk_unprepare(data->num_clks, domain->clks); > > return ret; > }
Hi Laurent, > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 2022年10月25日 8:26 > To: Marek Vasut <marex@denx.de> > Cc: linux-arm-kernel@lists.infradead.org; Fabio Estevam > <festevam@denx.de>; Jacky Bai <ping.bai@nxp.com>; Lucas Stach > <l.stach@pengutronix.de>; Peng Fan <peng.fan@nxp.com>; Shawn Guo > <shawnguo@kernel.org>; S.J. Wang <shengjiu.wang@nxp.com>; Stephen > Boyd <sboyd@kernel.org>; linux-clk@vger.kernel.org; dl-linux-imx <linux- > imx@nxp.com> > Subject: Re: [PATCH] [RFC] soc: imx: imx8m-blk-ctrl: Split clock prepare from > clock enable in the domain > > Hi Marek, > > Thank you for the patch. > > On Mon, Oct 24, 2022 at 05:09:47PM +0200, Marek Vasut wrote: > > It is possible for clk_disable_unused() to trigger lockdep warning > > regarding lock ordering in this driver. This happens in case of the > > following conditions: > > > > A) clock core clk_disable_unused() triggers the following sequence in a > > driver which also uses blkctrl domain: > > - clk_prepare_lock() -> obtains clock core prepare_lock > > - pm_runtime_get*() -> obtains &blk_ctrl_genpd_lock_class > > > > B) driver powers up a power domain and triggers the following sequence > > in blkctrl: > > - pm_runtime_get_sync() -> obtains &blk_ctrl_genpd_lock_class > > - clk_bulk_prepare_enable() -> obtains clock core prepare_lock > > > > This can lead to a deadlock in case A and B runs on separate CPUs. > > > > To avoid the deadlock, split clk_*prepare() from clk_*enable() and > > call the former before pm_runtime_get_sync(). This way, the blkctrl > > driver always claims the prepare_lock before blk_ctrl_genpd_lock_class > > and the deadlock is avoided. > > I'm afraid I'm still getting occasional warnings from lockdep :-( This one > happened at boot time: > > [ 17.837243] > [ 17.837254] > ====================================================== > [ 17.837256] WARNING: possible circular locking dependency detected > [ 17.837260] 6.1.0-rc2-00199-gd504fb27855a-dirty #301 Not tainted > [ 17.837265] ------------------------------------------------------ > [ 17.837267] kworker/u8:6/152 is trying to acquire lock: > [ 17.837273] ffff000000eab6f0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}, at: > genpd_lock_mtx+0x20/0x30 > [ 17.837300] > [ 17.837300] but task is already holding lock: > [ 17.837302] ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: > clk_prepare_lock+0x34/0xb0 > [ 17.837321] > [ 17.837321] which lock already depends on the new lock. > [ 17.837321] > [ 17.837323] > [ 17.837323] the existing dependency chain (in reverse order) is: > [ 17.837326] > [ 17.837326] -> #1 (prepare_lock){+.+.}-{3:3}: > [ 17.837337] lock_acquire+0x68/0x90 > [ 17.837345] __mutex_lock+0x8c/0x420 > [ 17.837354] mutex_lock_nested+0x34/0x40 > [ 17.837360] clk_prepare_lock+0x54/0xb0 > [ 17.837367] clk_unprepare+0x2c/0x50 > [ 17.837373] clk_bulk_unprepare+0x44/0x60 > [ 17.837380] imx8mp_blk_ctrl_power_off+0xb4/0x100 > [ 17.837386] _genpd_power_off+0x9c/0x180 > [ 17.837392] genpd_power_off.part.0.isra.0+0x130/0x2a0 > [ 17.837399] genpd_power_off_work_fn+0x6c/0x90 > [ 17.837405] process_one_work+0x294/0x6b0 > [ 17.837412] worker_thread+0x7c/0x450 > [ 17.837416] kthread+0x114/0x120 > [ 17.837424] ret_from_fork+0x10/0x20 > [ 17.837431] > [ 17.837431] -> #0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}: > [ 17.837442] __lock_acquire+0x1148/0x1910 > [ 17.837448] lock_acquire.part.0+0xec/0x260 > [ 17.837454] lock_acquire+0x68/0x90 > [ 17.837459] __mutex_lock+0x8c/0x420 > [ 17.837466] mutex_lock_nested+0x34/0x40 > [ 17.837472] genpd_lock_mtx+0x20/0x30 > [ 17.837478] genpd_runtime_resume+0xfc/0x2e0 > [ 17.837484] __rpm_callback+0x50/0x1b0 > [ 17.837492] rpm_callback+0x74/0x80 > [ 17.837499] rpm_resume+0x434/0x6e0 > [ 17.837505] __pm_runtime_resume+0x54/0xa0 > [ 17.837512] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > [ 17.837518] clk_core_set_rate_nolock+0xbc/0x2b0 > [ 17.837524] clk_set_rate+0x40/0xa0 Which clk is you using? This clk seems requires a power domain. As Marek replied, the blk ctrl driver may need similar change to split clk prepare and clk enable. Do you have a public tree that for test? Thanks, Peng. > [ 17.837529] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > [ 17.837544] > drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > [ 17.837551] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > [ 17.837558] commit_tail+0xac/0x1a0 > [ 17.837563] drm_atomic_helper_commit+0x1a0/0x1c0 > [ 17.837569] drm_atomic_commit+0x94/0x130 > [ 17.837574] drm_client_modeset_commit_atomic+0x230/0x2a0 > [ 17.837582] drm_client_modeset_commit_locked+0x68/0x1d0 > [ 17.837588] drm_client_modeset_commit+0x38/0x60 > [ 17.837594] drm_fb_helper_set_par+0xd4/0x130 > [ 17.837600] fbcon_init+0x3d4/0x540 > [ 17.837606] visual_init+0xb8/0x110 > [ 17.837613] do_bind_con_driver.isra.0+0x1c8/0x3a0 > [ 17.837620] do_take_over_console+0x148/0x200 > [ 17.837627] do_fbcon_takeover+0x74/0xf0 > [ 17.837632] fbcon_fb_registered+0x18c/0x200 > [ 17.837637] register_framebuffer+0x1bc/0x2b0 > [ 17.837645] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > [ 17.837650] drm_fbdev_client_hotplug+0xd8/0x1c0 > [ 17.837656] drm_fbdev_generic_setup+0xbc/0x1b0 > [ 17.837661] lcdif_probe+0x2b0/0x310 [imx_lcdif] > [ 17.837672] platform_probe+0x70/0xf0 > [ 17.837678] really_probe+0xc8/0x3e0 > [ 17.837685] __driver_probe_device+0x84/0x190 > [ 17.837692] driver_probe_device+0x44/0x120 > [ 17.837699] __device_attach_driver+0xc4/0x160 > [ 17.837707] bus_for_each_drv+0x80/0xe0 > [ 17.837713] __device_attach+0xb0/0x1f0 > [ 17.837720] device_initial_probe+0x1c/0x30 > [ 17.837727] bus_probe_device+0xa4/0xb0 > [ 17.837734] deferred_probe_work_func+0xac/0x100 > [ 17.837740] process_one_work+0x294/0x6b0 > [ 17.837746] worker_thread+0x7c/0x450 > [ 17.837751] kthread+0x114/0x120 > [ 17.837758] ret_from_fork+0x10/0x20 > [ 17.837764] > [ 17.837764] other info that might help us debug this: > [ 17.837764] > [ 17.837766] Possible unsafe locking scenario: > [ 17.837766] > [ 17.837768] CPU0 CPU1 > [ 17.837770] ---- ---- > [ 17.837772] lock(prepare_lock); > [ 17.837779] lock(&blk_ctrl_genpd_lock_class); > [ 17.837785] lock(prepare_lock); > [ 17.837790] lock(&blk_ctrl_genpd_lock_class); > [ 17.837796] > [ 17.837796] *** DEADLOCK *** > [ 17.837796] > [ 17.837799] 11 locks held by kworker/u8:6/152: > [ 17.837803] #0: ffff000000040938 > ((wq_completion)events_unbound){+.+.}-{0:0}, at: > process_one_work+0x1fc/0x6b0 > [ 17.837821] #1: ffff80000b74bdd0 (deferred_probe_work){+.+.}-{0:0}, at: > process_one_work+0x1fc/0x6b0 > [ 17.837838] #2: ffff0000003f58f8 (&dev->mutex){....}-{3:3}, at: > __device_attach+0x40/0x1f0 > [ 17.837856] #3: ffff800009c1ddc0 (registration_lock){+.+.}-{3:3}, at: > register_framebuffer+0x4c/0x2b0 > [ 17.837875] #4: ffff800009ae4e68 (console_lock){+.+.}-{0:0}, at: > fbcon_fb_registered+0x1f8/0x200 > [ 17.837892] #5: ffff000002f1ee78 (&helper->lock){+.+.}-{3:3}, at: > drm_fb_helper_set_par+0x98/0x130 > [ 17.837909] #6: ffff0000038611a8 (&dev->master_mutex){+.+.}-{3:3}, at: > drm_master_internal_acquire+0x2c/0x70 > [ 17.837928] #7: ffff000002f1ec98 (&client->modeset_mutex){+.+.}-{3:3}, at: > drm_client_modeset_commit_locked+0x34/0x1d0 > [ 17.837947] #8: ffff80000b74b690 (crtc_ww_class_acquire){+.+.}-{0:0}, at: > drm_client_modeset_commit_atomic+0x50/0x2a0 > [ 17.837964] #9: ffff000003863138 (crtc_ww_class_mutex){+.+.}-{3:3}, at: > drm_modeset_lock+0x48/0x130 > [ 17.837984] #10: ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: > clk_prepare_lock+0x34/0xb0 > [ 17.838003] > [ 17.838003] stack backtrace: > [ 17.838007] CPU: 1 PID: 152 Comm: kworker/u8:6 Not tainted 6.1.0-rc2- > 00199-gd504fb27855a-dirty #301 > [ 17.838013] Hardware name: Polyhex Debix Model A i.MX8MPlus board > (DT) > [ 17.838018] Workqueue: events_unbound deferred_probe_work_func > [ 17.838029] Call trace: > [ 17.838031] dump_backtrace+0xe4/0x140 > [ 17.838042] show_stack+0x20/0x50 > [ 17.838049] dump_stack_lvl+0x9c/0xd8 > [ 17.838055] dump_stack+0x18/0x34 > [ 17.838060] print_circular_bug+0x1f8/0x200 > [ 17.838066] check_noncircular+0x160/0x180 > [ 17.838072] __lock_acquire+0x1148/0x1910 > [ 17.838078] lock_acquire.part.0+0xec/0x260 > [ 17.838084] lock_acquire+0x68/0x90 > [ 17.838089] __mutex_lock+0x8c/0x420 > [ 17.838098] mutex_lock_nested+0x34/0x40 > [ 17.838104] genpd_lock_mtx+0x20/0x30 > [ 17.838110] genpd_runtime_resume+0xfc/0x2e0 > [ 17.838117] __rpm_callback+0x50/0x1b0 > [ 17.838125] rpm_callback+0x74/0x80 > [ 17.838132] rpm_resume+0x434/0x6e0 > [ 17.838138] __pm_runtime_resume+0x54/0xa0 > [ 17.838145] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > [ 17.838150] clk_core_set_rate_nolock+0xbc/0x2b0 > [ 17.838157] clk_set_rate+0x40/0xa0 > [ 17.838162] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > [ 17.838175] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > [ 17.838182] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > [ 17.838188] commit_tail+0xac/0x1a0 > [ 17.838194] drm_atomic_helper_commit+0x1a0/0x1c0 > [ 17.838200] drm_atomic_commit+0x94/0x130 > [ 17.838205] drm_client_modeset_commit_atomic+0x230/0x2a0 > [ 17.838212] drm_client_modeset_commit_locked+0x68/0x1d0 > [ 17.838218] drm_client_modeset_commit+0x38/0x60 > [ 17.838224] drm_fb_helper_set_par+0xd4/0x130 > [ 17.838230] fbcon_init+0x3d4/0x540 > [ 17.838235] visual_init+0xb8/0x110 > [ 17.838243] do_bind_con_driver.isra.0+0x1c8/0x3a0 > [ 17.838250] do_take_over_console+0x148/0x200 > [ 17.838257] do_fbcon_takeover+0x74/0xf0 > [ 17.838262] fbcon_fb_registered+0x18c/0x200 > [ 17.838268] register_framebuffer+0x1bc/0x2b0 > [ 17.838275] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > [ 17.838282] drm_fbdev_client_hotplug+0xd8/0x1c0 > [ 17.838288] drm_fbdev_generic_setup+0xbc/0x1b0 > [ 17.838293] lcdif_probe+0x2b0/0x310 [imx_lcdif] > [ 17.838304] platform_probe+0x70/0xf0 > [ 17.838309] really_probe+0xc8/0x3e0 > [ 17.838316] __driver_probe_device+0x84/0x190 > [ 17.838323] driver_probe_device+0x44/0x120 > [ 17.838330] __device_attach_driver+0xc4/0x160 > [ 17.838338] bus_for_each_drv+0x80/0xe0 > [ 17.838344] __device_attach+0xb0/0x1f0 > [ 17.838351] device_initial_probe+0x1c/0x30 > [ 17.838358] bus_probe_device+0xa4/0xb0 > [ 17.838365] deferred_probe_work_func+0xac/0x100 > [ 17.838372] process_one_work+0x294/0x6b0 > [ 17.838378] worker_thread+0x7c/0x450 > [ 17.838383] kthread+0x114/0x120 > [ 17.838390] ret_from_fork+0x10/0x20 > > > Signed-off-by: Marek Vasut <marex@denx.de> > > --- > > Cc: Fabio Estevam <festevam@denx.de> > > Cc: Jacky Bai <ping.bai@nxp.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Peng Fan <peng.fan@nxp.com> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: Shengjiu Wang <shengjiu.wang@nxp.com> > > Cc: Stephen Boyd <sboyd@kernel.org> > > Cc: linux-clk@vger.kernel.org > > Cc: linux-imx@nxp.com > > To: linux-arm-kernel@lists.infradead.org > > --- > > drivers/soc/imx/imx8mp-blk-ctrl.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c > > b/drivers/soc/imx/imx8mp-blk-ctrl.c > > index b7d4161fcda9c..571c98b5b005b 100644 > > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > > @@ -393,15 +393,21 @@ static int imx8mp_blk_ctrl_power_on(struct > generic_pm_domain *genpd) > > struct imx8mp_blk_ctrl *bc = domain->bc; > > int ret; > > > > + ret = clk_bulk_prepare(data->num_clks, domain->clks); > > + if (ret) { > > + dev_err(bc->dev, "failed to enable clocks\n"); > > + return ret; > > + } > > + > > /* make sure bus domain is awake */ > > ret = pm_runtime_resume_and_get(bc->bus_power_dev); > > if (ret < 0) { > > dev_err(bc->dev, "failed to power up bus domain\n"); > > - return ret; > > + goto rpm_put; > > } > > > > /* enable upstream clocks */ > > - ret = clk_bulk_prepare_enable(data->num_clks, domain->clks); > > + ret = clk_bulk_enable(data->num_clks, domain->clks); > > if (ret) { > > dev_err(bc->dev, "failed to enable clocks\n"); > > goto bus_put; > > @@ -426,9 +432,11 @@ static int imx8mp_blk_ctrl_power_on(struct > generic_pm_domain *genpd) > > return 0; > > > > clk_disable: > > - clk_bulk_disable_unprepare(data->num_clks, domain->clks); > > + clk_bulk_disable(data->num_clks, domain->clks); > > bus_put: > > pm_runtime_put(bc->bus_power_dev); > > +rpm_put: > > + clk_bulk_unprepare(data->num_clks, domain->clks); > > > > return ret; > > } > > -- > Regards, > > Laurent Pinchart
Hi Peng, On Tue, Oct 25, 2022 at 12:49:46AM +0000, Peng Fan wrote: > On 2022年10月25日 8:26, Laurent Pinchart wrote: > > On Mon, Oct 24, 2022 at 05:09:47PM +0200, Marek Vasut wrote: > > > It is possible for clk_disable_unused() to trigger lockdep warning > > > regarding lock ordering in this driver. This happens in case of the > > > following conditions: > > > > > > A) clock core clk_disable_unused() triggers the following sequence in a > > > driver which also uses blkctrl domain: > > > - clk_prepare_lock() -> obtains clock core prepare_lock > > > - pm_runtime_get*() -> obtains &blk_ctrl_genpd_lock_class > > > > > > B) driver powers up a power domain and triggers the following sequence > > > in blkctrl: > > > - pm_runtime_get_sync() -> obtains &blk_ctrl_genpd_lock_class > > > - clk_bulk_prepare_enable() -> obtains clock core prepare_lock > > > > > > This can lead to a deadlock in case A and B runs on separate CPUs. > > > > > > To avoid the deadlock, split clk_*prepare() from clk_*enable() and > > > call the former before pm_runtime_get_sync(). This way, the blkctrl > > > driver always claims the prepare_lock before blk_ctrl_genpd_lock_class > > > and the deadlock is avoided. > > > > I'm afraid I'm still getting occasional warnings from lockdep :-( This one > > happened at boot time: > > > > [ 17.837243] > > [ 17.837254] ====================================================== > > [ 17.837256] WARNING: possible circular locking dependency detected > > [ 17.837260] 6.1.0-rc2-00199-gd504fb27855a-dirty #301 Not tainted > > [ 17.837265] ------------------------------------------------------ > > [ 17.837267] kworker/u8:6/152 is trying to acquire lock: > > [ 17.837273] ffff000000eab6f0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}, at: genpd_lock_mtx+0x20/0x30 > > [ 17.837300] > > [ 17.837300] but task is already holding lock: > > [ 17.837302] ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 > > [ 17.837321] > > [ 17.837321] which lock already depends on the new lock. > > [ 17.837321] > > [ 17.837323] > > [ 17.837323] the existing dependency chain (in reverse order) is: > > [ 17.837326] > > [ 17.837326] -> #1 (prepare_lock){+.+.}-{3:3}: > > [ 17.837337] lock_acquire+0x68/0x90 > > [ 17.837345] __mutex_lock+0x8c/0x420 > > [ 17.837354] mutex_lock_nested+0x34/0x40 > > [ 17.837360] clk_prepare_lock+0x54/0xb0 > > [ 17.837367] clk_unprepare+0x2c/0x50 > > [ 17.837373] clk_bulk_unprepare+0x44/0x60 > > [ 17.837380] imx8mp_blk_ctrl_power_off+0xb4/0x100 > > [ 17.837386] _genpd_power_off+0x9c/0x180 > > [ 17.837392] genpd_power_off.part.0.isra.0+0x130/0x2a0 > > [ 17.837399] genpd_power_off_work_fn+0x6c/0x90 > > [ 17.837405] process_one_work+0x294/0x6b0 > > [ 17.837412] worker_thread+0x7c/0x450 > > [ 17.837416] kthread+0x114/0x120 > > [ 17.837424] ret_from_fork+0x10/0x20 > > [ 17.837431] > > [ 17.837431] -> #0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}: > > [ 17.837442] __lock_acquire+0x1148/0x1910 > > [ 17.837448] lock_acquire.part.0+0xec/0x260 > > [ 17.837454] lock_acquire+0x68/0x90 > > [ 17.837459] __mutex_lock+0x8c/0x420 > > [ 17.837466] mutex_lock_nested+0x34/0x40 > > [ 17.837472] genpd_lock_mtx+0x20/0x30 > > [ 17.837478] genpd_runtime_resume+0xfc/0x2e0 > > [ 17.837484] __rpm_callback+0x50/0x1b0 > > [ 17.837492] rpm_callback+0x74/0x80 > > [ 17.837499] rpm_resume+0x434/0x6e0 > > [ 17.837505] __pm_runtime_resume+0x54/0xa0 > > [ 17.837512] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > > [ 17.837518] clk_core_set_rate_nolock+0xbc/0x2b0 > > [ 17.837524] clk_set_rate+0x40/0xa0 > > Which clk is you using? This clk seems requires a power domain. If I'm not mistaken, the is the "pix" clock for the lcdif device, which is provided by the hdmi_tx_phy. See drivers/gpu/drm/mxsfb/lcdif_drv.c and arch/arm64/boot/dts/freescale/imx8mp.dtsi. > As Marek replied, the blk ctrl driver may need similar change to split > clk prepare and clk enable. > Do you have a public tree that for test? Sure, it is available at https://gitlab.com/ideasonboard/nxp/linux/tree/pinchartl/v6.1/dev/isp/next > > [ 17.837529] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > > [ 17.837544] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > > [ 17.837551] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > > [ 17.837558] commit_tail+0xac/0x1a0 > > [ 17.837563] drm_atomic_helper_commit+0x1a0/0x1c0 > > [ 17.837569] drm_atomic_commit+0x94/0x130 > > [ 17.837574] drm_client_modeset_commit_atomic+0x230/0x2a0 > > [ 17.837582] drm_client_modeset_commit_locked+0x68/0x1d0 > > [ 17.837588] drm_client_modeset_commit+0x38/0x60 > > [ 17.837594] drm_fb_helper_set_par+0xd4/0x130 > > [ 17.837600] fbcon_init+0x3d4/0x540 > > [ 17.837606] visual_init+0xb8/0x110 > > [ 17.837613] do_bind_con_driver.isra.0+0x1c8/0x3a0 > > [ 17.837620] do_take_over_console+0x148/0x200 > > [ 17.837627] do_fbcon_takeover+0x74/0xf0 > > [ 17.837632] fbcon_fb_registered+0x18c/0x200 > > [ 17.837637] register_framebuffer+0x1bc/0x2b0 > > [ 17.837645] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > > [ 17.837650] drm_fbdev_client_hotplug+0xd8/0x1c0 > > [ 17.837656] drm_fbdev_generic_setup+0xbc/0x1b0 > > [ 17.837661] lcdif_probe+0x2b0/0x310 [imx_lcdif] > > [ 17.837672] platform_probe+0x70/0xf0 > > [ 17.837678] really_probe+0xc8/0x3e0 > > [ 17.837685] __driver_probe_device+0x84/0x190 > > [ 17.837692] driver_probe_device+0x44/0x120 > > [ 17.837699] __device_attach_driver+0xc4/0x160 > > [ 17.837707] bus_for_each_drv+0x80/0xe0 > > [ 17.837713] __device_attach+0xb0/0x1f0 > > [ 17.837720] device_initial_probe+0x1c/0x30 > > [ 17.837727] bus_probe_device+0xa4/0xb0 > > [ 17.837734] deferred_probe_work_func+0xac/0x100 > > [ 17.837740] process_one_work+0x294/0x6b0 > > [ 17.837746] worker_thread+0x7c/0x450 > > [ 17.837751] kthread+0x114/0x120 > > [ 17.837758] ret_from_fork+0x10/0x20 > > [ 17.837764] > > [ 17.837764] other info that might help us debug this: > > [ 17.837764] > > [ 17.837766] Possible unsafe locking scenario: > > [ 17.837766] > > [ 17.837768] CPU0 CPU1 > > [ 17.837770] ---- ---- > > [ 17.837772] lock(prepare_lock); > > [ 17.837779] lock(&blk_ctrl_genpd_lock_class); > > [ 17.837785] lock(prepare_lock); > > [ 17.837790] lock(&blk_ctrl_genpd_lock_class); > > [ 17.837796] > > [ 17.837796] *** DEADLOCK *** > > [ 17.837796] > > [ 17.837799] 11 locks held by kworker/u8:6/152: > > [ 17.837803] #0: ffff000000040938 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 > > [ 17.837821] #1: ffff80000b74bdd0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 > > [ 17.837838] #2: ffff0000003f58f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x40/0x1f0 > > [ 17.837856] #3: ffff800009c1ddc0 (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x4c/0x2b0 > > [ 17.837875] #4: ffff800009ae4e68 (console_lock){+.+.}-{0:0}, at: fbcon_fb_registered+0x1f8/0x200 > > [ 17.837892] #5: ffff000002f1ee78 (&helper->lock){+.+.}-{3:3}, at: drm_fb_helper_set_par+0x98/0x130 > > [ 17.837909] #6: ffff0000038611a8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x2c/0x70 > > [ 17.837928] #7: ffff000002f1ec98 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x34/0x1d0 > > [ 17.837947] #8: ffff80000b74b690 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x50/0x2a0 > > [ 17.837964] #9: ffff000003863138 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_lock+0x48/0x130 > > [ 17.837984] #10: ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 > > [ 17.838003] > > [ 17.838003] stack backtrace: > > [ 17.838007] CPU: 1 PID: 152 Comm: kworker/u8:6 Not tainted 6.1.0-rc2-00199-gd504fb27855a-dirty #301 > > [ 17.838013] Hardware name: Polyhex Debix Model A i.MX8MPlus board (DT) > > [ 17.838018] Workqueue: events_unbound deferred_probe_work_func > > [ 17.838029] Call trace: > > [ 17.838031] dump_backtrace+0xe4/0x140 > > [ 17.838042] show_stack+0x20/0x50 > > [ 17.838049] dump_stack_lvl+0x9c/0xd8 > > [ 17.838055] dump_stack+0x18/0x34 > > [ 17.838060] print_circular_bug+0x1f8/0x200 > > [ 17.838066] check_noncircular+0x160/0x180 > > [ 17.838072] __lock_acquire+0x1148/0x1910 > > [ 17.838078] lock_acquire.part.0+0xec/0x260 > > [ 17.838084] lock_acquire+0x68/0x90 > > [ 17.838089] __mutex_lock+0x8c/0x420 > > [ 17.838098] mutex_lock_nested+0x34/0x40 > > [ 17.838104] genpd_lock_mtx+0x20/0x30 > > [ 17.838110] genpd_runtime_resume+0xfc/0x2e0 > > [ 17.838117] __rpm_callback+0x50/0x1b0 > > [ 17.838125] rpm_callback+0x74/0x80 > > [ 17.838132] rpm_resume+0x434/0x6e0 > > [ 17.838138] __pm_runtime_resume+0x54/0xa0 > > [ 17.838145] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > > [ 17.838150] clk_core_set_rate_nolock+0xbc/0x2b0 > > [ 17.838157] clk_set_rate+0x40/0xa0 > > [ 17.838162] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > > [ 17.838175] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > > [ 17.838182] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > > [ 17.838188] commit_tail+0xac/0x1a0 > > [ 17.838194] drm_atomic_helper_commit+0x1a0/0x1c0 > > [ 17.838200] drm_atomic_commit+0x94/0x130 > > [ 17.838205] drm_client_modeset_commit_atomic+0x230/0x2a0 > > [ 17.838212] drm_client_modeset_commit_locked+0x68/0x1d0 > > [ 17.838218] drm_client_modeset_commit+0x38/0x60 > > [ 17.838224] drm_fb_helper_set_par+0xd4/0x130 > > [ 17.838230] fbcon_init+0x3d4/0x540 > > [ 17.838235] visual_init+0xb8/0x110 > > [ 17.838243] do_bind_con_driver.isra.0+0x1c8/0x3a0 > > [ 17.838250] do_take_over_console+0x148/0x200 > > [ 17.838257] do_fbcon_takeover+0x74/0xf0 > > [ 17.838262] fbcon_fb_registered+0x18c/0x200 > > [ 17.838268] register_framebuffer+0x1bc/0x2b0 > > [ 17.838275] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > > [ 17.838282] drm_fbdev_client_hotplug+0xd8/0x1c0 > > [ 17.838288] drm_fbdev_generic_setup+0xbc/0x1b0 > > [ 17.838293] lcdif_probe+0x2b0/0x310 [imx_lcdif] > > [ 17.838304] platform_probe+0x70/0xf0 > > [ 17.838309] really_probe+0xc8/0x3e0 > > [ 17.838316] __driver_probe_device+0x84/0x190 > > [ 17.838323] driver_probe_device+0x44/0x120 > > [ 17.838330] __device_attach_driver+0xc4/0x160 > > [ 17.838338] bus_for_each_drv+0x80/0xe0 > > [ 17.838344] __device_attach+0xb0/0x1f0 > > [ 17.838351] device_initial_probe+0x1c/0x30 > > [ 17.838358] bus_probe_device+0xa4/0xb0 > > [ 17.838365] deferred_probe_work_func+0xac/0x100 > > [ 17.838372] process_one_work+0x294/0x6b0 > > [ 17.838378] worker_thread+0x7c/0x450 > > [ 17.838383] kthread+0x114/0x120 > > [ 17.838390] ret_from_fork+0x10/0x20 > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > --- > > > Cc: Fabio Estevam <festevam@denx.de> > > > Cc: Jacky Bai <ping.bai@nxp.com> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Cc: Peng Fan <peng.fan@nxp.com> > > > Cc: Shawn Guo <shawnguo@kernel.org> > > > Cc: Shengjiu Wang <shengjiu.wang@nxp.com> > > > Cc: Stephen Boyd <sboyd@kernel.org> > > > Cc: linux-clk@vger.kernel.org > > > Cc: linux-imx@nxp.com > > > To: linux-arm-kernel@lists.infradead.org > > > --- > > > drivers/soc/imx/imx8mp-blk-ctrl.c | 14 +++++++++++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c > > > b/drivers/soc/imx/imx8mp-blk-ctrl.c > > > index b7d4161fcda9c..571c98b5b005b 100644 > > > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > > > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > > > @@ -393,15 +393,21 @@ static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd) > > > struct imx8mp_blk_ctrl *bc = domain->bc; > > > int ret; > > > > > > + ret = clk_bulk_prepare(data->num_clks, domain->clks); > > > + if (ret) { > > > + dev_err(bc->dev, "failed to enable clocks\n"); > > > + return ret; > > > + } > > > + > > > /* make sure bus domain is awake */ > > > ret = pm_runtime_resume_and_get(bc->bus_power_dev); > > > if (ret < 0) { > > > dev_err(bc->dev, "failed to power up bus domain\n"); > > > - return ret; > > > + goto rpm_put; > > > } > > > > > > /* enable upstream clocks */ > > > - ret = clk_bulk_prepare_enable(data->num_clks, domain->clks); > > > + ret = clk_bulk_enable(data->num_clks, domain->clks); > > > if (ret) { > > > dev_err(bc->dev, "failed to enable clocks\n"); > > > goto bus_put; > > > @@ -426,9 +432,11 @@ static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd) > > > return 0; > > > > > > clk_disable: > > > - clk_bulk_disable_unprepare(data->num_clks, domain->clks); > > > + clk_bulk_disable(data->num_clks, domain->clks); > > > bus_put: > > > pm_runtime_put(bc->bus_power_dev); > > > +rpm_put: > > > + clk_bulk_unprepare(data->num_clks, domain->clks); > > > > > > return ret; > > > }
On 10/25/22 02:25, Laurent Pinchart wrote: > Hi Marek, Hi, > Thank you for the patch. > > On Mon, Oct 24, 2022 at 05:09:47PM +0200, Marek Vasut wrote: >> It is possible for clk_disable_unused() to trigger lockdep warning >> regarding lock ordering in this driver. This happens in case of the >> following conditions: >> >> A) clock core clk_disable_unused() triggers the following sequence in a >> driver which also uses blkctrl domain: >> - clk_prepare_lock() -> obtains clock core prepare_lock >> - pm_runtime_get*() -> obtains &blk_ctrl_genpd_lock_class >> >> B) driver powers up a power domain and triggers the following sequence >> in blkctrl: >> - pm_runtime_get_sync() -> obtains &blk_ctrl_genpd_lock_class >> - clk_bulk_prepare_enable() -> obtains clock core prepare_lock >> >> This can lead to a deadlock in case A and B runs on separate CPUs. >> >> To avoid the deadlock, split clk_*prepare() from clk_*enable() and >> call the former before pm_runtime_get_sync(). This way, the blkctrl >> driver always claims the prepare_lock before blk_ctrl_genpd_lock_class >> and the deadlock is avoided. > > I'm afraid I'm still getting occasional warnings from lockdep :-( This > one happened at boot time: > > [ 17.837243] > [ 17.837254] ====================================================== > [ 17.837256] WARNING: possible circular locking dependency detected > [ 17.837260] 6.1.0-rc2-00199-gd504fb27855a-dirty #301 Not tainted > [ 17.837265] ------------------------------------------------------ > [ 17.837267] kworker/u8:6/152 is trying to acquire lock: > [ 17.837273] ffff000000eab6f0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}, at: genpd_lock_mtx+0x20/0x30 > [ 17.837300] > [ 17.837300] but task is already holding lock: > [ 17.837302] ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 > [ 17.837321] > [ 17.837321] which lock already depends on the new lock. > [ 17.837321] > [ 17.837323] > [ 17.837323] the existing dependency chain (in reverse order) is: > [ 17.837326] > [ 17.837326] -> #1 (prepare_lock){+.+.}-{3:3}: > [ 17.837337] lock_acquire+0x68/0x90 > [ 17.837345] __mutex_lock+0x8c/0x420 > [ 17.837354] mutex_lock_nested+0x34/0x40 > [ 17.837360] clk_prepare_lock+0x54/0xb0 > [ 17.837367] clk_unprepare+0x2c/0x50 > [ 17.837373] clk_bulk_unprepare+0x44/0x60 > [ 17.837380] imx8mp_blk_ctrl_power_off+0xb4/0x100 > [ 17.837386] _genpd_power_off+0x9c/0x180 > [ 17.837392] genpd_power_off.part.0.isra.0+0x130/0x2a0 > [ 17.837399] genpd_power_off_work_fn+0x6c/0x90 > [ 17.837405] process_one_work+0x294/0x6b0 > [ 17.837412] worker_thread+0x7c/0x450 > [ 17.837416] kthread+0x114/0x120 > [ 17.837424] ret_from_fork+0x10/0x20 > [ 17.837431] > [ 17.837431] -> #0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}: > [ 17.837442] __lock_acquire+0x1148/0x1910 > [ 17.837448] lock_acquire.part.0+0xec/0x260 > [ 17.837454] lock_acquire+0x68/0x90 > [ 17.837459] __mutex_lock+0x8c/0x420 > [ 17.837466] mutex_lock_nested+0x34/0x40 > [ 17.837472] genpd_lock_mtx+0x20/0x30 > [ 17.837478] genpd_runtime_resume+0xfc/0x2e0 > [ 17.837484] __rpm_callback+0x50/0x1b0 > [ 17.837492] rpm_callback+0x74/0x80 > [ 17.837499] rpm_resume+0x434/0x6e0 > [ 17.837505] __pm_runtime_resume+0x54/0xa0 > [ 17.837512] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > [ 17.837518] clk_core_set_rate_nolock+0xbc/0x2b0 > [ 17.837524] clk_set_rate+0x40/0xa0 > [ 17.837529] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > [ 17.837544] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > [ 17.837551] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > [ 17.837558] commit_tail+0xac/0x1a0 > [ 17.837563] drm_atomic_helper_commit+0x1a0/0x1c0 > [ 17.837569] drm_atomic_commit+0x94/0x130 > [ 17.837574] drm_client_modeset_commit_atomic+0x230/0x2a0 > [ 17.837582] drm_client_modeset_commit_locked+0x68/0x1d0 > [ 17.837588] drm_client_modeset_commit+0x38/0x60 > [ 17.837594] drm_fb_helper_set_par+0xd4/0x130 > [ 17.837600] fbcon_init+0x3d4/0x540 > [ 17.837606] visual_init+0xb8/0x110 > [ 17.837613] do_bind_con_driver.isra.0+0x1c8/0x3a0 > [ 17.837620] do_take_over_console+0x148/0x200 > [ 17.837627] do_fbcon_takeover+0x74/0xf0 > [ 17.837632] fbcon_fb_registered+0x18c/0x200 > [ 17.837637] register_framebuffer+0x1bc/0x2b0 > [ 17.837645] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > [ 17.837650] drm_fbdev_client_hotplug+0xd8/0x1c0 > [ 17.837656] drm_fbdev_generic_setup+0xbc/0x1b0 > [ 17.837661] lcdif_probe+0x2b0/0x310 [imx_lcdif] > [ 17.837672] platform_probe+0x70/0xf0 > [ 17.837678] really_probe+0xc8/0x3e0 > [ 17.837685] __driver_probe_device+0x84/0x190 > [ 17.837692] driver_probe_device+0x44/0x120 > [ 17.837699] __device_attach_driver+0xc4/0x160 > [ 17.837707] bus_for_each_drv+0x80/0xe0 > [ 17.837713] __device_attach+0xb0/0x1f0 > [ 17.837720] device_initial_probe+0x1c/0x30 > [ 17.837727] bus_probe_device+0xa4/0xb0 > [ 17.837734] deferred_probe_work_func+0xac/0x100 > [ 17.837740] process_one_work+0x294/0x6b0 > [ 17.837746] worker_thread+0x7c/0x450 > [ 17.837751] kthread+0x114/0x120 > [ 17.837758] ret_from_fork+0x10/0x20 > [ 17.837764] > [ 17.837764] other info that might help us debug this: > [ 17.837764] > [ 17.837766] Possible unsafe locking scenario: > [ 17.837766] > [ 17.837768] CPU0 CPU1 > [ 17.837770] ---- ---- > [ 17.837772] lock(prepare_lock); > [ 17.837779] lock(&blk_ctrl_genpd_lock_class); > [ 17.837785] lock(prepare_lock); > [ 17.837790] lock(&blk_ctrl_genpd_lock_class); > [ 17.837796] > [ 17.837796] *** DEADLOCK *** > [ 17.837796] > [ 17.837799] 11 locks held by kworker/u8:6/152: > [ 17.837803] #0: ffff000000040938 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 > [ 17.837821] #1: ffff80000b74bdd0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 > [ 17.837838] #2: ffff0000003f58f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x40/0x1f0 > [ 17.837856] #3: ffff800009c1ddc0 (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x4c/0x2b0 > [ 17.837875] #4: ffff800009ae4e68 (console_lock){+.+.}-{0:0}, at: fbcon_fb_registered+0x1f8/0x200 > [ 17.837892] #5: ffff000002f1ee78 (&helper->lock){+.+.}-{3:3}, at: drm_fb_helper_set_par+0x98/0x130 > [ 17.837909] #6: ffff0000038611a8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x2c/0x70 > [ 17.837928] #7: ffff000002f1ec98 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x34/0x1d0 > [ 17.837947] #8: ffff80000b74b690 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x50/0x2a0 > [ 17.837964] #9: ffff000003863138 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_lock+0x48/0x130 > [ 17.837984] #10: ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 > [ 17.838003] > [ 17.838003] stack backtrace: > [ 17.838007] CPU: 1 PID: 152 Comm: kworker/u8:6 Not tainted 6.1.0-rc2-00199-gd504fb27855a-dirty #301 > [ 17.838013] Hardware name: Polyhex Debix Model A i.MX8MPlus board (DT) > [ 17.838018] Workqueue: events_unbound deferred_probe_work_func > [ 17.838029] Call trace: > [ 17.838031] dump_backtrace+0xe4/0x140 > [ 17.838042] show_stack+0x20/0x50 > [ 17.838049] dump_stack_lvl+0x9c/0xd8 > [ 17.838055] dump_stack+0x18/0x34 > [ 17.838060] print_circular_bug+0x1f8/0x200 > [ 17.838066] check_noncircular+0x160/0x180 > [ 17.838072] __lock_acquire+0x1148/0x1910 > [ 17.838078] lock_acquire.part.0+0xec/0x260 > [ 17.838084] lock_acquire+0x68/0x90 > [ 17.838089] __mutex_lock+0x8c/0x420 > [ 17.838098] mutex_lock_nested+0x34/0x40 > [ 17.838104] genpd_lock_mtx+0x20/0x30 > [ 17.838110] genpd_runtime_resume+0xfc/0x2e0 > [ 17.838117] __rpm_callback+0x50/0x1b0 > [ 17.838125] rpm_callback+0x74/0x80 > [ 17.838132] rpm_resume+0x434/0x6e0 > [ 17.838138] __pm_runtime_resume+0x54/0xa0 > [ 17.838145] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > [ 17.838150] clk_core_set_rate_nolock+0xbc/0x2b0 > [ 17.838157] clk_set_rate+0x40/0xa0 > [ 17.838162] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > [ 17.838175] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > [ 17.838182] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > [ 17.838188] commit_tail+0xac/0x1a0 > [ 17.838194] drm_atomic_helper_commit+0x1a0/0x1c0 > [ 17.838200] drm_atomic_commit+0x94/0x130 > [ 17.838205] drm_client_modeset_commit_atomic+0x230/0x2a0 > [ 17.838212] drm_client_modeset_commit_locked+0x68/0x1d0 > [ 17.838218] drm_client_modeset_commit+0x38/0x60 > [ 17.838224] drm_fb_helper_set_par+0xd4/0x130 > [ 17.838230] fbcon_init+0x3d4/0x540 > [ 17.838235] visual_init+0xb8/0x110 > [ 17.838243] do_bind_con_driver.isra.0+0x1c8/0x3a0 > [ 17.838250] do_take_over_console+0x148/0x200 > [ 17.838257] do_fbcon_takeover+0x74/0xf0 > [ 17.838262] fbcon_fb_registered+0x18c/0x200 > [ 17.838268] register_framebuffer+0x1bc/0x2b0 > [ 17.838275] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > [ 17.838282] drm_fbdev_client_hotplug+0xd8/0x1c0 > [ 17.838288] drm_fbdev_generic_setup+0xbc/0x1b0 > [ 17.838293] lcdif_probe+0x2b0/0x310 [imx_lcdif] > [ 17.838304] platform_probe+0x70/0xf0 > [ 17.838309] really_probe+0xc8/0x3e0 > [ 17.838316] __driver_probe_device+0x84/0x190 > [ 17.838323] driver_probe_device+0x44/0x120 > [ 17.838330] __device_attach_driver+0xc4/0x160 > [ 17.838338] bus_for_each_drv+0x80/0xe0 > [ 17.838344] __device_attach+0xb0/0x1f0 > [ 17.838351] device_initial_probe+0x1c/0x30 > [ 17.838358] bus_probe_device+0xa4/0xb0 > [ 17.838365] deferred_probe_work_func+0xac/0x100 > [ 17.838372] process_one_work+0x294/0x6b0 > [ 17.838378] worker_thread+0x7c/0x450 > [ 17.838383] kthread+0x114/0x120 > [ 17.838390] ret_from_fork+0x10/0x20 Let's see if this works any better: https://patchwork.kernel.org/project/linux-pm/list/?series=693012
Hi Marek, On Wed, Nov 09, 2022 at 01:28:03AM +0100, Marek Vasut wrote: > On 10/25/22 02:25, Laurent Pinchart wrote: > > On Mon, Oct 24, 2022 at 05:09:47PM +0200, Marek Vasut wrote: > >> It is possible for clk_disable_unused() to trigger lockdep warning > >> regarding lock ordering in this driver. This happens in case of the > >> following conditions: > >> > >> A) clock core clk_disable_unused() triggers the following sequence in a > >> driver which also uses blkctrl domain: > >> - clk_prepare_lock() -> obtains clock core prepare_lock > >> - pm_runtime_get*() -> obtains &blk_ctrl_genpd_lock_class > >> > >> B) driver powers up a power domain and triggers the following sequence > >> in blkctrl: > >> - pm_runtime_get_sync() -> obtains &blk_ctrl_genpd_lock_class > >> - clk_bulk_prepare_enable() -> obtains clock core prepare_lock > >> > >> This can lead to a deadlock in case A and B runs on separate CPUs. > >> > >> To avoid the deadlock, split clk_*prepare() from clk_*enable() and > >> call the former before pm_runtime_get_sync(). This way, the blkctrl > >> driver always claims the prepare_lock before blk_ctrl_genpd_lock_class > >> and the deadlock is avoided. > > > > I'm afraid I'm still getting occasional warnings from lockdep :-( This > > one happened at boot time: > > > > [ 17.837243] > > [ 17.837254] ====================================================== > > [ 17.837256] WARNING: possible circular locking dependency detected > > [ 17.837260] 6.1.0-rc2-00199-gd504fb27855a-dirty #301 Not tainted > > [ 17.837265] ------------------------------------------------------ > > [ 17.837267] kworker/u8:6/152 is trying to acquire lock: > > [ 17.837273] ffff000000eab6f0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}, at: genpd_lock_mtx+0x20/0x30 > > [ 17.837300] > > [ 17.837300] but task is already holding lock: > > [ 17.837302] ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 > > [ 17.837321] > > [ 17.837321] which lock already depends on the new lock. > > [ 17.837321] > > [ 17.837323] > > [ 17.837323] the existing dependency chain (in reverse order) is: > > [ 17.837326] > > [ 17.837326] -> #1 (prepare_lock){+.+.}-{3:3}: > > [ 17.837337] lock_acquire+0x68/0x90 > > [ 17.837345] __mutex_lock+0x8c/0x420 > > [ 17.837354] mutex_lock_nested+0x34/0x40 > > [ 17.837360] clk_prepare_lock+0x54/0xb0 > > [ 17.837367] clk_unprepare+0x2c/0x50 > > [ 17.837373] clk_bulk_unprepare+0x44/0x60 > > [ 17.837380] imx8mp_blk_ctrl_power_off+0xb4/0x100 > > [ 17.837386] _genpd_power_off+0x9c/0x180 > > [ 17.837392] genpd_power_off.part.0.isra.0+0x130/0x2a0 > > [ 17.837399] genpd_power_off_work_fn+0x6c/0x90 > > [ 17.837405] process_one_work+0x294/0x6b0 > > [ 17.837412] worker_thread+0x7c/0x450 > > [ 17.837416] kthread+0x114/0x120 > > [ 17.837424] ret_from_fork+0x10/0x20 > > [ 17.837431] > > [ 17.837431] -> #0 (&blk_ctrl_genpd_lock_class){+.+.}-{3:3}: > > [ 17.837442] __lock_acquire+0x1148/0x1910 > > [ 17.837448] lock_acquire.part.0+0xec/0x260 > > [ 17.837454] lock_acquire+0x68/0x90 > > [ 17.837459] __mutex_lock+0x8c/0x420 > > [ 17.837466] mutex_lock_nested+0x34/0x40 > > [ 17.837472] genpd_lock_mtx+0x20/0x30 > > [ 17.837478] genpd_runtime_resume+0xfc/0x2e0 > > [ 17.837484] __rpm_callback+0x50/0x1b0 > > [ 17.837492] rpm_callback+0x74/0x80 > > [ 17.837499] rpm_resume+0x434/0x6e0 > > [ 17.837505] __pm_runtime_resume+0x54/0xa0 > > [ 17.837512] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > > [ 17.837518] clk_core_set_rate_nolock+0xbc/0x2b0 > > [ 17.837524] clk_set_rate+0x40/0xa0 > > [ 17.837529] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > > [ 17.837544] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > > [ 17.837551] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > > [ 17.837558] commit_tail+0xac/0x1a0 > > [ 17.837563] drm_atomic_helper_commit+0x1a0/0x1c0 > > [ 17.837569] drm_atomic_commit+0x94/0x130 > > [ 17.837574] drm_client_modeset_commit_atomic+0x230/0x2a0 > > [ 17.837582] drm_client_modeset_commit_locked+0x68/0x1d0 > > [ 17.837588] drm_client_modeset_commit+0x38/0x60 > > [ 17.837594] drm_fb_helper_set_par+0xd4/0x130 > > [ 17.837600] fbcon_init+0x3d4/0x540 > > [ 17.837606] visual_init+0xb8/0x110 > > [ 17.837613] do_bind_con_driver.isra.0+0x1c8/0x3a0 > > [ 17.837620] do_take_over_console+0x148/0x200 > > [ 17.837627] do_fbcon_takeover+0x74/0xf0 > > [ 17.837632] fbcon_fb_registered+0x18c/0x200 > > [ 17.837637] register_framebuffer+0x1bc/0x2b0 > > [ 17.837645] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > > [ 17.837650] drm_fbdev_client_hotplug+0xd8/0x1c0 > > [ 17.837656] drm_fbdev_generic_setup+0xbc/0x1b0 > > [ 17.837661] lcdif_probe+0x2b0/0x310 [imx_lcdif] > > [ 17.837672] platform_probe+0x70/0xf0 > > [ 17.837678] really_probe+0xc8/0x3e0 > > [ 17.837685] __driver_probe_device+0x84/0x190 > > [ 17.837692] driver_probe_device+0x44/0x120 > > [ 17.837699] __device_attach_driver+0xc4/0x160 > > [ 17.837707] bus_for_each_drv+0x80/0xe0 > > [ 17.837713] __device_attach+0xb0/0x1f0 > > [ 17.837720] device_initial_probe+0x1c/0x30 > > [ 17.837727] bus_probe_device+0xa4/0xb0 > > [ 17.837734] deferred_probe_work_func+0xac/0x100 > > [ 17.837740] process_one_work+0x294/0x6b0 > > [ 17.837746] worker_thread+0x7c/0x450 > > [ 17.837751] kthread+0x114/0x120 > > [ 17.837758] ret_from_fork+0x10/0x20 > > [ 17.837764] > > [ 17.837764] other info that might help us debug this: > > [ 17.837764] > > [ 17.837766] Possible unsafe locking scenario: > > [ 17.837766] > > [ 17.837768] CPU0 CPU1 > > [ 17.837770] ---- ---- > > [ 17.837772] lock(prepare_lock); > > [ 17.837779] lock(&blk_ctrl_genpd_lock_class); > > [ 17.837785] lock(prepare_lock); > > [ 17.837790] lock(&blk_ctrl_genpd_lock_class); > > [ 17.837796] > > [ 17.837796] *** DEADLOCK *** > > [ 17.837796] > > [ 17.837799] 11 locks held by kworker/u8:6/152: > > [ 17.837803] #0: ffff000000040938 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 > > [ 17.837821] #1: ffff80000b74bdd0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1fc/0x6b0 > > [ 17.837838] #2: ffff0000003f58f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x40/0x1f0 > > [ 17.837856] #3: ffff800009c1ddc0 (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x4c/0x2b0 > > [ 17.837875] #4: ffff800009ae4e68 (console_lock){+.+.}-{0:0}, at: fbcon_fb_registered+0x1f8/0x200 > > [ 17.837892] #5: ffff000002f1ee78 (&helper->lock){+.+.}-{3:3}, at: drm_fb_helper_set_par+0x98/0x130 > > [ 17.837909] #6: ffff0000038611a8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x2c/0x70 > > [ 17.837928] #7: ffff000002f1ec98 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x34/0x1d0 > > [ 17.837947] #8: ffff80000b74b690 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x50/0x2a0 > > [ 17.837964] #9: ffff000003863138 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_lock+0x48/0x130 > > [ 17.837984] #10: ffff800009c1e5e8 (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x34/0xb0 > > [ 17.838003] > > [ 17.838003] stack backtrace: > > [ 17.838007] CPU: 1 PID: 152 Comm: kworker/u8:6 Not tainted 6.1.0-rc2-00199-gd504fb27855a-dirty #301 > > [ 17.838013] Hardware name: Polyhex Debix Model A i.MX8MPlus board (DT) > > [ 17.838018] Workqueue: events_unbound deferred_probe_work_func > > [ 17.838029] Call trace: > > [ 17.838031] dump_backtrace+0xe4/0x140 > > [ 17.838042] show_stack+0x20/0x50 > > [ 17.838049] dump_stack_lvl+0x9c/0xd8 > > [ 17.838055] dump_stack+0x18/0x34 > > [ 17.838060] print_circular_bug+0x1f8/0x200 > > [ 17.838066] check_noncircular+0x160/0x180 > > [ 17.838072] __lock_acquire+0x1148/0x1910 > > [ 17.838078] lock_acquire.part.0+0xec/0x260 > > [ 17.838084] lock_acquire+0x68/0x90 > > [ 17.838089] __mutex_lock+0x8c/0x420 > > [ 17.838098] mutex_lock_nested+0x34/0x40 > > [ 17.838104] genpd_lock_mtx+0x20/0x30 > > [ 17.838110] genpd_runtime_resume+0xfc/0x2e0 > > [ 17.838117] __rpm_callback+0x50/0x1b0 > > [ 17.838125] rpm_callback+0x74/0x80 > > [ 17.838132] rpm_resume+0x434/0x6e0 > > [ 17.838138] __pm_runtime_resume+0x54/0xa0 > > [ 17.838145] clk_pm_runtime_get.part.0.isra.0+0x24/0xa0 > > [ 17.838150] clk_core_set_rate_nolock+0xbc/0x2b0 > > [ 17.838157] clk_set_rate+0x40/0xa0 > > [ 17.838162] lcdif_crtc_atomic_enable+0x98/0x880 [imx_lcdif] > > [ 17.838175] drm_atomic_helper_commit_modeset_enables+0xac/0x2b0 > > [ 17.838182] drm_atomic_helper_commit_tail_rpm+0x58/0xb0 > > [ 17.838188] commit_tail+0xac/0x1a0 > > [ 17.838194] drm_atomic_helper_commit+0x1a0/0x1c0 > > [ 17.838200] drm_atomic_commit+0x94/0x130 > > [ 17.838205] drm_client_modeset_commit_atomic+0x230/0x2a0 > > [ 17.838212] drm_client_modeset_commit_locked+0x68/0x1d0 > > [ 17.838218] drm_client_modeset_commit+0x38/0x60 > > [ 17.838224] drm_fb_helper_set_par+0xd4/0x130 > > [ 17.838230] fbcon_init+0x3d4/0x540 > > [ 17.838235] visual_init+0xb8/0x110 > > [ 17.838243] do_bind_con_driver.isra.0+0x1c8/0x3a0 > > [ 17.838250] do_take_over_console+0x148/0x200 > > [ 17.838257] do_fbcon_takeover+0x74/0xf0 > > [ 17.838262] fbcon_fb_registered+0x18c/0x200 > > [ 17.838268] register_framebuffer+0x1bc/0x2b0 > > [ 17.838275] __drm_fb_helper_initial_config_and_unlock+0x338/0x5c0 > > [ 17.838282] drm_fbdev_client_hotplug+0xd8/0x1c0 > > [ 17.838288] drm_fbdev_generic_setup+0xbc/0x1b0 > > [ 17.838293] lcdif_probe+0x2b0/0x310 [imx_lcdif] > > [ 17.838304] platform_probe+0x70/0xf0 > > [ 17.838309] really_probe+0xc8/0x3e0 > > [ 17.838316] __driver_probe_device+0x84/0x190 > > [ 17.838323] driver_probe_device+0x44/0x120 > > [ 17.838330] __device_attach_driver+0xc4/0x160 > > [ 17.838338] bus_for_each_drv+0x80/0xe0 > > [ 17.838344] __device_attach+0xb0/0x1f0 > > [ 17.838351] device_initial_probe+0x1c/0x30 > > [ 17.838358] bus_probe_device+0xa4/0xb0 > > [ 17.838365] deferred_probe_work_func+0xac/0x100 > > [ 17.838372] process_one_work+0x294/0x6b0 > > [ 17.838378] worker_thread+0x7c/0x450 > > [ 17.838383] kthread+0x114/0x120 > > [ 17.838390] ret_from_fork+0x10/0x20 > > Let's see if this works any better: > > https://patchwork.kernel.org/project/linux-pm/list/?series=693012 I've added that to my tree, but won't be able to test it this week as I'm travelling and don't have access to the necessary hardware. I'll try to report test results next week.
diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c index b7d4161fcda9c..571c98b5b005b 100644 --- a/drivers/soc/imx/imx8mp-blk-ctrl.c +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c @@ -393,15 +393,21 @@ static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd) struct imx8mp_blk_ctrl *bc = domain->bc; int ret; + ret = clk_bulk_prepare(data->num_clks, domain->clks); + if (ret) { + dev_err(bc->dev, "failed to enable clocks\n"); + return ret; + } + /* make sure bus domain is awake */ ret = pm_runtime_resume_and_get(bc->bus_power_dev); if (ret < 0) { dev_err(bc->dev, "failed to power up bus domain\n"); - return ret; + goto rpm_put; } /* enable upstream clocks */ - ret = clk_bulk_prepare_enable(data->num_clks, domain->clks); + ret = clk_bulk_enable(data->num_clks, domain->clks); if (ret) { dev_err(bc->dev, "failed to enable clocks\n"); goto bus_put; @@ -426,9 +432,11 @@ static int imx8mp_blk_ctrl_power_on(struct generic_pm_domain *genpd) return 0; clk_disable: - clk_bulk_disable_unprepare(data->num_clks, domain->clks); + clk_bulk_disable(data->num_clks, domain->clks); bus_put: pm_runtime_put(bc->bus_power_dev); +rpm_put: + clk_bulk_unprepare(data->num_clks, domain->clks); return ret; }
It is possible for clk_disable_unused() to trigger lockdep warning regarding lock ordering in this driver. This happens in case of the following conditions: A) clock core clk_disable_unused() triggers the following sequence in a driver which also uses blkctrl domain: - clk_prepare_lock() -> obtains clock core prepare_lock - pm_runtime_get*() -> obtains &blk_ctrl_genpd_lock_class B) driver powers up a power domain and triggers the following sequence in blkctrl: - pm_runtime_get_sync() -> obtains &blk_ctrl_genpd_lock_class - clk_bulk_prepare_enable() -> obtains clock core prepare_lock This can lead to a deadlock in case A and B runs on separate CPUs. To avoid the deadlock, split clk_*prepare() from clk_*enable() and call the former before pm_runtime_get_sync(). This way, the blkctrl driver always claims the prepare_lock before blk_ctrl_genpd_lock_class and the deadlock is avoided. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Fabio Estevam <festevam@denx.de> Cc: Jacky Bai <ping.bai@nxp.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Peng Fan <peng.fan@nxp.com> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Shengjiu Wang <shengjiu.wang@nxp.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: linux-clk@vger.kernel.org Cc: linux-imx@nxp.com To: linux-arm-kernel@lists.infradead.org --- drivers/soc/imx/imx8mp-blk-ctrl.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)