| Message ID | 1409918385-30379-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 05, 2014 at 07:59:45AM -0400, Rob Clark wrote: > While in real life, we could never fail to grab the newly created mutex, > ww_mutex fault injection has no way to know this. Which could result > that kernels built with CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y might fail to > acquire the new crtc lock. Which results in bad things when the locks > are dropped. > > See: https://bugzilla.kernel.org/show_bug.cgi?id=83341 > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/drm_crtc.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 7d7c1fd..8bb11fa 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -682,7 +682,15 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > drm_modeset_lock_all(dev); > drm_modeset_lock_init(&crtc->mutex); > /* dropped by _unlock_all(): */ > - drm_modeset_lock(&crtc->mutex, config->acquire_ctx); > + /* NOTE: use trylock here for the benefit of ww_mutex fault > + * injection. We cannot actually fail to grab this lock (as > + * it has only just been created), but fault injection does > + * not know this, which can result in the this lock failing, > + * and hilarity when we later try to drop the locks. See: > + * https://bugzilla.kernel.org/show_bug.cgi?id=83341 > + */ > + ret = ww_mutex_trylock(&crtc->mutex.mutex); > + WARN_ON(ret); Hm, I've thought on our quick discussion on irc we've agreed that the locking here in the init path is useless anyway and best dropped? Not just remove the crtc locking, but the entire modeset_lock_all. -Daniel > > ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); > if (ret) > -- > 1.9.3 >
On Fri, Sep 5, 2014 at 8:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Sep 05, 2014 at 07:59:45AM -0400, Rob Clark wrote: >> While in real life, we could never fail to grab the newly created mutex, >> ww_mutex fault injection has no way to know this. Which could result >> that kernels built with CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y might fail to >> acquire the new crtc lock. Which results in bad things when the locks >> are dropped. >> >> See: https://bugzilla.kernel.org/show_bug.cgi?id=83341 >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> drivers/gpu/drm/drm_crtc.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 7d7c1fd..8bb11fa 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -682,7 +682,15 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >> drm_modeset_lock_all(dev); >> drm_modeset_lock_init(&crtc->mutex); >> /* dropped by _unlock_all(): */ >> - drm_modeset_lock(&crtc->mutex, config->acquire_ctx); >> + /* NOTE: use trylock here for the benefit of ww_mutex fault >> + * injection. We cannot actually fail to grab this lock (as >> + * it has only just been created), but fault injection does >> + * not know this, which can result in the this lock failing, >> + * and hilarity when we later try to drop the locks. See: >> + * https://bugzilla.kernel.org/show_bug.cgi?id=83341 >> + */ >> + ret = ww_mutex_trylock(&crtc->mutex.mutex); >> + WARN_ON(ret); > > Hm, I've thought on our quick discussion on irc we've agreed that the > locking here in the init path is useless anyway and best dropped? Not just > remove the crtc locking, but the entire modeset_lock_all. well, 0day appears to disagree with you.. I still think we should go the trylock route for 3.17, as it is more the more conservative patch. I'm not against getting rid of that locking (which is in fact overkill) once the other fallout is fixed up. But that seems more like a merge-window thing, so probably best to wait for 3.18. BR, -R > -Daniel > >> >> ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); >> if (ret) >> -- >> 1.9.3 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On 09/07/2014 05:02 PM, Rob Clark wrote: > On Fri, Sep 5, 2014 at 8:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Fri, Sep 05, 2014 at 07:59:45AM -0400, Rob Clark wrote: >>> While in real life, we could never fail to grab the newly created mutex, >>> ww_mutex fault injection has no way to know this. Which could result >>> that kernels built with CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y might fail to >>> acquire the new crtc lock. Which results in bad things when the locks >>> are dropped. >>> >>> See: https://bugzilla.kernel.org/show_bug.cgi?id=83341 >>> >>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>> --- >>> drivers/gpu/drm/drm_crtc.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>> index 7d7c1fd..8bb11fa 100644 >>> --- a/drivers/gpu/drm/drm_crtc.c >>> +++ b/drivers/gpu/drm/drm_crtc.c >>> @@ -682,7 +682,15 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >>> drm_modeset_lock_all(dev); >>> drm_modeset_lock_init(&crtc->mutex); >>> /* dropped by _unlock_all(): */ >>> - drm_modeset_lock(&crtc->mutex, config->acquire_ctx); >>> + /* NOTE: use trylock here for the benefit of ww_mutex fault >>> + * injection. We cannot actually fail to grab this lock (as >>> + * it has only just been created), but fault injection does >>> + * not know this, which can result in the this lock failing, >>> + * and hilarity when we later try to drop the locks. See: >>> + * https://bugzilla.kernel.org/show_bug.cgi?id=83341 >>> + */ >>> + ret = ww_mutex_trylock(&crtc->mutex.mutex); >>> + WARN_ON(ret); >> Hm, I've thought on our quick discussion on irc we've agreed that the >> locking here in the init path is useless anyway and best dropped? Not just >> remove the crtc locking, but the entire modeset_lock_all. > well, 0day appears to disagree with you.. I still think we should go > the trylock route for 3.17, as it is more the more conservative patch. > > I'm not against getting rid of that locking (which is in fact > overkill) once the other fallout is fixed up. But that seems more > like a merge-window thing, so probably best to wait for 3.18. > > BR, > -R > FWIW, Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com> >> -Daniel >> >>> ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); >>> if (ret) >>> -- >>> 1.9.3 >>> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7d7c1fd..8bb11fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -682,7 +682,15 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, drm_modeset_lock_all(dev); drm_modeset_lock_init(&crtc->mutex); /* dropped by _unlock_all(): */ - drm_modeset_lock(&crtc->mutex, config->acquire_ctx); + /* NOTE: use trylock here for the benefit of ww_mutex fault + * injection. We cannot actually fail to grab this lock (as + * it has only just been created), but fault injection does + * not know this, which can result in the this lock failing, + * and hilarity when we later try to drop the locks. See: + * https://bugzilla.kernel.org/show_bug.cgi?id=83341 + */ + ret = ww_mutex_trylock(&crtc->mutex.mutex); + WARN_ON(ret); ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret)
While in real life, we could never fail to grab the newly created mutex, ww_mutex fault injection has no way to know this. Which could result that kernels built with CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y might fail to acquire the new crtc lock. Which results in bad things when the locks are dropped. See: https://bugzilla.kernel.org/show_bug.cgi?id=83341 Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)