Hi
On Thu, Jul 24, 2014 at 12:03 PM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
