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