[PATCH] drm: Fix deadlock due to getconnector locking changes

[Tommi Rantala]

2015-02-24 1:55 GMT+02:00 Marc Finet <m.dreadlock at gmail.com>:
> On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote:
>> In
>>
>> daniel at phenom:~/linux/src$ git show ccfc08655
>> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
>> Author: Rob Clark <robdclark at gmail.com>
>> Date:   Thu Dec 18 16:01:48 2014 -0500
>>
>>     drm: tweak getconnector locking
>>
>> We need to extend the locking to cover connector->state reading for
>> atomic drivers, but the above commit was a bit too eager and also
>> included the fill_modes callback. Which on i915 on old platforms using
>> load detection needs to acquire modeset locks, resulting in a deadlock
>> on output probing.
>>
>> Reported-by: Marc Finet <m.dreadlock at gmail.com>
>> Cc: Marc Finet <m.dreadlock at gmail.com>
>> Cc: robdclark at gmail.com
>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>
> Tested-by: Marc Finet <m.dreadlock at gmail.com>
>
> Thanks.
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index b15d720eda4c..ce5f1193ecd6 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -2180,7 +2180,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>       DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>>
>>       mutex_lock(&dev->mode_config.mutex);
>> -     drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>
>>       connector = drm_connector_find(dev, out_resp->connector_id);
>>       if (!connector) {
>> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>       out_resp->mm_height = connector->display_info.height_mm;
>>       out_resp->subpixel = connector->display_info.subpixel_order;
>>       out_resp->connection = connector->status;
>> +
>> +     drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>       encoder = drm_connector_get_encoder(connector);
>>       if (encoder)
>>               out_resp->encoder_id = encoder->base.id;
>> --
>> 2.1.4
>>

Hi,

I think this patch introduced the following.

If the drm_connector_find() call fails, we jump to the 'out' label and
call drm_modeset_unlock(), before the drm_modeset_lock() call.

[   59.076011] =====================================
[   59.076011] [ BUG: bad unlock balance detected! ]
[   59.076011] 4.0.0-rc1+ #60 Tainted: G        W
[   59.076011] -------------------------------------
[   59.076011] trinity-c14/2418 is trying to release lock
(crtc_ww_class_mutex) at:
[   59.076011] [<ffffffff81dc2872>] ww_mutex_unlock+0x42/0xb0
[   59.076011] but there are no more locks to release!
[   59.076011]
[   59.076011] other info that might help us debug this:
[   59.076011] 1 lock held by trinity-c14/2418:
[   59.076011]  #0:  (&dev->mode_config.mutex){+.+.+.}, at:
[<ffffffff815ef253>] drm_mode_getconnector+0x83/0x400
[   59.076011]
[   59.076011] stack backtrace:
[   59.076011] CPU: 0 PID: 2418 Comm: trinity-c14 Tainted: G        W
     4.0.0-rc1+ #60
[   59.076011] Hardware name: Hewlett-Packard HP Compaq dc5750 Small
Form Factor/0A64h, BIOS 786E3 v02.10 01/25/2007
[   59.076011]  ffffffff81dc2872 ffff88005b777b48 ffffffff81db6706
0000000000000000
[   59.076011]  ffff88005b719440 ffff88005b777b78 ffffffff810f8074
00000000001d51c0
[   59.076011]  ffff880069d3a5d0 ffff88005b719440 ffffffff81dc2872
ffff88005b777bf8
[   59.076011] Call Trace:
[   59.076011]  [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0
[   59.076011]  [<ffffffff81db6706>] dump_stack+0x4c/0x65
[   59.076011]  [<ffffffff810f8074>] print_unlock_imbalance_bug+0xf4/0x100
[   59.076011]  [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0
[   59.076011]  [<ffffffff810fc76e>] lock_release_non_nested+0x24e/0x350
[   59.076011]  [<ffffffff810f8eb5>] ? mark_held_locks+0x75/0xa0
[   59.076011]  [<ffffffff81dc275d>] ? __mutex_unlock_slowpath+0xad/0x170
[   59.076011]  [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0
[   59.076011]  [<ffffffff810fcd7c>] lock_release+0xbc/0x380
[   59.076011]  [<ffffffff81dc2721>] __mutex_unlock_slowpath+0x71/0x170
[   59.076011]  [<ffffffff815ef253>] ? drm_mode_getconnector+0x83/0x400
[   59.076011]  [<ffffffff81dc2872>] ww_mutex_unlock+0x42/0xb0
[   59.076011]  [<ffffffff815fc21f>] drm_modeset_unlock+0x2f/0x40
[   59.076011]  [<ffffffff815ef450>] drm_mode_getconnector+0x280/0x400
[   59.076011]  [<ffffffff811cfb07>] ? might_fault+0x57/0xb0
[   59.076011]  [<ffffffff815e1f7c>] drm_ioctl+0x19c/0x640
[   59.076011]  [<ffffffff810f90ad>] ? trace_hardirqs_on+0xd/0x10
[   59.076011]  [<ffffffff8160c066>] radeon_drm_ioctl+0x46/0x80
[   59.076011]  [<ffffffff81211868>] do_vfs_ioctl+0x318/0x570
[   59.076011]  [<ffffffff81462ef6>] ? selinux_file_ioctl+0x56/0x110
[   59.076011]  [<ffffffff81211b41>] SyS_ioctl+0x81/0xa0
[   59.076011]  [<ffffffff81dc6492>] system_call_fastpath+0x12/0x17
[   59.316190] [drm:radeon_gem_pread_ioctl] *ERROR* unimplemented
radeon_gem_pread_ioctl
[   59.579952] [drm:radeon_info_ioctl] *ERROR* copy_to_user
radeon_info_ioctl:555
[watchdog] kernel became tainted! (512/0) Last seed was 1541132817

Tommi