Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935416AbdGTMyI (ORCPT ); Thu, 20 Jul 2017 08:54:08 -0400 Received: from foss.arm.com ([217.140.101.70]:53410 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934151AbdGTMyH (ORCPT ); Thu, 20 Jul 2017 08:54:07 -0400 Date: Thu, 20 Jul 2017 13:54:04 +0100 From: Liviu Dudau To: Russell King - ARM Linux Cc: David Airlie , DRI devel , LKML Subject: Re: [PATCH] drm/i2c: tda998x: Fix lockdep warning about possible circular dependency Message-ID: <20170720125404.GA3532@e110455-lin.cambridge.arm.com> References: <20170720110450.7435-1-Liviu.Dudau@arm.com> <20170720113853.GX31807@n2100.armlinux.org.uk> <20170720114449.GA19026@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170720114449.GA19026@n2100.armlinux.org.uk> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14052 Lines: 248 On Thu, Jul 20, 2017 at 12:44:49PM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 20, 2017 at 12:38:53PM +0100, Russell King - ARM Linux wrote: > > On Thu, Jul 20, 2017 at 12:04:50PM +0100, Liviu Dudau wrote: > > > When enabling lockdep debugging on Juno platform with HDLCD and TDA998x > > > I get the following warning from the system: > > > > > > [ 25.990733] ====================================================== > > > [ 25.998637] WARNING: possible circular locking dependency detected > > > [ 26.006531] 4.13.0-rc1-00284-g28c0a682ecbf-dirty #17 Not tainted > > > [ 26.014246] ------------------------------------------------------ > > > [ 26.022142] kworker/1:2/140 is trying to acquire lock: > > > [ 26.029001] (&priv->audio_mutex){+.+.+.}, at: [] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x] > > > [ 26.041100] > > > [ 26.041100] but task is already holding lock: > > > [ 26.050436] (crtc_ww_class_mutex){+.+.+.}, at: [] drm_modeset_lock+0x64/0xf8 [drm] > > > [ 26.061531] > > > [ 26.061531] which lock already depends on the new lock. > > > [ 26.061531] > > > [ 26.075063] > > > [ 26.075063] the existing dependency chain (in reverse order) is: > > > [ 26.086031] > > > [ 26.086031] -> #2 (crtc_ww_class_mutex){+.+.+.}: > > > [ 26.095657] __lock_acquire+0x18a0/0x19b8 > > > [ 26.101918] lock_acquire+0xd0/0x2b0 > > > [ 26.107731] __ww_mutex_lock.constprop.3+0x90/0xe78 > > > [ 26.114817] ww_mutex_lock+0x54/0xe0 > > > [ 26.120672] drm_modeset_lock+0x64/0xf8 [drm] > > > [ 26.127253] drm_helper_probe_single_connector_modes+0x7c/0x6b8 [drm_kms_helper] > > > [ 26.136829] tda998x_connector_fill_modes+0x44/0xa8 [tda998x] > > > [ 26.144797] drm_setup_crtcs+0x19c/0xba0 [drm_kms_helper] > > > [ 26.152429] drm_fb_helper_initial_config+0x70/0x440 [drm_kms_helper] > > > [ 26.161097] drm_fbdev_cma_init_with_funcs+0x94/0x168 [drm_kms_helper] > > > [ 26.169857] drm_fbdev_cma_init+0x38/0x50 [drm_kms_helper] > > > [ 26.177559] hdlcd_drm_bind+0x1f8/0x4a8 [hdlcd] > > > [ 26.184310] try_to_bring_up_master+0x180/0x1e0 > > > [ 26.191043] component_master_add_with_match+0xb0/0x108 > > > [ 26.198458] hdlcd_probe+0x58/0x80 [hdlcd] > > > [ 26.204735] platform_drv_probe+0x60/0xc0 > > > [ 26.210913] driver_probe_device+0x23c/0x2e8 > > > [ 26.217350] __driver_attach+0xd4/0xd8 > > > [ 26.223256] bus_for_each_dev+0x5c/0xa8 > > > [ 26.229232] driver_attach+0x30/0x40 > > > [ 26.234917] bus_add_driver+0x1d8/0x248 > > > [ 26.240831] driver_register+0x6c/0x118 > > > [ 26.246715] __platform_driver_register+0x54/0x60 > > > [ 26.253461] 0xffff000000e1b018 > > > [ 26.258644] do_one_initcall+0x44/0x138 > > > [ 26.264503] do_init_module+0x64/0x1d4 > > > [ 26.270238] load_module+0x1f90/0x2590 > > > [ 26.275957] SyS_finit_module+0xb0/0xc8 > > > [ 26.281765] __sys_trace_return+0x0/0x4 > > > [ 26.281767] > > > [ 26.281767] -> #1 (crtc_ww_class_acquire){+.+.+.}: > > > [ 26.281778] __lock_acquire+0x18a0/0x19b8 > > > [ 26.281782] lock_acquire+0xd0/0x2b0 > > > [ 26.281877] drm_modeset_acquire_init+0xa8/0xe0 [drm] > > > [ 26.281921] drm_helper_probe_single_connector_modes+0x48/0x6b8 [drm_kms_helper] > > > [ 26.281929] tda998x_connector_fill_modes+0x44/0xa8 [tda998x] > > > [ 26.281970] drm_setup_crtcs+0x19c/0xba0 [drm_kms_helper] > > > [ 26.282009] drm_fb_helper_initial_config+0x70/0x440 [drm_kms_helper] > > > [ 26.282049] drm_fbdev_cma_init_with_funcs+0x94/0x168 [drm_kms_helper] > > > [ 26.282088] drm_fbdev_cma_init+0x38/0x50 [drm_kms_helper] > > > [ 26.282095] hdlcd_drm_bind+0x1f8/0x4a8 [hdlcd] > > > [ 26.282099] try_to_bring_up_master+0x180/0x1e0 > > > [ 26.282104] component_master_add_with_match+0xb0/0x108 > > > [ 26.282110] hdlcd_probe+0x58/0x80 [hdlcd] > > > [ 26.282114] platform_drv_probe+0x60/0xc0 > > > [ 26.282117] driver_probe_device+0x23c/0x2e8 > > > [ 26.282121] __driver_attach+0xd4/0xd8 > > > [ 26.282124] bus_for_each_dev+0x5c/0xa8 > > > [ 26.282127] driver_attach+0x30/0x40 > > > [ 26.282130] bus_add_driver+0x1d8/0x248 > > > [ 26.282134] driver_register+0x6c/0x118 > > > [ 26.282138] __platform_driver_register+0x54/0x60 > > > [ 26.282141] 0xffff000000e1b018 > > > [ 26.282145] do_one_initcall+0x44/0x138 > > > [ 26.282149] do_init_module+0x64/0x1d4 > > > [ 26.282152] load_module+0x1f90/0x2590 > > > [ 26.282156] SyS_finit_module+0xb0/0xc8 > > > [ 26.282159] __sys_trace_return+0x0/0x4 > > > [ 26.282161] > > > [ 26.282161] -> #0 (&priv->audio_mutex){+.+.+.}: > > > [ 26.282172] print_circular_bug+0x80/0x2e0 > > > [ 26.282176] __lock_acquire+0x15a8/0x19b8 > > > [ 26.282180] lock_acquire+0xd0/0x2b0 > > > [ 26.282184] __mutex_lock+0x78/0x8e0 > > > [ 26.282188] mutex_lock_nested+0x3c/0x50 > > > [ 26.282196] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x] > > > [ 26.282237] drm_atomic_helper_commit_modeset_disables+0x328/0x3a0 [drm_kms_helper] > > > [ 26.282251] malidp_atomic_commit_tail+0x44/0x6b0 [mali_dp] > > > [ 26.282292] commit_tail+0x4c/0x80 [drm_kms_helper] > > > [ 26.282333] drm_atomic_helper_commit+0xe8/0x180 [drm_kms_helper] > > > [ 26.282427] drm_atomic_commit+0x54/0x70 [drm] > > > [ 26.282467] restore_fbdev_mode_atomic+0x1f0/0x220 [drm_kms_helper] > > > [ 26.282507] restore_fbdev_mode+0x38/0x188 [drm_kms_helper] > > > [ 26.282547] drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xd0 [drm_kms_helper] > > > [ 26.282586] drm_fb_helper_set_par+0x34/0x80 [drm_kms_helper] > > > [ 26.282625] drm_fb_helper_hotplug_event.part.19+0x94/0xb0 [drm_kms_helper] > > > [ 26.282665] drm_fb_helper_hotplug_event+0x2c/0x48 [drm_kms_helper] > > > [ 26.282704] drm_fbdev_cma_hotplug_event+0x24/0x30 [drm_kms_helper] > > > [ 26.282716] malidp_output_poll_changed+0x24/0x30 [mali_dp] > > > [ 26.282757] drm_kms_helper_hotplug_event+0x34/0x40 [drm_kms_helper] > > > [ 26.282797] output_poll_execute+0x1a0/0x1f0 [drm_kms_helper] > > > [ 26.282803] process_one_work+0x280/0x790 > > > [ 26.282808] worker_thread+0x48/0x450 > > > [ 26.282812] kthread+0x138/0x140 > > > [ 26.282815] ret_from_fork+0x10/0x40 > > > [ 26.282817] > > > [ 26.282817] other info that might help us debug this: > > > [ 26.282817] > > > [ 26.282819] Chain exists of: > > > [ 26.282819] &priv->audio_mutex --> crtc_ww_class_acquire --> crtc_ww_class_mutex > > > [ 26.282819] > > > [ 26.282830] Possible unsafe locking scenario: > > > [ 26.282830] > > > [ 26.282832] CPU0 CPU1 > > > [ 26.282834] ---- ---- > > > [ 26.282835] lock(crtc_ww_class_mutex); > > > [ 26.282840] lock(crtc_ww_class_acquire); > > > [ 26.282845] lock(crtc_ww_class_mutex); > > > [ 26.282850] lock(&priv->audio_mutex); > > > [ 26.282854] > > > [ 26.282854] *** DEADLOCK *** > > > [ 26.282854] > > > [ 26.282858] 5 locks held by kworker/1:2/140: > > > [ 26.282859] #0: ("events"){.+.+.+}, at: [] process_one_work+0x1d8/0x790 > > > [ 26.282871] #1: ((&(&dev->mode_config.output_poll_work)->work)){+.+.+.}, at: [] process_one_work+0x1d8/0x790 > > > [ 26.282883] #2: (&helper->lock){+.+.+.}, at: [] drm_fb_helper_restore_fbdev_mode_unlocked+0x3c/0xd0 [drm_kms_helper] > > > [ 26.282929] #3: (crtc_ww_class_acquire){+.+.+.}, at: [] restore_fbdev_mode_atomic+0x38/0x220 [drm_kms_helper] > > > [ 26.282976] #4: (crtc_ww_class_mutex){+.+.+.}, at: [] drm_modeset_lock+0x64/0xf8 [drm] > > > [ 26.283077] > > > [ 26.283077] stack backtrace: > > > [ 26.283082] CPU: 1 PID: 140 Comm: kworker/1:2 Not tainted 4.13.0-rc1-00284-g28c0a682ecbf-dirty #17 > > > [ 26.283084] Hardware name: ARM Juno development board (r0) (DT) > > > [ 26.283127] Workqueue: events output_poll_execute [drm_kms_helper] > > > [ 26.283131] Call trace: > > > [ 26.283137] [] dump_backtrace+0x0/0x268 > > > [ 26.283142] [] show_stack+0x24/0x30 > > > [ 26.283146] [] dump_stack+0xbc/0xf4 > > > [ 26.283151] [] print_circular_bug+0x1d4/0x2e0 > > > [ 26.283155] [] __lock_acquire+0x15a8/0x19b8 > > > [ 26.283159] [] lock_acquire+0xd0/0x2b0 > > > [ 26.283163] [] __mutex_lock+0x78/0x8e0 > > > [ 26.283168] [] mutex_lock_nested+0x3c/0x50 > > > [ 26.283176] [] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x] > > > [ 26.283217] [] drm_atomic_helper_commit_modeset_disables+0x328/0x3a0 [drm_kms_helper] > > > [ 26.283230] [] malidp_atomic_commit_tail+0x44/0x6b0 [mali_dp] > > > [ 26.283271] [] commit_tail+0x4c/0x80 [drm_kms_helper] > > > [ 26.283312] [] drm_atomic_helper_commit+0xe8/0x180 [drm_kms_helper] > > > [ 26.283406] [] drm_atomic_commit+0x54/0x70 [drm] > > > [ 26.283447] [] restore_fbdev_mode_atomic+0x1f0/0x220 [drm_kms_helper] > > > [ 26.283487] [] restore_fbdev_mode+0x38/0x188 [drm_kms_helper] > > > [ 26.283526] [] drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xd0 [drm_kms_helper] > > > [ 26.283566] [] drm_fb_helper_set_par+0x34/0x80 [drm_kms_helper] > > > [ 26.283606] [] drm_fb_helper_hotplug_event.part.19+0x94/0xb0 [drm_kms_helper] > > > [ 26.283645] [] drm_fb_helper_hotplug_event+0x2c/0x48 [drm_kms_helper] > > > [ 26.283685] [] drm_fbdev_cma_hotplug_event+0x24/0x30 [drm_kms_helper] > > > [ 26.283697] [] malidp_output_poll_changed+0x24/0x30 [mali_dp] > > > [ 26.283738] [] drm_kms_helper_hotplug_event+0x34/0x40 [drm_kms_helper] > > > [ 26.283779] [] output_poll_execute+0x1a0/0x1f0 [drm_kms_helper] > > > [ 26.283784] [] process_one_work+0x280/0x790 > > > [ 26.283788] [] worker_thread+0x48/0x450 > > > [ 26.283792] [] kthread+0x138/0x140 > > > [ 26.283796] [] ret_from_fork+0x10/0x40 > > > > > > Fix the warning by moving the acquiring of the priv->audio_mutex in > > > tda998x_connector_fill_modes() after the drm_helper_probe_single_connector_modes(). > > > > > > Signed-off-by: Liviu Dudau > > > Cc: Russell King > > > Fixes: 02efac0fbfd5 ("drm/i2c: tda998x: remove complexity from tda998x_audio_get_eld()") > > > --- > > > drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > > > index d1e7ac540199..006ffeb50f34 100644 > > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > > > @@ -983,9 +983,9 @@ static int tda998x_connector_fill_modes(struct drm_connector *connector, > > > struct tda998x_priv *priv = conn_to_tda998x_priv(connector); > > > int ret; > > > > > > - mutex_lock(&priv->audio_mutex); > > > ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY); > > > > > > + mutex_lock(&priv->audio_mutex); > > Hi Russell, > > This breaks the locking strategy. TBH I don't claim that I'm familiar with the locking strategy here, I was just parsing the warning's stack dump. > > > > tda998x_audio_get_eld() takes the mutex to safely read the ELD. The > > ELD is updated in tda998x_connector_get_modes(), which is called > > within drm_helper_probe_single_connector_modes(). Moving the mutex > > in this way means that the update of the ELD happens outside the lock, > > which then means there's no protection between reading and writing > > the ELD. First, please note that tda998x_audio_get_eld() is only involved as far as the name of the patch that I claim to fix, it doesn't seem to be present in the call trace. I picked that patch as the likely place where the order of the calls was last introduced. Second, maybe I'm missing something obvious, but drm_edid_to_eld() updates connector->eld without taking any lock, so I don't see how tda998x_audio_get_eld() is prevented from reading an ELD that is being updated at the same time. > > > > What could be done instead is to move the locking as you have above, > > but also move the call to drm_edid_to_eld(connector, edid) into this > > function, also within the lock. This has the advantage that if the > > user needs to override the EDID, then the overriden EDID is also used > > for the ELD. > > > > IMHO that's more correct as the reason for overriding the EDID may be > > to correct the audio information. > > Actually, scrub that idea - drm_helper_probe_single_connector_modes() > calls drm_edid_to_eld() for these cases anyway, so we must call > drm_helper_probe_single_connector_modes() with the audio_mutex held. OK, so the lockdep warning is spurious? Best regards, Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯