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

Tommi Rantala
2015-03-02T20:03:43Z

2015-02-24 1:55 GMT+02:00 Marc Finet <m.dreadlock@gmail.com>:
> On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote:
>> In
>>
>> daniel@phenom:~/linux/src$ git show ccfc08655
>> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
>> Author: Rob Clark <robdclark@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@gmail.com>
>> Cc: Marc Finet <m.dreadlock@gmail.com>
>> Cc: robdclark@gmail.com
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Tested-by: Marc Finet <m.dreadlock@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