drm/i2c: tda998x: Fix lockdep warning about possible circular dependency

Message ID 20170720110450.7435-1-Liviu.Dudau@arm.com
State New
Headers show

Commit Message

Liviu Dudau July 20, 2017, 11:04 a.m.
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(-)

Comments

Daniel Vetter July 20, 2017, 2:57 p.m. | #1
On Thu, Jul 20, 2017 at 4:40 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Thu, Jul 20, 2017 at 03:24:13PM +0100, Russell King - ARM Linux wrote:
>> On Thu, Jul 20, 2017 at 03:19:10PM +0100, Liviu Dudau wrote:
>> > On Thu, Jul 20, 2017 at 02:08:29PM +0100, Russell King - ARM Linux wrote:
>> > > On Thu, Jul 20, 2017 at 01:54:04PM +0100, Liviu Dudau wrote:
>> > > > On Thu, Jul 20, 2017 at 12:44:49PM +0100, Russell King - ARM Linux wrote:
>> > > > > 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?
>> > >
>> > > I don't think so.  I think there's two ways to solve this:
>> > >
>> > > 1. replace the audio_mutex in tda998x_audio_get_eld() and
>> > >    tda998x_connector_fill_modes() with a new mutex (eld_mutex) to
>> > >    protect just the ELD.
>> > >
>> > > 2. remove the mutex from these two functions, and take the connection_mutex
>> > >    modeset lock in tda998x_audio_get_eld().
>> > >
>> > > However, I don't have a view on which would be best.
>> >
>> > If you don't mind, I took the liberty of picking option 2, just because
>> > I don't like adding new locks when existing ones might do the job.
>>
>> I don't mind - but one question for the DRM people in connection with
>> your patch is whether we need the acquire context for this relatively
>> simple lock/copy/unlock sequence.  This path for getting the ELD
>> shouldn't be holding any other DRM locks.
>
> Cc-ing Daniel Vetter in hope of clarifications / nod of approval.
> However, I can only see my emails in the online dri-devel archive, not
> yours, so I can't point him to the whole discussion.
>
> danvet: a while ago while I was debugging the delayed fb setup I found
> a lockdep warning with the tda998x driver. Now I've had some more time
> to investigate so I have created a patch trying to fix the issue, which
> was on v1 just a re-ordering of places where tda998x's audio_mutex lock
> was taken. Russell suggested a different approach, which I have
> implemented in [1], but we wonder if we really have to go through the
> whole dance.

If all you do is take only one ww mutex (wrapped up in
drm_modeset_lock for kms) then you can pass a NULL acquire context.
The context is only needed when you want to take multiple locks at the
same time (to be able to resolve deadlocks). Taking a single lock
within the modeset lock class can't deadlock.

Reading the kerneldoc that's not explained at all :-( Can you pls type
a patch to improve the docs for drm_modeset_lock?

Thanks, Daniel
Liviu Dudau July 20, 2017, 3:06 p.m. | #2
On Thu, Jul 20, 2017 at 04:57:12PM +0200, Daniel Vetter wrote:
> On Thu, Jul 20, 2017 at 4:40 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Thu, Jul 20, 2017 at 03:24:13PM +0100, Russell King - ARM Linux wrote:
> >> On Thu, Jul 20, 2017 at 03:19:10PM +0100, Liviu Dudau wrote:
> >> > On Thu, Jul 20, 2017 at 02:08:29PM +0100, Russell King - ARM Linux wrote:
> >> > > On Thu, Jul 20, 2017 at 01:54:04PM +0100, Liviu Dudau wrote:
> >> > > > On Thu, Jul 20, 2017 at 12:44:49PM +0100, Russell King - ARM Linux wrote:
> >> > > > > 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?
> >> > >
> >> > > I don't think so.  I think there's two ways to solve this:
> >> > >
> >> > > 1. replace the audio_mutex in tda998x_audio_get_eld() and
> >> > >    tda998x_connector_fill_modes() with a new mutex (eld_mutex) to
> >> > >    protect just the ELD.
> >> > >
> >> > > 2. remove the mutex from these two functions, and take the connection_mutex
> >> > >    modeset lock in tda998x_audio_get_eld().
> >> > >
> >> > > However, I don't have a view on which would be best.
> >> >
> >> > If you don't mind, I took the liberty of picking option 2, just because
> >> > I don't like adding new locks when existing ones might do the job.
> >>
> >> I don't mind - but one question for the DRM people in connection with
> >> your patch is whether we need the acquire context for this relatively
> >> simple lock/copy/unlock sequence.  This path for getting the ELD
> >> shouldn't be holding any other DRM locks.
> >
> > Cc-ing Daniel Vetter in hope of clarifications / nod of approval.
> > However, I can only see my emails in the online dri-devel archive, not
> > yours, so I can't point him to the whole discussion.
> >
> > danvet: a while ago while I was debugging the delayed fb setup I found
> > a lockdep warning with the tda998x driver. Now I've had some more time
> > to investigate so I have created a patch trying to fix the issue, which
> > was on v1 just a re-ordering of places where tda998x's audio_mutex lock
> > was taken. Russell suggested a different approach, which I have
> > implemented in [1], but we wonder if we really have to go through the
> > whole dance.
> 
> If all you do is take only one ww mutex (wrapped up in
> drm_modeset_lock for kms) then you can pass a NULL acquire context.
> The context is only needed when you want to take multiple locks at the
> same time (to be able to resolve deadlocks). Taking a single lock
> within the modeset lock class can't deadlock.

Hi Daniel,

Thanks for clarification. I'll post a v3 with a NULL acquire context.

Best regards,
Liviu

> 
> Reading the kerneldoc that's not explained at all :-( Can you pls type
> a patch to improve the docs for drm_modeset_lock?
> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Patch

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);
 	if (connector->edid_blob_ptr) {
 		struct edid *edid = (void *)connector->edid_blob_ptr->data;