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
thanks,
-- Ilija