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

[Daniel Vetter]

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 at 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 at 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.

Ah, I see. Makes sense. Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> > 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