From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 12/12] drm: make sysfs device always available for minors Date: Thu, 24 Jul 2014 12:36:06 +0200 Message-ID: <20140724103606.GR15237@phenom.ffwll.local> References: <1406129207-1302-1-git-send-email-dh.herrmann@gmail.com> <1406129207-1302-13-git-send-email-dh.herrmann@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f175.google.com (mail-wi0-f175.google.com [209.85.212.175]) by gabe.freedesktop.org (Postfix) with ESMTP id 039016E0C1 for ; Thu, 24 Jul 2014 03:35:59 -0700 (PDT) Received: by mail-wi0-f175.google.com with SMTP id ho1so9311682wib.8 for ; Thu, 24 Jul 2014 03:35:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1406129207-1302-13-git-send-email-dh.herrmann@gmail.com> 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 Wed, Jul 23, 2014 at 05:26:47PM +0200, David Herrmann wrote: > For each minor we allocate a sysfs device as minor->kdev. Currently, this > is allocated and registered in drm_minor_register(). This makes it > impossible to add sysfs-attributes to the device before it is registered. > Therefore, they are not added atomically, nor can we move device_add() > *after* ->load() is called. > > This patch makes minor->kdev available early, but only adds the device > during minor-registration. > > Signed-off-by: David Herrmann Some diff got confused with this one. One comment below, but this is Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/drm_stub.c | 22 ++++++++--- > drivers/gpu/drm/drm_sysfs.c | 90 +++++++++++++++++++-------------------------- > include/drm/drmP.h | 3 +- > 3 files changed, 54 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > index 8b24db5..09c6bfb 100644 > --- a/drivers/gpu/drm/drm_stub.c > +++ b/drivers/gpu/drm/drm_stub.c > @@ -281,9 +281,19 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > > minor->index = r; > > + minor->kdev = drm_sysfs_minor_alloc(minor); > + if (IS_ERR(minor->kdev)) { > + r = PTR_ERR(minor->kdev); > + goto err_index; > + } > + > *drm_minor_get_slot(dev, type) = minor; > return 0; > > +err_index: > + spin_lock_irqsave(&drm_minor_lock, flags); > + idr_remove(&drm_minors_idr, minor->index); > + spin_unlock_irqrestore(&drm_minor_lock, flags); > err_free: > kfree(minor); > return r; > @@ -300,6 +310,7 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type) > return; > > drm_mode_group_destroy(&minor->mode_group); > + put_device(minor->kdev); > > spin_lock_irqsave(&drm_minor_lock, flags); > idr_remove(&drm_minors_idr, minor->index); > @@ -327,11 +338,9 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) > return ret; > } > > - ret = drm_sysfs_device_add(minor); > - if (ret) { > - DRM_ERROR("DRM: Error sysfs_device_add.\n"); > + ret = device_add(minor->kdev); > + if (ret) > goto err_debugfs; > - } > > /* replace NULL with @minor so lookups will succeed from now on */ > spin_lock_irqsave(&drm_minor_lock, flags); > @@ -352,7 +361,7 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) > unsigned long flags; > > minor = *drm_minor_get_slot(dev, type); > - if (!minor || !minor->kdev) > + if (!minor || !device_is_registered(minor->kdev)) > return; > > /* replace @minor with NULL so lookups will fail from now on */ > @@ -360,8 +369,9 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) > idr_replace(&drm_minors_idr, NULL, minor->index); > spin_unlock_irqrestore(&drm_minor_lock, flags); > > + device_del(minor->kdev); > + dev_set_drvdata(minor->kdev, NULL); /* safety belt */ > drm_debugfs_cleanup(minor); > - drm_sysfs_device_remove(minor); > } > > /** > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index 7827dad..ab1a5f6 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -493,71 +493,55 @@ static void drm_sysfs_release(struct device *dev) > } > > /** > - * drm_sysfs_device_add - adds a class device to sysfs for a character driver > - * @dev: DRM device to be added > - * @head: DRM head in question > + * drm_sysfs_minor_alloc() - Allocate sysfs device for given minor > + * @minor: minor to allocate sysfs device for > * > - * Add a DRM device to the DRM's device model class. We use @dev's PCI device > - * as the parent for the Linux device, and make sure it has a file containing > - * the driver we're using (for userspace compatibility). > + * This allocates a new sysfs device for @minor and returns it. The device is > + * not registered nor linked. The caller has to use device_add() and > + * device_del() to register and unregister it. > + * > + * Note that dev_get_drvdata() on the new device will return the minor. > + * However, the device does not hold a ref-count to the minor nor to the > + * underlying drm_device. This is unproblematic as long as you access the > + * private data only in sysfs callbacks. device_del() disables those > + * synchronously, so they cannot be called after you cleanup a minor. > */ > -int drm_sysfs_device_add(struct drm_minor *minor) > +struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) > { > - char *minor_str; > + const char *minor_str; > + struct device *kdev; > int r; > > if (minor->type == DRM_MINOR_CONTROL) > minor_str = "controlD%d"; > - else if (minor->type == DRM_MINOR_RENDER) > - minor_str = "renderD%d"; > - else > - minor_str = "card%d"; > - > - minor->kdev = kzalloc(sizeof(*minor->kdev), GFP_KERNEL); > - if (!minor->kdev) { > - r = -ENOMEM; > - goto error; > - } > - > - device_initialize(minor->kdev); > - minor->kdev->devt = MKDEV(DRM_MAJOR, minor->index); > - minor->kdev->class = drm_class; > - minor->kdev->type = &drm_sysfs_device_minor; > - minor->kdev->parent = minor->dev->dev; > - minor->kdev->release = drm_sysfs_release; > - dev_set_drvdata(minor->kdev, minor); > - > - r = dev_set_name(minor->kdev, minor_str, minor->index); > + else if (minor->type == DRM_MINOR_RENDER) > + minor_str = "renderD%d"; > + else > + minor_str = "card%d"; > + > + kdev = kzalloc(sizeof(*kdev), GFP_KERNEL); > + if (!kdev) > + return ERR_PTR(-ENOMEM); Not sure about the device model, but could we just embedded the kdev into struct drm_minor? Separate patch ofc. > + > + device_initialize(kdev); > + kdev->devt = MKDEV(DRM_MAJOR, minor->index); > + kdev->class = drm_class; > + kdev->type = &drm_sysfs_device_minor; > + kdev->parent = minor->dev->dev; > + kdev->release = drm_sysfs_release; > + dev_set_drvdata(kdev, minor); > + > + r = dev_set_name(kdev, minor_str, minor->index); > if (r < 0) > - goto error; > - > - r = device_add(minor->kdev); > - if (r < 0) > - goto error; > - > - return 0; > + goto err_free; > > -error: > - DRM_ERROR("device create failed %d\n", r); > - put_device(minor->kdev); > - return r; > -} > + return kdev; > > -/** > - * drm_sysfs_device_remove - remove DRM device > - * @dev: DRM device to remove > - * > - * This call unregisters and cleans up a class device that was created with a > - * call to drm_sysfs_device_add() > - */ > -void drm_sysfs_device_remove(struct drm_minor *minor) > -{ > - if (minor->kdev) > - device_unregister(minor->kdev); > - minor->kdev = NULL; > +err_free: > + put_device(kdev); > + return ERR_PTR(r); > } > > - > /** > * drm_class_device_register - Register a struct device in the drm class. > * > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 17dd67a..d28d337 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1492,9 +1492,8 @@ extern int drm_pci_set_unique(struct drm_device *dev, > struct drm_sysfs_class; > extern struct class *drm_sysfs_create(struct module *owner, char *name); > extern void drm_sysfs_destroy(void); > -extern int drm_sysfs_device_add(struct drm_minor *minor); > +extern struct device *drm_sysfs_minor_alloc(struct drm_minor *minor); > extern void drm_sysfs_hotplug_event(struct drm_device *dev); > -extern void drm_sysfs_device_remove(struct drm_minor *minor); > extern int drm_sysfs_connector_add(struct drm_connector *connector); > extern void drm_sysfs_connector_remove(struct drm_connector *connector); > > -- > 2.0.2 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch