drm/sysfs: fix hotplug regression since lifetime changes

Commit Message

David Herrmann Nov. 21, 2013, 10 a.m. UTC

Hi

On Thu, Nov 21, 2013 at 10:58 AM, Dave Airlie <[e-mail]> wrote:
> On Thu, Nov 21, 2013 at 7:49 PM, David Herrmann <[e-mail]> wrote:
>> Hi
>>
>> On Thu, Nov 21, 2013 at 10:25 AM, Dave Airlie <[e-mail]> wrote:
>>> On Thu, Nov 21, 2013 at 7:22 PM, Daniel Vetter <[e-mail]> wrote:
>>>> On Thu, Nov 21, 2013 at 11:51:04AM +1000, Dave Airlie wrote:
>>>>> 5bdebb183c9702a8c57a01dff09337be3de337a6 changed the lifetimes, but it
>>>>> also meant we no longer set the device_type field properly, so the
>>>>> hotplug events in userspace weren't fully formed enough for drivers to care.
>>>>>
>>>>> Reported-by: Jesse Barnes <[e-mail]>
>>>>> Signed-off-by: Dave Airlie <[e-mail]>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_sysfs.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>>>>> index 1a35ea5..c6a3902 100644
>>>>> --- a/drivers/gpu/drm/drm_sysfs.c
>>>>> +++ b/drivers/gpu/drm/drm_sysfs.c
>>>>> @@ -516,6 +516,7 @@ int drm_sysfs_device_add(struct drm_minor *minor)
>>>>>               DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
>>>>>               return PTR_ERR(minor->kdev);
>>>>>       }
>>>>> +     minor->kdev->type = &drm_sysfs_device_minor;
>>>>
>>>> Isn't this one of the sysfs races Greg is fighting against? At least from
>>>> a cursor read through the driver core it looks like we'd better set the
>>>> dev->type before we set it up with device_create().
>>>
>>> Possibly but how can we do that? since minor->kdev is something we
>>> just created 2 lines earlier
>>> unless there is another create function I should be calling, I don't
>>> see a device_create_with_type.
>>
>> See device_create_groups_vargs() in drivers/base/core.c. Just copy the
>> code from it and do device initialization yourself. device_create() is
>> only a wrapper around kzalloc()+device_register() anyway.
>>
>
> It does seem a bit like cut-n-paste magic to me to do that,
>
> It does however seem to be the accepted norm.

This should do it (untested!):

Patch

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 1a35ea5..cb951b2 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -489,6 +489,11 @@  void drm_sysfs_hotplug_event(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_sysfs_hotplug_event);

+static void drm_sysfs_release(struct device *dev)
+{
+ kfree(dev);
+}
+
 /**
  * drm_sysfs_device_add - adds a class device to sysfs for a character driver
  * @dev: DRM device to be added
@@ -501,6 +506,7 @@  EXPORT_SYMBOL(drm_sysfs_hotplug_event);
 int drm_sysfs_device_add(struct drm_minor *minor)
 {
  char *minor_str;
+ int r;

  if (minor->type == DRM_MINOR_CONTROL)
  minor_str = "controlD%d";
@@ -509,14 +515,35 @@  int drm_sysfs_device_add(struct drm_minor *minor)
         else
                 minor_str = "card%d";

- minor->kdev = device_create(drm_class, minor->dev->dev,
-    MKDEV(DRM_MAJOR, minor->index),
-    minor, minor_str, minor->index);
- if (IS_ERR(minor->kdev)) {
- DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
- return PTR_ERR(minor->kdev);
+ minor->kdev = kzalloc(sizeof(*minor->kdev), GFP_KERNEL);
+ if (!minor->dev) {
+ 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->groups = NULL;
+ minor->kdev->release = drm_sysfs_release;
+ dev_set_drvdata(minor->kdev, minor);
+
+ r = dev_set_name(minor->kdev, minor_str, minor->index);
+ if (r < 0)
+ goto error;
+
+ r = device_add(minor->kdev);
+ if (r < 0)
+ goto error;
+
  return 0;
+
+error:
+ DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
+ put_device(minor->kdev);
+ return r;
 }

 /**