[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