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

[Thomas Hellstrom <thellstrom@vmware.com>]

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