[PATCH] drm: Fix deadlock due to getconnector locking changes
2015-02-24 1:55 GMT+02:00 Marc Finet :
> 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
>> 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
>> Cc: Marc Finet
>> Cc: robdclark at gmail.com
>> Signed-off-by: Daniel Vetter
>
> Tested-by: Marc Finet
>
> 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(>mode_config.mutex);
>> - drm_modeset_lock(>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(>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: GW
[ 59.076011] -
[ 59.076011] trinity-c14/2418 is trying to release lock
(crtc_ww_class_mutex) at:
[ 59.076011] [] 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: (>mode_config.mutex){+.+.+.}, at:
[] drm_mode_getconnector+0x83/0x400
[ 59.076011]
[ 59.076011] stack backtrace:
[ 59.076011] CPU: 0 PID: 2418 Comm: trinity-c14 Tainted: GW
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] 81dc2872 88005b777b48 81db6706
[ 59.076011] 88005b719440 88005b777b78 810f8074
001d51c0
[ 59.076011] 880069d3a5d0 88005b719440 81dc2872
88005b777bf8
[ 59.076011] Call Trace:
[ 59.076011] [] ? ww_mutex_unlock+0x42/0xb0
[ 59.076011] [] dump_stack+0x4c/0x65
[ 59.076011] [] print_unlock_imbalance_bug+0xf4/0x100
[ 59.076011] [] ? ww_mutex_unlock+0x42/0xb0
[ 59.076011] [] lock_release_non_nested+0x24e/0x350
[ 59.076011] [] ? mark_held_locks+0x75/0xa0
[ 59.076011] [] ? __mutex_unlock_slowpath+0xad/0x170
[ 59.076011] [] ? ww_mutex_unlock+0x42/0xb0
[ 59.076011] [] lock_release+0xbc/0x380
[ 59.076011] [] __mutex_unlock_slowpath+0x71/0x170
[ 59.076011] [] ? drm_mode_getconnector+0x83/0x400
[ 59.076011] [] ww_mutex_unlock+0x42/0xb0
[ 59.076011] [] drm_modeset_unlock+0x2f/0x40
[ 59.076011] [] drm_mode_getconnector+0x280/0x400
[ 59.076011] [] ? might_fault+0x57/0xb0
[ 59.076011] [] drm_ioctl+0x19c/0x640
[ 59.076011] [] ? trace_hardirqs_on+0xd/0x10
[ 59.076011] [] radeon_drm_ioctl+0x46/0x80
[ 59.076011] [] do_vfs_ioctl+0x318/0x570
[ 59.076011] [] ? selinux_file_ioctl+0x56/0x110
[ 59.076011] [] SyS_ioctl+0x81/0xa0
[ 59.076011] [] 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
[PATCH] drm: Fix deadlock due to getconnector locking changes
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
> 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
> Cc: Marc Finet
> Cc: robdclark at gmail.com
> Signed-off-by: Daniel Vetter
Tested-by: Marc Finet
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(>mode_config.mutex);
> - drm_modeset_lock(>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(>mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
[PATCH] drm: Fix deadlock due to getconnector locking changes
On Sun, 22 Feb 2015, Daniel Vetter wrote:
> In
>
> daniel at phenom:~/linux/src$ git show ccfc08655
copy-paste fail?
J.
> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
> Author: Rob Clark
> 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
> Cc: Marc Finet
> Cc: robdclark at gmail.com
> Signed-off-by: Daniel Vetter
> ---
> 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(>mode_config.mutex);
> - drm_modeset_lock(>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(>mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
[PATCH] drm: Fix deadlock due to getconnector locking changes
On Sun, Feb 22, 2015 at 5:38 AM, Daniel Vetter
wrote:
> In
>
> daniel at phenom:~/linux/src$ git show ccfc08655
> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
> Author: Rob Clark
> 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
> Cc: Marc Finet
> Cc: robdclark at gmail.com
> Signed-off-by: Daniel Vetter
Reviewed-by: Rob Clark
> ---
> 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(>mode_config.mutex);
> - drm_modeset_lock(>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(>mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
[PATCH] drm: Fix deadlock due to getconnector locking changes
In
daniel at phenom:~/linux/src$ git show ccfc08655
commit ccfc08655d5fd5076828f45fb09194c070f2f63a
Author: Rob Clark
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
Cc: Marc Finet
Cc: robdclark at gmail.com
Signed-off-by: Daniel Vetter
---
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(>mode_config.mutex);
- drm_modeset_lock(>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(>mode_config.connection_mutex, NULL);
encoder = drm_connector_get_encoder(connector);
if (encoder)
out_resp->encoder_id = encoder->base.id;
--
2.1.4
