lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170720113853.GX31807@n2100.armlinux.org.uk>
Date:   Thu, 20 Jul 2017 12:38:53 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Liviu Dudau <Liviu.Dudau@....com>
Cc:     David Airlie <airlied@...ux.ie>,
        DRI devel <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/i2c: tda998x: Fix lockdep warning about possible
 circular dependency

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@....com>
> Cc: Russell King <linux@...linux.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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ