From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 11/12] drm: make minor->index available early Date: Thu, 24 Jul 2014 14:30:28 +0200 Message-ID: <20140724123028.GU15237@phenom.ffwll.local> References: <1406129207-1302-1-git-send-email-dh.herrmann@gmail.com> <1406129207-1302-12-git-send-email-dh.herrmann@gmail.com> <20140724100355.GO15237@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f171.google.com (mail-we0-f171.google.com [74.125.82.171]) by gabe.freedesktop.org (Postfix) with ESMTP id 58D326E6F3 for ; Thu, 24 Jul 2014 05:30:19 -0700 (PDT) Received: by mail-we0-f171.google.com with SMTP id p10so2668174wes.30 for ; Thu, 24 Jul 2014 05:30:18 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: David Herrmann Cc: Daniel Vetter , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org 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 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 > >> --- > >> 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 > > > 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