* [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
@ 2013-11-21 1:51 Dave Airlie
2013-11-21 9:22 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Dave Airlie @ 2013-11-21 1:51 UTC (permalink / raw)
To: dri-devel
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 <jbarnes@virtuosugeek.org>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
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;
return 0;
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 1:51 [PATCH] drm/sysfs: fix hotplug regression since lifetime changes Dave Airlie
@ 2013-11-21 9:22 ` Daniel Vetter
2013-11-21 9:25 ` Dave Airlie
2013-11-21 15:24 ` Greg KH
0 siblings, 2 replies; 10+ messages in thread
From: Daniel Vetter @ 2013-11-21 9:22 UTC (permalink / raw)
To: Dave Airlie; +Cc: Greg KH, dri-devel
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 <jbarnes@virtuosugeek.org>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> 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().
Cc'ing Greg for insight.
Aside: I wonder whether we could get rid of our drm_minor type and use it
for something more useful like the render node vs legacy node split ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 9:22 ` Daniel Vetter
@ 2013-11-21 9:25 ` Dave Airlie
2013-11-21 9:49 ` David Herrmann
2013-11-21 10:20 ` Daniel Vetter
2013-11-21 15:24 ` Greg KH
1 sibling, 2 replies; 10+ messages in thread
From: Dave Airlie @ 2013-11-21 9:25 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Greg KH, dri-devel
On Thu, Nov 21, 2013 at 7:22 PM, Daniel Vetter <daniel@ffwll.ch> 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 <jbarnes@virtuosugeek.org>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>> 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.
>
> Cc'ing Greg for insight.
>
> Aside: I wonder whether we could get rid of our drm_minor type and use it
> for something more useful like the render node vs legacy node split ...
Its ABI now. all the userspace drivers need it in the uevents for
hotplug to keep working.
Dave.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 9:25 ` Dave Airlie
@ 2013-11-21 9:49 ` David Herrmann
2013-11-21 9:58 ` Dave Airlie
2013-11-21 10:20 ` Daniel Vetter
1 sibling, 1 reply; 10+ messages in thread
From: David Herrmann @ 2013-11-21 9:49 UTC (permalink / raw)
To: Dave Airlie; +Cc: Greg KH, dri-devel
Hi
On Thu, Nov 21, 2013 at 10:25 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Thu, Nov 21, 2013 at 7:22 PM, Daniel Vetter <daniel@ffwll.ch> 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 <jbarnes@virtuosugeek.org>
>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>> ---
>>> 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.
Thanks
David
>> Cc'ing Greg for insight.
>>
>> Aside: I wonder whether we could get rid of our drm_minor type and use it
>> for something more useful like the render node vs legacy node split ...
>
> Its ABI now. all the userspace drivers need it in the uevents for
> hotplug to keep working.
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 9:49 ` David Herrmann
@ 2013-11-21 9:58 ` Dave Airlie
2013-11-21 10:00 ` David Herrmann
0 siblings, 1 reply; 10+ messages in thread
From: Dave Airlie @ 2013-11-21 9:58 UTC (permalink / raw)
To: David Herrmann; +Cc: Greg KH, dri-devel
On Thu, Nov 21, 2013 at 7:49 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Nov 21, 2013 at 10:25 AM, Dave Airlie <airlied@gmail.com> wrote:
>> On Thu, Nov 21, 2013 at 7:22 PM, Daniel Vetter <daniel@ffwll.ch> 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 <jbarnes@virtuosugeek.org>
>>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>>> ---
>>>> 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.
Dave.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 9:58 ` Dave Airlie
@ 2013-11-21 10:00 ` David Herrmann
0 siblings, 0 replies; 10+ messages in thread
From: David Herrmann @ 2013-11-21 10:00 UTC (permalink / raw)
To: Dave Airlie; +Cc: Greg KH, dri-devel
Hi
On Thu, Nov 21, 2013 at 10:58 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Thu, Nov 21, 2013 at 7:49 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Thu, Nov 21, 2013 at 10:25 AM, Dave Airlie <airlied@gmail.com> wrote:
>>> On Thu, Nov 21, 2013 at 7:22 PM, Daniel Vetter <daniel@ffwll.ch> 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 <jbarnes@virtuosugeek.org>
>>>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>>>> ---
>>>>> 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!):
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;
}
/**
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 9:25 ` Dave Airlie
2013-11-21 9:49 ` David Herrmann
@ 2013-11-21 10:20 ` Daniel Vetter
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2013-11-21 10:20 UTC (permalink / raw)
To: Dave Airlie; +Cc: Greg KH, dri-devel
On Thu, Nov 21, 2013 at 07:25:39PM +1000, Dave Airlie wrote:
> On Thu, Nov 21, 2013 at 7:22 PM, Daniel Vetter <daniel@ffwll.ch> 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 <jbarnes@virtuosugeek.org>
> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
> >> ---
> >> 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.
> >
> > Cc'ing Greg for insight.
> >
> > Aside: I wonder whether we could get rid of our drm_minor type and use it
> > for something more useful like the render node vs legacy node split ...
>
> Its ABI now. all the userspace drivers need it in the uevents for
> hotplug to keep working.
Yeah I know that the drm_minor is abi now on legacy node. More thought of
a drm_render type for the render nodes. Could help with finding them
through udev (e.g. for headless usage in transcode jobs or for opencl).
I think we should waste a few braincycles on this before making
rendernodes official.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 9:22 ` Daniel Vetter
2013-11-21 9:25 ` Dave Airlie
@ 2013-11-21 15:24 ` Greg KH
2013-11-21 15:40 ` David Herrmann
1 sibling, 1 reply; 10+ messages in thread
From: Greg KH @ 2013-11-21 15:24 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
On Thu, Nov 21, 2013 at 10:22:55AM +0100, Daniel Vetter 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 <jbarnes@virtuosugeek.org>
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > ---
> > 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().
>
> Cc'ing Greg for insight.
No, setting the type at this point is fine, I don't see the race, care
to explain what I'm missing?
But, if you want to be "correct" you can just create the device
structure yourself, set the fields (including the type) and then call
device_register() yourself. Look at usb_create_ep_devs() for an
example of how to do this.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 15:24 ` Greg KH
@ 2013-11-21 15:40 ` David Herrmann
2013-11-21 15:48 ` Greg KH
0 siblings, 1 reply; 10+ messages in thread
From: David Herrmann @ 2013-11-21 15:40 UTC (permalink / raw)
To: Greg KH; +Cc: dri-devel@lists.freedesktop.org
Hi
On Thu, Nov 21, 2013 at 4:24 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 21, 2013 at 10:22:55AM +0100, Daniel Vetter 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 <jbarnes@virtuosugeek.org>
>> > Signed-off-by: Dave Airlie <airlied@redhat.com>
>> > ---
>> > 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().
>>
>> Cc'ing Greg for insight.
>
> No, setting the type at this point is fine, I don't see the race, care
> to explain what I'm missing?
>
> But, if you want to be "correct" you can just create the device
> structure yourself, set the fields (including the type) and then call
> device_register() yourself. Look at usb_create_ep_devs() for an
> example of how to do this.
We already pushed a fix:
http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-fixes&id=760c960bd6880cf22a57c0af9ff60c96250aad39
The race is the same as with sysfs attributes. If we set the device
type _after_ device_add(), the initial uevent might not have
DEVTYPE=drm_minor set, thus, slip through any
udev_*_match_subsystem_devtype() filters.
Thanks
David
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/sysfs: fix hotplug regression since lifetime changes
2013-11-21 15:40 ` David Herrmann
@ 2013-11-21 15:48 ` Greg KH
0 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2013-11-21 15:48 UTC (permalink / raw)
To: David Herrmann; +Cc: dri-devel@lists.freedesktop.org
On Thu, Nov 21, 2013 at 04:40:29PM +0100, David Herrmann wrote:
> Hi
>
> On Thu, Nov 21, 2013 at 4:24 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Nov 21, 2013 at 10:22:55AM +0100, Daniel Vetter 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 <jbarnes@virtuosugeek.org>
> >> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> >> > ---
> >> > 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().
> >>
> >> Cc'ing Greg for insight.
> >
> > No, setting the type at this point is fine, I don't see the race, care
> > to explain what I'm missing?
> >
> > But, if you want to be "correct" you can just create the device
> > structure yourself, set the fields (including the type) and then call
> > device_register() yourself. Look at usb_create_ep_devs() for an
> > example of how to do this.
>
> We already pushed a fix:
> http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-fixes&id=760c960bd6880cf22a57c0af9ff60c96250aad39
>
> The race is the same as with sysfs attributes. If we set the device
> type _after_ device_add(), the initial uevent might not have
> DEVTYPE=drm_minor set, thus, slip through any
> udev_*_match_subsystem_devtype() filters.
Ah, that makes sense, nice fix.
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-11-21 15:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-21 1:51 [PATCH] drm/sysfs: fix hotplug regression since lifetime changes Dave Airlie
2013-11-21 9:22 ` Daniel Vetter
2013-11-21 9:25 ` Dave Airlie
2013-11-21 9:49 ` David Herrmann
2013-11-21 9:58 ` Dave Airlie
2013-11-21 10:00 ` David Herrmann
2013-11-21 10:20 ` Daniel Vetter
2013-11-21 15:24 ` Greg KH
2013-11-21 15:40 ` David Herrmann
2013-11-21 15:48 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.