[PATCH 06/16] drm: Protect the master management with a drm_device::master_mutex

[David Herrmann]

Hi

On Wed, Mar 26, 2014 at 9:40 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
>>  - Why don't add a spin-lock to "drm_file" instead? Use that one to
>> manage master contexts, but keep "struct_mutex" whenever calling into
>> driver callbacks (set_master/drop_master)
>
> See above. We can't have a lock in the drm_file structure since it
> protects drm_minor data. Also, while it might be possible to restructure
> some code to be able to use spinlocks instead of mutexes I see no reason
> to. The established locking order now is master_mutex -> ttm_lock ->
> struct_mutex which means master_mutex must be a mutex.

Thanks, that order really helps understanding what these locks do.
More, actually, than the commit message ;) It also shows how awful
struct_mutex is.. Using it as data-protection and execution-sync is
really weird. So I'm all for doing more fine-grained locking if it's
as simple as with the master-stuff.

>>  - why is master_mutex per device and not per-minor? I thought masters
>> on minors are _entirely_ independent?How do multiple keysyms
>
> Because currently there is only one master capable minor per device, so
> it would be equivalent. And even if there were more, there is no reason
> to expect any contention and thus a single master_mutex would be fine.

Fair enough.

>>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>>> index e6cdd0f..dad571f 100644
>>> --- a/drivers/gpu/drm/drm_fops.c
>>> +++ b/drivers/gpu/drm/drm_fops.c
>>> @@ -231,12 +231,13 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>>
>>>         /* if there is no current master make this fd it, but do not create
>>>          * any master object for render clients */
>>> -       mutex_lock(&dev->struct_mutex);
>>> +       mutex_lock(&dev->master_mutex);
>>>         if (drm_is_legacy_client(priv) && !priv->minor->master) {
>>>                 /* create a new master */
>>> +               mutex_lock(&dev->struct_mutex);
>>>                 priv->minor->master = drm_master_create(priv->minor);
>>> +               mutex_unlock(&dev->struct_mutex);
>>>                 if (!priv->minor->master) {
>>> -                       mutex_unlock(&dev->struct_mutex);
>>>                         ret = -ENOMEM;
>>>                         goto out_close;
>>>                 }
>>> @@ -245,28 +246,25 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
>>>                 /* take another reference for the copy in the local file priv */
>>>                 priv->master = drm_master_get(priv->minor->master);
>>>
>>> +               mutex_lock(&dev->struct_mutex);
>>>                 priv->authenticated = 1;
>>>
>>>                 mutex_unlock(&dev->struct_mutex);
>> What's that struct_mutex doing here? We're in ->open(), there is
>> no-one racing against us.
>
> Well, it was held at that point before, and the purpose of this patch is
> not to generally clean up struct_mutex usage.

Well, it now looks like this:

mutex_lock(&dev->struct_mutex);
priv->authenticated = 1;
mutex_unlock(&dev->struct_mutex);

Which looks so _wrong_ that I thought we should fix it right away. But
ok, I'm not going to force you to do so.

Quick lock-review:
* drm_authmagic() uses drm_global_mutex to protect setting
priv->authenticated (racing against us..)
* current context is ->open() so no-one has access to "priv". We
haven't even linked it to dev->filelist, but we called into the driver
which might have done anything..
* drm-fops read ->authenticated _unlocked_, so no reason at all to do
an explicit locking around a _single_ write (which is only needed in
very rare cases, anyway)
* it is never set to anything else but 1; a _single_ barrier after
setting it should be enough

In case you don't want to incorporate that, I will send a cleanup.

Would be nice to have the mutex-locking in drm-next to get some
testing. v2 looks good, I haven't done a thorough locking review,
though. But I guess you did, so feel free to include it in your pull.
Thanks
David