On Thu, Jul 24, 2014 at 12:16:59PM +0200, David Herrmann wrote:
> 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.
Ah, I see. Makes sense. Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> > 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.
/me remembers again ...
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
