| Message ID | 1384998664-5680-1-git-send-email-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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
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
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
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
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; }
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(+)