Re: [PATCH 11/12] drm: make minor->index available early

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Thu, Jul 24, 2014 at 12:03 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 23, 2014 at 05:26:46PM +0200, David Herrmann wrote:
>> Instead of allocating the minor-index during registration, we now do this
>> during allocation. This way, debug-messages between minor-allocation and
>> minor-registration will now use the correct minor instead of 0. Same is
>> done for unregistration vs. free, so debug-messages between
>> device-shutdown and device-destruction show proper indices.
>>
>> Even though minor-indices are allocated early, we don't enable minor
>> lookup early. Instead, we keep the entry set to NULL and replace it during
>> registration / unregistration. This way, the index is allocated but lookup
>> only works if registered.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_stub.c | 84 +++++++++++++++++++++++++---------------------
>>  1 file changed, 46 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
>> index b249f14..8b24db5 100644
>> --- a/drivers/gpu/drm/drm_stub.c
>> +++ b/drivers/gpu/drm/drm_stub.c
>> @@ -256,6 +256,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
>>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>>  {
>>       struct drm_minor *minor;
>> +     unsigned long flags;
>> +     int r;
>>
>>       minor = kzalloc(sizeof(*minor), GFP_KERNEL);
>>       if (!minor)
>> @@ -264,57 +266,68 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>>       minor->type = type;
>>       minor->dev = dev;
>>
>> +     idr_preload(GFP_KERNEL);
>> +     spin_lock_irqsave(&drm_minor_lock, flags);
>> +     r = idr_alloc(&drm_minors_idr,
>> +                   NULL,
>> +                   64 * type,
>> +                   64 * (type + 1),
>> +                   GFP_NOWAIT);
>> +     spin_unlock_irqrestore(&drm_minor_lock, flags);
>> +     idr_preload_end();
>> +
>> +     if (r < 0)
>> +             goto err_free;
>> +
>> +     minor->index = r;
>> +
>>       *drm_minor_get_slot(dev, type) = minor;
>>       return 0;
>> +
>> +err_free:
>> +     kfree(minor);
>> +     return r;
>>  }
>>
>>  static void drm_minor_free(struct drm_device *dev, unsigned int type)
>>  {
>> -     struct drm_minor **slot;
>> +     struct drm_minor **slot, *minor;
>> +     unsigned long flags;
>>
>>       slot = drm_minor_get_slot(dev, type);
>> -     if (*slot) {
>> -             drm_mode_group_destroy(&(*slot)->mode_group);
>> -             kfree(*slot);
>> -             *slot = NULL;
>> -     }
>> +     minor = *slot;
>> +     if (!minor)
>> +             return;
>> +
>> +     drm_mode_group_destroy(&minor->mode_group);
>> +
>> +     spin_lock_irqsave(&drm_minor_lock, flags);
>> +     idr_remove(&drm_minors_idr, minor->index);
>> +     spin_unlock_irqrestore(&drm_minor_lock, flags);
>
> I don't understand why you do the idr removal in stages too. Otherwise
> this looks good.

If you call drm_minor_unregister(), we now disable lookup but keep
minor->index. If I also released the ID, a new drm_minor might get
access to it before we call drm_minor_free on the old one. This might
cause misleading debug-messages as both minor objects have the same
index. This is not really a problem, but kinda ugly.

> Aside: If two drivers load concurrently (i.e. in the brave new world
> withou drm_global_mutex) we might end up with interleaved minor ids. Dunno
> whether we'll care since userspace should use udev/sysfs lookups anyway.
> igt sometimes doesn't ;-)

I did post a patch some time ago that makes minor-ID allocations
predictable. I got a NACK from you, so this one is one you ;) But I
agree, we really should fix user-space instead of making random IDs
predictable.

Thanks
David