Re: [PATCH 2/3] drm: make DRM_UNLOCKED ioctls with their own mutex

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 26, 2011 at 11:11:14AM -0500, Ilija Hadzic wrote:
> On Wed, 26 Oct 2011, Daniel Vetter wrote:
> >
> >Just to check before I dig into reviewing this: Have you check all the
> >other users of these data structure that these functions touch whether
> >they don't accidentally rely on the global lock being taken?
> 
> I did and thought it was safe. I re-checked this morning prompted by
> your note and of course there is one (easily fixable) thing that I
> missed.
> Here is the full "report":
> 
> drm_getclient is safe: the only thing that should be protected there
> is dev->filelist and that is all protected in other functions using
> struct_mutex.
> 
> drm_getstats is safe: actually I think there is no need for any mutex there
> because the loop runs through quasi-static (set at load time only)
> data, and the actual count access is done with atomic_read()
> 
> drm_getmap has one problem which I can fix (and it's the hardest to
> follow): the structure that should be protected there is the
> dev->maplist. There are three functions that may potentially be an
> issue:
> 
>    drm_getsarea doesn't grab any lock before touching dev->maplist (so
>    global mutex won't help here either). However, drivers seem to call
>    it only at initialization time so it probably doesn't matter
> 
>    drm_master_destroy doesn't grab any lock either, but it is called
>    from drm_master_put which is protected with dev->struct_mutex
>    everywhere else in drm module. I also see a couple of calls
>    to drm_master_destroy from vmwgfx driver that look unlocked
>    to me, but drm_global_mutex won't help here either
> 
>    drm_getsareactx uses struct_mutex, but it apparently releases it
>    too early (that's the problem I missed initially). If it's released
>    after it's done with dev->maplist, we should be fine.
> 
> I'll redo this patch with a fix to drm_getsareactx and that should do it.
> I would still appreciate another pair of eyes to make sure I didn't
> miss anything else

Awesome. Please include this in the patch description, and if the
description gets too complicated, maybe even split up the patches into the
individual cases. Also, when you can convince yourself that drm_getstats
doesn't need the struct_mutex, I think it'd be best to drop it. Locking
that doesn't actually protect anything just confuses, especially here in
drm where the locking isn't too clear to begin with.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48