Re: [PATCH] drm/i2c: tda998x: Fix lockdep warning about possible circular dependency

[Russell King - ARM Linux <linux@armlinux.org.uk>]

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: [<ffff000000d0319c>] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x]
> [   26.041100]
> [   26.041100] but task is already holding lock:
> [   26.050436]  (crtc_ww_class_mutex){+.+.+.}, at: [<ffff000000eaefe4>] 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: [<ffff0000080f8500>] process_one_work+0x1d8/0x790
> [   26.282871]  #1:  ((&(&dev->mode_config.output_poll_work)->work)){+.+.+.}, at: [<ffff0000080f8500>] process_one_work+0x1d8/0x790
> [   26.282883]  #2:  (&helper->lock){+.+.+.}, at: [<ffff000000c0631c>] drm_fb_helper_restore_fbdev_mode_unlocked+0x3c/0xd0 [drm_kms_helper]
> [   26.282929]  #3:  (crtc_ww_class_acquire){+.+.+.}, at: [<ffff000000c02d80>] restore_fbdev_mode_atomic+0x38/0x220 [drm_kms_helper]
> [   26.282976]  #4:  (crtc_ww_class_mutex){+.+.+.}, at: [<ffff000000eaefe4>] 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] [<ffff00000808a778>] dump_backtrace+0x0/0x268
> [   26.283142] [<ffff00000808aabc>] show_stack+0x24/0x30
> [   26.283146] [<ffff000008aa36a8>] dump_stack+0xbc/0xf4
> [   26.283151] [<ffff00000812f454>] print_circular_bug+0x1d4/0x2e0
> [   26.283155] [<ffff000008132480>] __lock_acquire+0x15a8/0x19b8
> [   26.283159] [<ffff000008133008>] lock_acquire+0xd0/0x2b0
> [   26.283163] [<ffff000008aba060>] __mutex_lock+0x78/0x8e0
> [   26.283168] [<ffff000008aba904>] mutex_lock_nested+0x3c/0x50
> [   26.283176] [<ffff000000d0319c>] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x]
> [   26.283217] [<ffff000000c00050>] drm_atomic_helper_commit_modeset_disables+0x328/0x3a0 [drm_kms_helper]
> [   26.283230] [<ffff000000f1bd0c>] malidp_atomic_commit_tail+0x44/0x6b0 [mali_dp]
> [   26.283271] [<ffff000000c0045c>] commit_tail+0x4c/0x80 [drm_kms_helper]
> [   26.283312] [<ffff000000c00630>] drm_atomic_helper_commit+0xe8/0x180 [drm_kms_helper]
> [   26.283406] [<ffff000000eb1604>] drm_atomic_commit+0x54/0x70 [drm]
> [   26.283447] [<ffff000000c02f38>] restore_fbdev_mode_atomic+0x1f0/0x220 [drm_kms_helper]
> [   26.283487] [<ffff000000c03cf0>] restore_fbdev_mode+0x38/0x188 [drm_kms_helper]
> [   26.283526] [<ffff000000c06324>] drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xd0 [drm_kms_helper]
> [   26.283566] [<ffff000000c0619c>] drm_fb_helper_set_par+0x34/0x80 [drm_kms_helper]
> [   26.283606] [<ffff000000c0627c>] drm_fb_helper_hotplug_event.part.19+0x94/0xb0 [drm_kms_helper]
> [   26.283645] [<ffff000000c062c4>] drm_fb_helper_hotplug_event+0x2c/0x48 [drm_kms_helper]
> [   26.283685] [<ffff000000c07124>] drm_fbdev_cma_hotplug_event+0x24/0x30 [drm_kms_helper]
> [   26.283697] [<ffff000000f1b44c>] malidp_output_poll_changed+0x24/0x30 [mali_dp]
> [   26.283738] [<ffff000000bf5264>] drm_kms_helper_hotplug_event+0x34/0x40 [drm_kms_helper]
> [   26.283779] [<ffff000000bf5480>] output_poll_execute+0x1a0/0x1f0 [drm_kms_helper]
> [   26.283784] [<ffff0000080f85a8>] process_one_work+0x280/0x790
> [   26.283788] [<ffff0000080f8b00>] worker_thread+0x48/0x450
> [   26.283792] [<ffff000008100430>] kthread+0x138/0x140
> [   26.283796] [<ffff000008083710>] 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 <Liviu.Dudau@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> 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);

This breaks the locking strategy.

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.

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.

-- 
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.