[PATCH] drm: use trylock to avoid fault injection antics

[Rob Clark]

On Fri, Sep 5, 2014 at 8:25 AM, Daniel Vetter <daniel at 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 at 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