| Message ID | 1406129207-1302-13-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dh.herrmann@gmail.com> Some diff got confused with this one. One comment below, but this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > 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 >
Hi On Thu, Jul 24, 2014 at 12:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > 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 <dh.herrmann@gmail.com> > > Some diff got confused with this one. One comment below, but this is > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> --- >> 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. I wouldn't do that. If we embed "struct device", then we _have_ to use the ref-counting of it (get_device(), put_device()) as all generic devices are ref-counted. I'd really like to avoid that. We make sure no-one uses dev_get_drvdata() on a device except for sysfs attribute handlers. Those handlers are removed synchronously during device_del(). Therefore, we can be sure even if the device lives longer than it's parent "struct drm_minor", no-one accesses the private data. We can thus release the kdev and free the minor early. The kdev will hang around until all refs are dropped, but it's detached from our drm_minor (that's why I do the drm_set_drvdata(kdev, NULL) after device_del() as safety-belt). Thanks David
On Thu, Jul 24, 2014 at 12:48:49PM +0200, David Herrmann wrote: > Hi > > On Thu, Jul 24, 2014 at 12:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > 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 <dh.herrmann@gmail.com> > > > > Some diff got confused with this one. One comment below, but this is > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > >> --- > >> 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. > > I wouldn't do that. If we embed "struct device", then we _have_ to use > the ref-counting of it (get_device(), put_device()) as all generic > devices are ref-counted. I'd really like to avoid that. We make sure > no-one uses dev_get_drvdata() on a device except for sysfs attribute > handlers. Those handlers are removed synchronously during > device_del(). Therefore, we can be sure even if the device lives > longer than it's parent "struct drm_minor", no-one accesses the > private data. We can thus release the kdev and free the minor early. > The kdev will hang around until all refs are dropped, but it's > detached from our drm_minor (that's why I do the drm_set_drvdata(kdev, > NULL) after device_del() as safety-belt). Hm yeah, makes sense. r-b still applies ;-) -Daniel
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); + + 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);
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 <dh.herrmann@gmail.com> --- 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(-)