[PATCH 12/12] drm: make sysfs device always available for minors

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 12:36 PM, Daniel Vetter <daniel at 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 at gmail.com>
>
> Some diff got confused with this one. One comment below, but this is
>
> Reviewed-by: Daniel Vetter <daniel.vetter at 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