| Message ID | 1411588510-3988-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-09-24 16:55 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>: > Paulo Zanoni reported a lockdep splat with a locking inversion between > fpriv->fbs_lock and the modeset locks. This issue was introduced in > > commit f2b50c1161590c3bcdbf3455fe4c575f1c1bd293 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Fri Sep 12 17:07:32 2014 +0200 > > drm: Fixup locking for universal cursor planes > > This here is actually one of the rare cases where lockdep hits a false > positive: The deadlock only happens in drm_fb_release, which cleans up > the file private structure when all the references are gone. So the > locking is the very last one and no one else can deadlock. It also > doesn't protect anything at all, since all ioctls are guaranteed to > have returned at this point - otherwise they'd still hold a reference > on the file. > > So let's just drop it and replace it with a big comment. > > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Paulo Zanoni <przanoni@gmail.com> > Reported-by: Paulo Zanoni <przanoni@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Apparently, it fixes the problem I was seeing while running igt/pm_rpm. It was not 100% reproducible, but it seems to be gone. Smoke-tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Testcase: igt/pm_rpm > --- > drivers/gpu/drm/drm_crtc.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b7021069b078..e79c8d3700d8 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3400,7 +3400,16 @@ void drm_fb_release(struct drm_file *priv) > struct drm_device *dev = priv->minor->dev; > struct drm_framebuffer *fb, *tfb; > > - mutex_lock(&priv->fbs_lock); > + /* > + * When the file gets released that means no one else can access the fb > + * list any more, so no need to grab fpriv->fbs_lock. And we need to to > + * avoid upsetting lockdep since the universal cursor code adds a > + * framebuffer while holding mutex locks. > + * > + * Note that a real deadlock between fpriv->fbs_lock and the modeset > + * locks is impossible here since no one else but this function can get > + * at it any more. > + */ > list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) { > > mutex_lock(&dev->mode_config.fb_lock); > @@ -3413,7 +3422,6 @@ void drm_fb_release(struct drm_file *priv) > /* This will also drop the fpriv->fbs reference. */ > drm_framebuffer_remove(fb); > } > - mutex_unlock(&priv->fbs_lock); > } > > /** > -- > 2.1.0 >
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b7021069b078..e79c8d3700d8 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3400,7 +3400,16 @@ void drm_fb_release(struct drm_file *priv) struct drm_device *dev = priv->minor->dev; struct drm_framebuffer *fb, *tfb; - mutex_lock(&priv->fbs_lock); + /* + * When the file gets released that means no one else can access the fb + * list any more, so no need to grab fpriv->fbs_lock. And we need to to + * avoid upsetting lockdep since the universal cursor code adds a + * framebuffer while holding mutex locks. + * + * Note that a real deadlock between fpriv->fbs_lock and the modeset + * locks is impossible here since no one else but this function can get + * at it any more. + */ list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) { mutex_lock(&dev->mode_config.fb_lock); @@ -3413,7 +3422,6 @@ void drm_fb_release(struct drm_file *priv) /* This will also drop the fpriv->fbs reference. */ drm_framebuffer_remove(fb); } - mutex_unlock(&priv->fbs_lock); } /**
Paulo Zanoni reported a lockdep splat with a locking inversion between fpriv->fbs_lock and the modeset locks. This issue was introduced in commit f2b50c1161590c3bcdbf3455fe4c575f1c1bd293 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Fri Sep 12 17:07:32 2014 +0200 drm: Fixup locking for universal cursor planes This here is actually one of the rare cases where lockdep hits a false positive: The deadlock only happens in drm_fb_release, which cleans up the file private structure when all the references are gone. So the locking is the very last one and no one else can deadlock. It also doesn't protect anything at all, since all ioctls are guaranteed to have returned at this point - otherwise they'd still hold a reference on the file. So let's just drop it and replace it with a big comment. Cc: David Herrmann <dh.herrmann@gmail.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Paulo Zanoni <przanoni@gmail.com> Reported-by: Paulo Zanoni <przanoni@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_crtc.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)