[01/44] drivers/base: Always release devres on device_del

Commit Message

Daniel Vetter April 3, 2020, 1:57 p.m. UTC

In drm we've added nice drm_device (the main gpu driver thing, which
also represents the userspace interfaces and has everything else
dangling off it) init functions using devres, devm_drm_dev_init and
soon devm_drm_dev_alloc (this patch series adds that).

A slight trouble is that drm_device itself holds a reference on the
struct device it's sitting on top (for sysfs links and dmesg debug and
lots of other things), so there's a reference loop. For real drivers
this is broken at remove/unplug time, where all devres resources are
released device_release_driver(), before the final device reference is
dropped. So far so good.

There's 2 exceptions:
- drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
  platform device to make them look more like normal devices to
  userspace. These aren't drivers in the driver model sense, we simple
  create a platform_device and register it.

- drm/i915/selftests, where we create minimal mock devices, and again
  the selftests aren't proper drivers in the driver model sense.

For these two cases the reference loop isn't broken, because devres is
only cleaned up when the last device reference is dropped. But that's
not happening, because the drm_device holds that last struct device
reference.

Thus far this wasn't a problem since the above cases simply
hand-rolled their cleanup code. But I want to convert all drivers over
to the devm_ versions, hence it would be really nice if these
virtual/fake/mock uses-cases could also be managed with devres
cleanup.

I see three possible approaches:

- Clean up devres from device_del (or platform_device_unregister) even
  when no driver is bound. This seems like the simplest solution, but
  also the one with the widest impact, and what this patch implements.
  We might want to include more of the cleanup than just
  devres_release_all, but this is all I need to get my use case going.

- Create a devres group and release that when we unbind. The code in
  virtual drivers gets a bit ugly, since every error case has a
  different cleanup code until we've chained everything
  (platform_device, devres group and then drm_device) together and a
  devres_release_group takes care of everything. Doable, but a bit
  confusing when I typed it out.

- Convert the virtual drivers to real (in the device model sense)
  drivers. Feels like too much boilerplate for not much gain. And
  especially with the mock objects minimal mocking is kinda the goal,
  to limit the amount of accidentally pulled in code into our unit
  tests as much as possible.

Either way I think time to discuss this a bit and figure out what's
the recommended approach here.

Signed-off-by: Daniel Vetter <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: "Rafael J. Wysocki" <[e-mail]>
---
 drivers/base/dd.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Greg KH April 3, 2020, 2:17 p.m. UTC | #1

On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> In drm we've added nice drm_device (the main gpu driver thing, which
> also represents the userspace interfaces and has everything else
> dangling off it) init functions using devres, devm_drm_dev_init and
> soon devm_drm_dev_alloc (this patch series adds that).
> 
> A slight trouble is that drm_device itself holds a reference on the
> struct device it's sitting on top (for sysfs links and dmesg debug and
> lots of other things), so there's a reference loop. For real drivers
> this is broken at remove/unplug time, where all devres resources are
> released device_release_driver(), before the final device reference is
> dropped. So far so good.
> 
> There's 2 exceptions:
> - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
>   platform device to make them look more like normal devices to
>   userspace. These aren't drivers in the driver model sense, we simple
>   create a platform_device and register it.

That's a horrid abuse of platform devices, just use a "virtual" device
please, create/remove it when you need it, and all should be fine.

> - drm/i915/selftests, where we create minimal mock devices, and again
>   the selftests aren't proper drivers in the driver model sense.

Again, virtual devices are best to use for this.

> For these two cases the reference loop isn't broken, because devres is
> only cleaned up when the last device reference is dropped. But that's
> not happening, because the drm_device holds that last struct device
> reference.
> 
> Thus far this wasn't a problem since the above cases simply
> hand-rolled their cleanup code. But I want to convert all drivers over
> to the devm_ versions, hence it would be really nice if these
> virtual/fake/mock uses-cases could also be managed with devres
> cleanup.
> 
> I see three possible approaches:
> 
> - Clean up devres from device_del (or platform_device_unregister) even
>   when no driver is bound. This seems like the simplest solution, but
>   also the one with the widest impact, and what this patch implements.
>   We might want to include more of the cleanup than just
>   devres_release_all, but this is all I need to get my use case going.

After device_del, you should never be using that structure again anyway.
So why is there any "resource leak"?  You can't recycle the structure,
and you can't assign it to anything else, so eventually you have to do
a final put on the thing, which will free up the resources.

And then all should be fine, right?  But, by putting the freeing here,
you can still have a "live" device that thinks it has resources availble
that it can access, but yet they are now gone.  Yeah, it's probably not
ever going to really happen, but the lifecycles of dynamic devices are
tough to "prove" at times, and I worry that freeing things this early is
going to cause odd disconnect issues.

thanks,

greg k-h

Daniel Vetter April 3, 2020, 2:47 p.m. UTC | #2

On Fri, Apr 3, 2020 at 4:17 PM Greg Kroah-Hartman
<[e-mail]> wrote:
>
> On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> > In drm we've added nice drm_device (the main gpu driver thing, which
> > also represents the userspace interfaces and has everything else
> > dangling off it) init functions using devres, devm_drm_dev_init and
> > soon devm_drm_dev_alloc (this patch series adds that).
> >
> > A slight trouble is that drm_device itself holds a reference on the
> > struct device it's sitting on top (for sysfs links and dmesg debug and
> > lots of other things), so there's a reference loop. For real drivers
> > this is broken at remove/unplug time, where all devres resources are
> > released device_release_driver(), before the final device reference is
> > dropped. So far so good.
> >
> > There's 2 exceptions:
> > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> >   platform device to make them look more like normal devices to
> >   userspace. These aren't drivers in the driver model sense, we simple
> >   create a platform_device and register it.
>
> That's a horrid abuse of platform devices, just use a "virtual" device
> please, create/remove it when you need it, and all should be fine.
>
> > - drm/i915/selftests, where we create minimal mock devices, and again
> >   the selftests aren't proper drivers in the driver model sense.
>
> Again, virtual devices are best to use for this.

Hm yeah, I guess we should fix that. i915 selftests do use raw struct
device though, and it's not really the problem.

> > For these two cases the reference loop isn't broken, because devres is
> > only cleaned up when the last device reference is dropped. But that's
> > not happening, because the drm_device holds that last struct device
> > reference.
> >
> > Thus far this wasn't a problem since the above cases simply
> > hand-rolled their cleanup code. But I want to convert all drivers over
> > to the devm_ versions, hence it would be really nice if these
> > virtual/fake/mock uses-cases could also be managed with devres
> > cleanup.
> >
> > I see three possible approaches:
> >
> > - Clean up devres from device_del (or platform_device_unregister) even
> >   when no driver is bound. This seems like the simplest solution, but
> >   also the one with the widest impact, and what this patch implements.
> >   We might want to include more of the cleanup than just
> >   devres_release_all, but this is all I need to get my use case going.
>
> After device_del, you should never be using that structure again anyway.
> So why is there any "resource leak"?  You can't recycle the structure,
> and you can't assign it to anything else, so eventually you have to do
> a final put on the thing, which will free up the resources.

I guess I should have spent more time explaining this. There's two
references involved:

- drm_device->dev points at the underlying struct device. The
drm_device holds a reference until it's fully cleaned up, so that we
can do nice stuff like use dev_ versions of printk functions, and you
always know that there's not going to be a use-after free.

- now the other dependency is that as long as the device exists (not
just in memory, but in the device model, i.e. between device_add() and
device_del()) the drm_device should exist. So what we do in the
bus-specific remove/disconnect callback is that we call
drm_dev_unregister(). This drops the drm_device refcount that the drm
chardev was holding, to make sure that an open() on the chardev can
actually get at the memory without going boom. Then after the
drm_dev_unregister, again in the remove/disconnect callback of th
driver, there's a drm_dev_put(). Which might or might not be the final
drm_dev_put(), since if there's currently some open fd we keep the
refcount elevated, to avoid oopses and fun stuff like that. And
drm_device pointers get shared very widely, thanks to fun stuff like
dma_buf buffer sharing and dma_fence hw syncpt sharing across
processes and drivers.

Once the final drm_dev_put() is called we also end up calling
put_device() and everything is happy.

So far so good.

Now the problem is that refcount is hard, and most drm drivers get it
wrong in some fashion or another, so I'm trying to solve all this with
more magic.

Since all drivers need to have a drm_dev_put() at the end of their
driver's remove/disconnect callback we've added a devm_drm_dev_init
function which registers a devres action to do that drm_dev_put() at
device_del time (which might or might not be the final drm_dev_put()).
Nothing has changed thus far.

Now this works really well because when you have a real driver model
driver attached, then device_del ends up calling devres_release_all(),
which ends up triggering the multi-stage cleanup of drm_devices. But
if you do _not_ have a real driver attached, then device_del does
nothing wrt devres cleanup. Instead this is delayed until the final
put_device().

Unfortunately that final put_device() will never happen, because
drm_device is still holding a reference to the struct device. And the
final drm_dev_put of that drm_device will never happen, because that
drm_dev_put call is in a devres actions, which wont ever get called.

This is the only case where this reference loop happens and doesn't
get broken. By calling devres_release_all at device_del time,
irrespective of whether a driver is bound or not, we make both cases
work the same. And at both cases the devres cleanup happens device_del
time, and not when the final put_device is called.

Aside: The final put_device has another devres_release_all() call,
which given your explanation sounds very wrong - at that point the
physical device is long gone, and cleaning up devres actions at that
point is way too late. I think a good cleanup patch on top of this
would be to remove that call, and replace it with an assert that no
one managed to sneak in a devres_add_action between device_del and the
final put_device().

> And then all should be fine, right?  But, by putting the freeing here,
> you can still have a "live" device that thinks it has resources availble
> that it can access, but yet they are now gone.  Yeah, it's probably not
> ever going to really happen, but the lifecycles of dynamic devices are
> tough to "prove" at times, and I worry that freeing things this early is
> going to cause odd disconnect issues.

Not exactly sure what you mean here, but trying to fix all the driver
bugs we have in drm is why I'm doing this. We have a massive amount of
gaps still, but we're slowly closing them all off with stuff like
drm_dev_enter/exit, to make sure there's no races possible between
driver hotunplug and concurrent access by userspace.

The additional trouble is that users are really pissed when we crash
their compositor just because they unplugged an usb display dongle or
an usb projector thing. So the failure mode we're aiming for in drm
for hotunplug is to be very graceful to userspace - experience says
that userspace is even less likely to handle this correctly than the
kernel. That's why we're refcounting drm_device and everything hanging
off it, so that it sticks around and we can pretend to userspace that
it's all still there (but disconnected from hw and the driver). Until
userspace has gone around and managed to process the udev events and
closed all open fds.

Cheers, Daniel

Daniel Vetter April 3, 2020, 2:51 p.m. UTC | #3

On Fri, Apr 3, 2020 at 4:47 PM Daniel Vetter <[e-mail]> wrote:
>
> On Fri, Apr 3, 2020 at 4:17 PM Greg Kroah-Hartman
> <[e-mail]> wrote:
> >
> > On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > also represents the userspace interfaces and has everything else
> > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > soon devm_drm_dev_alloc (this patch series adds that).
> > >
> > > A slight trouble is that drm_device itself holds a reference on the
> > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > lots of other things), so there's a reference loop. For real drivers
> > > this is broken at remove/unplug time, where all devres resources are
> > > released device_release_driver(), before the final device reference is
> > > dropped. So far so good.
> > >
> > > There's 2 exceptions:
> > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > >   platform device to make them look more like normal devices to
> > >   userspace. These aren't drivers in the driver model sense, we simple
> > >   create a platform_device and register it.
> >
> > That's a horrid abuse of platform devices, just use a "virtual" device
> > please, create/remove it when you need it, and all should be fine.
> >
> > > - drm/i915/selftests, where we create minimal mock devices, and again
> > >   the selftests aren't proper drivers in the driver model sense.
> >
> > Again, virtual devices are best to use for this.
>
> Hm yeah, I guess we should fix that. i915 selftests do use raw struct
> device though, and it's not really the problem.
>
> > > For these two cases the reference loop isn't broken, because devres is
> > > only cleaned up when the last device reference is dropped. But that's
> > > not happening, because the drm_device holds that last struct device
> > > reference.
> > >
> > > Thus far this wasn't a problem since the above cases simply
> > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > to the devm_ versions, hence it would be really nice if these
> > > virtual/fake/mock uses-cases could also be managed with devres
> > > cleanup.
> > >
> > > I see three possible approaches:
> > >
> > > - Clean up devres from device_del (or platform_device_unregister) even
> > >   when no driver is bound. This seems like the simplest solution, but
> > >   also the one with the widest impact, and what this patch implements.
> > >   We might want to include more of the cleanup than just
> > >   devres_release_all, but this is all I need to get my use case going.
> >
> > After device_del, you should never be using that structure again anyway.
> > So why is there any "resource leak"?  You can't recycle the structure,
> > and you can't assign it to anything else, so eventually you have to do
> > a final put on the thing, which will free up the resources.
>
> I guess I should have spent more time explaining this. There's two
> references involved:
>
> - drm_device->dev points at the underlying struct device. The
> drm_device holds a reference until it's fully cleaned up, so that we
> can do nice stuff like use dev_ versions of printk functions, and you
> always know that there's not going to be a use-after free.
>
> - now the other dependency is that as long as the device exists (not
> just in memory, but in the device model, i.e. between device_add() and
> device_del()) the drm_device should exist. So what we do in the
> bus-specific remove/disconnect callback is that we call
> drm_dev_unregister(). This drops the drm_device refcount that the drm
> chardev was holding, to make sure that an open() on the chardev can
> actually get at the memory without going boom. Then after the
> drm_dev_unregister, again in the remove/disconnect callback of th
> driver, there's a drm_dev_put(). Which might or might not be the final
> drm_dev_put(), since if there's currently some open fd we keep the
> refcount elevated, to avoid oopses and fun stuff like that. And
> drm_device pointers get shared very widely, thanks to fun stuff like
> dma_buf buffer sharing and dma_fence hw syncpt sharing across
> processes and drivers.
>
> Once the final drm_dev_put() is called we also end up calling
> put_device() and everything is happy.
>
> So far so good.
>
> Now the problem is that refcount is hard, and most drm drivers get it
> wrong in some fashion or another, so I'm trying to solve all this with
> more magic.
>
> Since all drivers need to have a drm_dev_put() at the end of their
> driver's remove/disconnect callback we've added a devm_drm_dev_init
> function which registers a devres action to do that drm_dev_put() at
> device_del time (which might or might not be the final drm_dev_put()).
> Nothing has changed thus far.
>
> Now this works really well because when you have a real driver model
> driver attached, then device_del ends up calling devres_release_all(),
> which ends up triggering the multi-stage cleanup of drm_devices. But
> if you do _not_ have a real driver attached, then device_del does
> nothing wrt devres cleanup. Instead this is delayed until the final
> put_device().
>
> Unfortunately that final put_device() will never happen, because
> drm_device is still holding a reference to the struct device. And the
> final drm_dev_put of that drm_device will never happen, because that
> drm_dev_put call is in a devres actions, which wont ever get called.
>
> This is the only case where this reference loop happens and doesn't
> get broken. By calling devres_release_all at device_del time,
> irrespective of whether a driver is bound or not, we make both cases
> work the same. And at both cases the devres cleanup happens device_del
> time, and not when the final put_device is called.
>
> Aside: The final put_device has another devres_release_all() call,
> which given your explanation sounds very wrong - at that point the
> physical device is long gone, and cleaning up devres actions at that
> point is way too late. I think a good cleanup patch on top of this
> would be to remove that call, and replace it with an assert that no
> one managed to sneak in a devres_add_action between device_del and the
> final put_device().

I've done a bit more digging, and found this commit:

commit a525a3ddeaca69f405d98442ab3c0746e53168dc
Author: Ming Lei <[e-mail]>
Date:   Wed Jul 25 01:42:29 2012 +0800

    driver core: free devres in device_release

Before this devres_release_all was called at device_del() time,
unconditionally whether a driver was bound or not. Which seems to have
been the intention at least back then. So in a way my patch simply
restores that behaviour for the case where no driver has been bound to
a device structure, but we still have devres resources hanging off it.
-Daniel

>
> > And then all should be fine, right?  But, by putting the freeing here,
> > you can still have a "live" device that thinks it has resources availble
> > that it can access, but yet they are now gone.  Yeah, it's probably not
> > ever going to really happen, but the lifecycles of dynamic devices are
> > tough to "prove" at times, and I worry that freeing things this early is
> > going to cause odd disconnect issues.
>
> Not exactly sure what you mean here, but trying to fix all the driver
> bugs we have in drm is why I'm doing this. We have a massive amount of
> gaps still, but we're slowly closing them all off with stuff like
> drm_dev_enter/exit, to make sure there's no races possible between
> driver hotunplug and concurrent access by userspace.
>
> The additional trouble is that users are really pissed when we crash
> their compositor just because they unplugged an usb display dongle or
> an usb projector thing. So the failure mode we're aiming for in drm
> for hotunplug is to be very graceful to userspace - experience says
> that userspace is even less likely to handle this correctly than the
> kernel. That's why we're refcounting drm_device and everything hanging
> off it, so that it sticks around and we can pretend to userspace that
> it's all still there (but disconnected from hw and the driver). Until
> userspace has gone around and managed to process the udev events and
> closed all open fds.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Greg KH April 3, 2020, 3:01 p.m. UTC | #4

On Fri, Apr 03, 2020 at 04:51:33PM +0200, Daniel Vetter wrote:
> On Fri, Apr 3, 2020 at 4:47 PM Daniel Vetter <[e-mail]> wrote:
> >
> > On Fri, Apr 3, 2020 at 4:17 PM Greg Kroah-Hartman
> > <[e-mail]> wrote:
> > >
> > > On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> > > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > > also represents the userspace interfaces and has everything else
> > > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > > soon devm_drm_dev_alloc (this patch series adds that).
> > > >
> > > > A slight trouble is that drm_device itself holds a reference on the
> > > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > > lots of other things), so there's a reference loop. For real drivers
> > > > this is broken at remove/unplug time, where all devres resources are
> > > > released device_release_driver(), before the final device reference is
> > > > dropped. So far so good.
> > > >
> > > > There's 2 exceptions:
> > > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > > >   platform device to make them look more like normal devices to
> > > >   userspace. These aren't drivers in the driver model sense, we simple
> > > >   create a platform_device and register it.
> > >
> > > That's a horrid abuse of platform devices, just use a "virtual" device
> > > please, create/remove it when you need it, and all should be fine.
> > >
> > > > - drm/i915/selftests, where we create minimal mock devices, and again
> > > >   the selftests aren't proper drivers in the driver model sense.
> > >
> > > Again, virtual devices are best to use for this.
> >
> > Hm yeah, I guess we should fix that. i915 selftests do use raw struct
> > device though, and it's not really the problem.
> >
> > > > For these two cases the reference loop isn't broken, because devres is
> > > > only cleaned up when the last device reference is dropped. But that's
> > > > not happening, because the drm_device holds that last struct device
> > > > reference.
> > > >
> > > > Thus far this wasn't a problem since the above cases simply
> > > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > > to the devm_ versions, hence it would be really nice if these
> > > > virtual/fake/mock uses-cases could also be managed with devres
> > > > cleanup.
> > > >
> > > > I see three possible approaches:
> > > >
> > > > - Clean up devres from device_del (or platform_device_unregister) even
> > > >   when no driver is bound. This seems like the simplest solution, but
> > > >   also the one with the widest impact, and what this patch implements.
> > > >   We might want to include more of the cleanup than just
> > > >   devres_release_all, but this is all I need to get my use case going.
> > >
> > > After device_del, you should never be using that structure again anyway.
> > > So why is there any "resource leak"?  You can't recycle the structure,
> > > and you can't assign it to anything else, so eventually you have to do
> > > a final put on the thing, which will free up the resources.
> >
> > I guess I should have spent more time explaining this. There's two
> > references involved:
> >
> > - drm_device->dev points at the underlying struct device. The
> > drm_device holds a reference until it's fully cleaned up, so that we
> > can do nice stuff like use dev_ versions of printk functions, and you
> > always know that there's not going to be a use-after free.
> >
> > - now the other dependency is that as long as the device exists (not
> > just in memory, but in the device model, i.e. between device_add() and
> > device_del()) the drm_device should exist. So what we do in the
> > bus-specific remove/disconnect callback is that we call
> > drm_dev_unregister(). This drops the drm_device refcount that the drm
> > chardev was holding, to make sure that an open() on the chardev can
> > actually get at the memory without going boom. Then after the
> > drm_dev_unregister, again in the remove/disconnect callback of th
> > driver, there's a drm_dev_put(). Which might or might not be the final
> > drm_dev_put(), since if there's currently some open fd we keep the
> > refcount elevated, to avoid oopses and fun stuff like that. And
> > drm_device pointers get shared very widely, thanks to fun stuff like
> > dma_buf buffer sharing and dma_fence hw syncpt sharing across
> > processes and drivers.
> >
> > Once the final drm_dev_put() is called we also end up calling
> > put_device() and everything is happy.
> >
> > So far so good.
> >
> > Now the problem is that refcount is hard, and most drm drivers get it
> > wrong in some fashion or another, so I'm trying to solve all this with
> > more magic.
> >
> > Since all drivers need to have a drm_dev_put() at the end of their
> > driver's remove/disconnect callback we've added a devm_drm_dev_init
> > function which registers a devres action to do that drm_dev_put() at
> > device_del time (which might or might not be the final drm_dev_put()).
> > Nothing has changed thus far.
> >
> > Now this works really well because when you have a real driver model
> > driver attached, then device_del ends up calling devres_release_all(),
> > which ends up triggering the multi-stage cleanup of drm_devices. But
> > if you do _not_ have a real driver attached, then device_del does
> > nothing wrt devres cleanup. Instead this is delayed until the final
> > put_device().
> >
> > Unfortunately that final put_device() will never happen, because
> > drm_device is still holding a reference to the struct device. And the
> > final drm_dev_put of that drm_device will never happen, because that
> > drm_dev_put call is in a devres actions, which wont ever get called.
> >
> > This is the only case where this reference loop happens and doesn't
> > get broken. By calling devres_release_all at device_del time,
> > irrespective of whether a driver is bound or not, we make both cases
> > work the same. And at both cases the devres cleanup happens device_del
> > time, and not when the final put_device is called.
> >
> > Aside: The final put_device has another devres_release_all() call,
> > which given your explanation sounds very wrong - at that point the
> > physical device is long gone, and cleaning up devres actions at that
> > point is way too late. I think a good cleanup patch on top of this
> > would be to remove that call, and replace it with an assert that no
> > one managed to sneak in a devres_add_action between device_del and the
> > final put_device().
> 
> I've done a bit more digging, and found this commit:
> 
> commit a525a3ddeaca69f405d98442ab3c0746e53168dc
> Author: Ming Lei <[e-mail]>
> Date:   Wed Jul 25 01:42:29 2012 +0800
> 
>     driver core: free devres in device_release
> 
> Before this devres_release_all was called at device_del() time,
> unconditionally whether a driver was bound or not. Which seems to have
> been the intention at least back then. So in a way my patch simply
> restores that behaviour for the case where no driver has been bound to
> a device structure, but we still have devres resources hanging off it.

Hey, I was right, I guessed well :)

I'll respond to the parent email, but it looks like you can't do this,
sorry.

greg k-h

Greg KH April 3, 2020, 3:06 p.m. UTC | #5

On Fri, Apr 03, 2020 at 04:47:04PM +0200, Daniel Vetter wrote:
> On Fri, Apr 3, 2020 at 4:17 PM Greg Kroah-Hartman
> <[e-mail]> wrote:
> >
> > On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > also represents the userspace interfaces and has everything else
> > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > soon devm_drm_dev_alloc (this patch series adds that).
> > >
> > > A slight trouble is that drm_device itself holds a reference on the
> > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > lots of other things), so there's a reference loop. For real drivers
> > > this is broken at remove/unplug time, where all devres resources are
> > > released device_release_driver(), before the final device reference is
> > > dropped. So far so good.
> > >
> > > There's 2 exceptions:
> > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > >   platform device to make them look more like normal devices to
> > >   userspace. These aren't drivers in the driver model sense, we simple
> > >   create a platform_device and register it.
> >
> > That's a horrid abuse of platform devices, just use a "virtual" device
> > please, create/remove it when you need it, and all should be fine.
> >
> > > - drm/i915/selftests, where we create minimal mock devices, and again
> > >   the selftests aren't proper drivers in the driver model sense.
> >
> > Again, virtual devices are best to use for this.
> 
> Hm yeah, I guess we should fix that. i915 selftests do use raw struct
> device though, and it's not really the problem.
> 
> > > For these two cases the reference loop isn't broken, because devres is
> > > only cleaned up when the last device reference is dropped. But that's
> > > not happening, because the drm_device holds that last struct device
> > > reference.
> > >
> > > Thus far this wasn't a problem since the above cases simply
> > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > to the devm_ versions, hence it would be really nice if these
> > > virtual/fake/mock uses-cases could also be managed with devres
> > > cleanup.
> > >
> > > I see three possible approaches:
> > >
> > > - Clean up devres from device_del (or platform_device_unregister) even
> > >   when no driver is bound. This seems like the simplest solution, but
> > >   also the one with the widest impact, and what this patch implements.
> > >   We might want to include more of the cleanup than just
> > >   devres_release_all, but this is all I need to get my use case going.
> >
> > After device_del, you should never be using that structure again anyway.
> > So why is there any "resource leak"?  You can't recycle the structure,
> > and you can't assign it to anything else, so eventually you have to do
> > a final put on the thing, which will free up the resources.
> 
> I guess I should have spent more time explaining this. There's two
> references involved:
> 
> - drm_device->dev points at the underlying struct device. The
> drm_device holds a reference until it's fully cleaned up, so that we
> can do nice stuff like use dev_ versions of printk functions, and you
> always know that there's not going to be a use-after free.
> 
> - now the other dependency is that as long as the device exists (not
> just in memory, but in the device model, i.e. between device_add() and
> device_del()) the drm_device should exist. So what we do in the
> bus-specific remove/disconnect callback is that we call
> drm_dev_unregister(). This drops the drm_device refcount that the drm
> chardev was holding, to make sure that an open() on the chardev can
> actually get at the memory without going boom. Then after the
> drm_dev_unregister, again in the remove/disconnect callback of th
> driver, there's a drm_dev_put(). Which might or might not be the final
> drm_dev_put(), since if there's currently some open fd we keep the
> refcount elevated, to avoid oopses and fun stuff like that. And
> drm_device pointers get shared very widely, thanks to fun stuff like
> dma_buf buffer sharing and dma_fence hw syncpt sharing across
> processes and drivers.
> 
> Once the final drm_dev_put() is called we also end up calling
> put_device() and everything is happy.
> 
> So far so good.
> 
> Now the problem is that refcount is hard, and most drm drivers get it
> wrong in some fashion or another, so I'm trying to solve all this with
> more magic.

Wait, no.  Fix the drivers.  Seriously, don't try to "bust" the
reference count logic here.

> Since all drivers need to have a drm_dev_put() at the end of their
> driver's remove/disconnect callback we've added a devm_drm_dev_init
> function which registers a devres action to do that drm_dev_put() at
> device_del time (which might or might not be the final drm_dev_put()).
> Nothing has changed thus far.
> 
> Now this works really well because when you have a real driver model
> driver attached, then device_del ends up calling devres_release_all(),
> which ends up triggering the multi-stage cleanup of drm_devices. But
> if you do _not_ have a real driver attached, then device_del does
> nothing wrt devres cleanup. Instead this is delayed until the final
> put_device().
> 
> Unfortunately that final put_device() will never happen, because
> drm_device is still holding a reference to the struct device. And the
> final drm_dev_put of that drm_device will never happen, because that
> drm_dev_put call is in a devres actions, which wont ever get called.

True, so fix the logic to do the final put_device() :)

> This is the only case where this reference loop happens and doesn't
> get broken. By calling devres_release_all at device_del time,
> irrespective of whether a driver is bound or not, we make both cases
> work the same. And at both cases the devres cleanup happens device_del
> time, and not when the final put_device is called.

So what is so odd about these random drivers that the normal process
does not work for them?

Along these lines, you might look into how the v4l developers finally
fixed this as they had much the same type of issues that you do with
regards to reference counting and resources and userspace file handles.

> Aside: The final put_device has another devres_release_all() call,
> which given your explanation sounds very wrong - at that point the
> physical device is long gone, and cleaning up devres actions at that
> point is way too late. I think a good cleanup patch on top of this
> would be to remove that call, and replace it with an assert that no
> one managed to sneak in a devres_add_action between device_del and the
> final put_device().

The physical device might be gone, but an open handle to the device
might still be around, keeping the structure in place and the driver
thinking it still has access to it's memory structures.

> > And then all should be fine, right?  But, by putting the freeing here,
> > you can still have a "live" device that thinks it has resources availble
> > that it can access, but yet they are now gone.  Yeah, it's probably not
> > ever going to really happen, but the lifecycles of dynamic devices are
> > tough to "prove" at times, and I worry that freeing things this early is
> > going to cause odd disconnect issues.
> 
> Not exactly sure what you mean here, but trying to fix all the driver
> bugs we have in drm is why I'm doing this. We have a massive amount of
> gaps still, but we're slowly closing them all off with stuff like
> drm_dev_enter/exit, to make sure there's no races possible between
> driver hotunplug and concurrent access by userspace.

Again, look at what v4l did, I think it might work out for you all as
well.

thanks,

greg k-h

Daniel Vetter April 3, 2020, 3:15 p.m. UTC | #6

On Fri, Apr 3, 2020 at 5:06 PM Greg Kroah-Hartman
<[e-mail]> wrote:
>
> On Fri, Apr 03, 2020 at 04:47:04PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 3, 2020 at 4:17 PM Greg Kroah-Hartman
> > <[e-mail]> wrote:
> > >
> > > On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> > > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > > also represents the userspace interfaces and has everything else
> > > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > > soon devm_drm_dev_alloc (this patch series adds that).
> > > >
> > > > A slight trouble is that drm_device itself holds a reference on the
> > > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > > lots of other things), so there's a reference loop. For real drivers
> > > > this is broken at remove/unplug time, where all devres resources are
> > > > released device_release_driver(), before the final device reference is
> > > > dropped. So far so good.
> > > >
> > > > There's 2 exceptions:
> > > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > > >   platform device to make them look more like normal devices to
> > > >   userspace. These aren't drivers in the driver model sense, we simple
> > > >   create a platform_device and register it.
> > >
> > > That's a horrid abuse of platform devices, just use a "virtual" device
> > > please, create/remove it when you need it, and all should be fine.
> > >
> > > > - drm/i915/selftests, where we create minimal mock devices, and again
> > > >   the selftests aren't proper drivers in the driver model sense.
> > >
> > > Again, virtual devices are best to use for this.
> >
> > Hm yeah, I guess we should fix that. i915 selftests do use raw struct
> > device though, and it's not really the problem.
> >
> > > > For these two cases the reference loop isn't broken, because devres is
> > > > only cleaned up when the last device reference is dropped. But that's
> > > > not happening, because the drm_device holds that last struct device
> > > > reference.
> > > >
> > > > Thus far this wasn't a problem since the above cases simply
> > > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > > to the devm_ versions, hence it would be really nice if these
> > > > virtual/fake/mock uses-cases could also be managed with devres
> > > > cleanup.
> > > >
> > > > I see three possible approaches:
> > > >
> > > > - Clean up devres from device_del (or platform_device_unregister) even
> > > >   when no driver is bound. This seems like the simplest solution, but
> > > >   also the one with the widest impact, and what this patch implements.
> > > >   We might want to include more of the cleanup than just
> > > >   devres_release_all, but this is all I need to get my use case going.
> > >
> > > After device_del, you should never be using that structure again anyway.
> > > So why is there any "resource leak"?  You can't recycle the structure,
> > > and you can't assign it to anything else, so eventually you have to do
> > > a final put on the thing, which will free up the resources.
> >
> > I guess I should have spent more time explaining this. There's two
> > references involved:
> >
> > - drm_device->dev points at the underlying struct device. The
> > drm_device holds a reference until it's fully cleaned up, so that we
> > can do nice stuff like use dev_ versions of printk functions, and you
> > always know that there's not going to be a use-after free.
> >
> > - now the other dependency is that as long as the device exists (not
> > just in memory, but in the device model, i.e. between device_add() and
> > device_del()) the drm_device should exist. So what we do in the
> > bus-specific remove/disconnect callback is that we call
> > drm_dev_unregister(). This drops the drm_device refcount that the drm
> > chardev was holding, to make sure that an open() on the chardev can
> > actually get at the memory without going boom. Then after the
> > drm_dev_unregister, again in the remove/disconnect callback of th
> > driver, there's a drm_dev_put(). Which might or might not be the final
> > drm_dev_put(), since if there's currently some open fd we keep the
> > refcount elevated, to avoid oopses and fun stuff like that. And
> > drm_device pointers get shared very widely, thanks to fun stuff like
> > dma_buf buffer sharing and dma_fence hw syncpt sharing across
> > processes and drivers.
> >
> > Once the final drm_dev_put() is called we also end up calling
> > put_device() and everything is happy.
> >
> > So far so good.
> >
> > Now the problem is that refcount is hard, and most drm drivers get it
> > wrong in some fashion or another, so I'm trying to solve all this with
> > more magic.
>
> Wait, no.  Fix the drivers.  Seriously, don't try to "bust" the
> reference count logic here.

I guess still not clear. What I'm doing is fixing the drivers. But
because they all get it wrong, I'm trying to hide as much of the
refcounting as possible behind functions which do the right thing.

Which works great for the epic pile of real hw drivers that we have.

The problem I have is is _only_ with those drm drivers which run on
fake hw. For those forcing them to use the exact same functions as the
real hw drivers has some challenges, because they're not really real
drivers. Now the original patch had 3 ideas for how to make things
work for those fake drivers too, with one of them implemented in this
patch (the one that required the least amount of typing on my part).

You harping that I'm getting the refcounting wrong is kinda totally
missing the point, because that's what I'm doing.

> > Since all drivers need to have a drm_dev_put() at the end of their
> > driver's remove/disconnect callback we've added a devm_drm_dev_init
> > function which registers a devres action to do that drm_dev_put() at
> > device_del time (which might or might not be the final drm_dev_put()).
> > Nothing has changed thus far.
> >
> > Now this works really well because when you have a real driver model
> > driver attached, then device_del ends up calling devres_release_all(),
> > which ends up triggering the multi-stage cleanup of drm_devices. But
> > if you do _not_ have a real driver attached, then device_del does
> > nothing wrt devres cleanup. Instead this is delayed until the final
> > put_device().
> >
> > Unfortunately that final put_device() will never happen, because
> > drm_device is still holding a reference to the struct device. And the
> > final drm_dev_put of that drm_device will never happen, because that
> > drm_dev_put call is in a devres actions, which wont ever get called.
>
> True, so fix the logic to do the final put_device() :)
>
> > This is the only case where this reference loop happens and doesn't
> > get broken. By calling devres_release_all at device_del time,
> > irrespective of whether a driver is bound or not, we make both cases
> > work the same. And at both cases the devres cleanup happens device_del
> > time, and not when the final put_device is called.
>
> So what is so odd about these random drivers that the normal process
> does not work for them?

They're not drivers in the driver model sense.

> Along these lines, you might look into how the v4l developers finally
> fixed this as they had much the same type of issues that you do with
> regards to reference counting and resources and userspace file handles.

We have a pile of v4l developers on dri-devel, they've been reviewing
the stuff we've been doing in drm over the past 2-3 years already ...

> > Aside: The final put_device has another devres_release_all() call,
> > which given your explanation sounds very wrong - at that point the
> > physical device is long gone, and cleaning up devres actions at that
> > point is way too late. I think a good cleanup patch on top of this
> > would be to remove that call, and replace it with an assert that no
> > one managed to sneak in a devres_add_action between device_del and the
> > final put_device().
>
> The physical device might be gone, but an open handle to the device
> might still be around, keeping the structure in place and the driver
> thinking it still has access to it's memory structures.

I understand that. This is not the problem I'm having here, we've
fixed that problem a while ago (at least in drm core, but the drivers
have a bit a harder time picking up the correct coding patterns,
mostly because there's overwhelming code in existing drivers that's
just plain broken).

> > > And then all should be fine, right?  But, by putting the freeing here,
> > > you can still have a "live" device that thinks it has resources availble
> > > that it can access, but yet they are now gone.  Yeah, it's probably not
> > > ever going to really happen, but the lifecycles of dynamic devices are
> > > tough to "prove" at times, and I worry that freeing things this early is
> > > going to cause odd disconnect issues.
> >
> > Not exactly sure what you mean here, but trying to fix all the driver
> > bugs we have in drm is why I'm doing this. We have a massive amount of
> > gaps still, but we're slowly closing them all off with stuff like
> > drm_dev_enter/exit, to make sure there's no races possible between
> > driver hotunplug and concurrent access by userspace.
>
> Again, look at what v4l did, I think it might work out for you all as
> well.

I think we're talking past each another quite badly here ...
-Daniel

Daniel Vetter April 3, 2020, 3:33 p.m. UTC | #7

On Fri, Apr 3, 2020 at 5:15 PM Daniel Vetter <[e-mail]> wrote:
>
> On Fri, Apr 3, 2020 at 5:06 PM Greg Kroah-Hartman
> <[e-mail]> wrote:
> >
> > On Fri, Apr 03, 2020 at 04:47:04PM +0200, Daniel Vetter wrote:
> > > On Fri, Apr 3, 2020 at 4:17 PM Greg Kroah-Hartman
> > > <[e-mail]> wrote:
> > > >
> > > > On Fri, Apr 03, 2020 at 03:57:45PM +0200, Daniel Vetter wrote:
> > > > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > > > also represents the userspace interfaces and has everything else
> > > > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > > > soon devm_drm_dev_alloc (this patch series adds that).
> > > > >
> > > > > A slight trouble is that drm_device itself holds a reference on the
> > > > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > > > lots of other things), so there's a reference loop. For real drivers
> > > > > this is broken at remove/unplug time, where all devres resources are
> > > > > released device_release_driver(), before the final device reference is
> > > > > dropped. So far so good.
> > > > >
> > > > > There's 2 exceptions:
> > > > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > > > >   platform device to make them look more like normal devices to
> > > > >   userspace. These aren't drivers in the driver model sense, we simple
> > > > >   create a platform_device and register it.
> > > >
> > > > That's a horrid abuse of platform devices, just use a "virtual" device
> > > > please, create/remove it when you need it, and all should be fine.
> > > >
> > > > > - drm/i915/selftests, where we create minimal mock devices, and again
> > > > >   the selftests aren't proper drivers in the driver model sense.
> > > >
> > > > Again, virtual devices are best to use for this.
> > >
> > > Hm yeah, I guess we should fix that. i915 selftests do use raw struct
> > > device though, and it's not really the problem.
> > >
> > > > > For these two cases the reference loop isn't broken, because devres is
> > > > > only cleaned up when the last device reference is dropped. But that's
> > > > > not happening, because the drm_device holds that last struct device
> > > > > reference.
> > > > >
> > > > > Thus far this wasn't a problem since the above cases simply
> > > > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > > > to the devm_ versions, hence it would be really nice if these
> > > > > virtual/fake/mock uses-cases could also be managed with devres
> > > > > cleanup.
> > > > >
> > > > > I see three possible approaches:
> > > > >
> > > > > - Clean up devres from device_del (or platform_device_unregister) even
> > > > >   when no driver is bound. This seems like the simplest solution, but
> > > > >   also the one with the widest impact, and what this patch implements.
> > > > >   We might want to include more of the cleanup than just
> > > > >   devres_release_all, but this is all I need to get my use case going.
> > > >
> > > > After device_del, you should never be using that structure again anyway.
> > > > So why is there any "resource leak"?  You can't recycle the structure,
> > > > and you can't assign it to anything else, so eventually you have to do
> > > > a final put on the thing, which will free up the resources.
> > >
> > > I guess I should have spent more time explaining this. There's two
> > > references involved:
> > >
> > > - drm_device->dev points at the underlying struct device. The
> > > drm_device holds a reference until it's fully cleaned up, so that we
> > > can do nice stuff like use dev_ versions of printk functions, and you
> > > always know that there's not going to be a use-after free.
> > >
> > > - now the other dependency is that as long as the device exists (not
> > > just in memory, but in the device model, i.e. between device_add() and
> > > device_del()) the drm_device should exist. So what we do in the
> > > bus-specific remove/disconnect callback is that we call
> > > drm_dev_unregister(). This drops the drm_device refcount that the drm
> > > chardev was holding, to make sure that an open() on the chardev can
> > > actually get at the memory without going boom. Then after the
> > > drm_dev_unregister, again in the remove/disconnect callback of th
> > > driver, there's a drm_dev_put(). Which might or might not be the final
> > > drm_dev_put(), since if there's currently some open fd we keep the
> > > refcount elevated, to avoid oopses and fun stuff like that. And
> > > drm_device pointers get shared very widely, thanks to fun stuff like
> > > dma_buf buffer sharing and dma_fence hw syncpt sharing across
> > > processes and drivers.
> > >
> > > Once the final drm_dev_put() is called we also end up calling
> > > put_device() and everything is happy.
> > >
> > > So far so good.
> > >
> > > Now the problem is that refcount is hard, and most drm drivers get it
> > > wrong in some fashion or another, so I'm trying to solve all this with
> > > more magic.
> >
> > Wait, no.  Fix the drivers.  Seriously, don't try to "bust" the
> > reference count logic here.
>
> I guess still not clear. What I'm doing is fixing the drivers. But
> because they all get it wrong, I'm trying to hide as much of the
> refcounting as possible behind functions which do the right thing.
>
> Which works great for the epic pile of real hw drivers that we have.
>
> The problem I have is is _only_ with those drm drivers which run on
> fake hw. For those forcing them to use the exact same functions as the
> real hw drivers has some challenges, because they're not really real
> drivers. Now the original patch had 3 ideas for how to make things
> work for those fake drivers too, with one of them implemented in this
> patch (the one that required the least amount of typing on my part).
>
> You harping that I'm getting the refcounting wrong is kinda totally
> missing the point, because that's what I'm doing.
>
> > > Since all drivers need to have a drm_dev_put() at the end of their
> > > driver's remove/disconnect callback we've added a devm_drm_dev_init
> > > function which registers a devres action to do that drm_dev_put() at
> > > device_del time (which might or might not be the final drm_dev_put()).
> > > Nothing has changed thus far.
> > >
> > > Now this works really well because when you have a real driver model
> > > driver attached, then device_del ends up calling devres_release_all(),
> > > which ends up triggering the multi-stage cleanup of drm_devices. But
> > > if you do _not_ have a real driver attached, then device_del does
> > > nothing wrt devres cleanup. Instead this is delayed until the final
> > > put_device().
> > >
> > > Unfortunately that final put_device() will never happen, because
> > > drm_device is still holding a reference to the struct device. And the
> > > final drm_dev_put of that drm_device will never happen, because that
> > > drm_dev_put call is in a devres actions, which wont ever get called.
> >
> > True, so fix the logic to do the final put_device() :)
> >
> > > This is the only case where this reference loop happens and doesn't
> > > get broken. By calling devres_release_all at device_del time,
> > > irrespective of whether a driver is bound or not, we make both cases
> > > work the same. And at both cases the devres cleanup happens device_del
> > > time, and not when the final put_device is called.
> >
> > So what is so odd about these random drivers that the normal process
> > does not work for them?
>
> They're not drivers in the driver model sense.
>
> > Along these lines, you might look into how the v4l developers finally
> > fixed this as they had much the same type of issues that you do with
> > regards to reference counting and resources and userspace file handles.
>
> We have a pile of v4l developers on dri-devel, they've been reviewing
> the stuff we've been doing in drm over the past 2-3 years already ...
>
> > > Aside: The final put_device has another devres_release_all() call,
> > > which given your explanation sounds very wrong - at that point the
> > > physical device is long gone, and cleaning up devres actions at that
> > > point is way too late. I think a good cleanup patch on top of this
> > > would be to remove that call, and replace it with an assert that no
> > > one managed to sneak in a devres_add_action between device_del and the
> > > final put_device().
> >
> > The physical device might be gone, but an open handle to the device
> > might still be around, keeping the structure in place and the driver
> > thinking it still has access to it's memory structures.
>
> I understand that. This is not the problem I'm having here, we've
> fixed that problem a while ago (at least in drm core, but the drivers
> have a bit a harder time picking up the correct coding patterns,
> mostly because there's overwhelming code in existing drivers that's
> just plain broken).
>
> > > > And then all should be fine, right?  But, by putting the freeing here,
> > > > you can still have a "live" device that thinks it has resources availble
> > > > that it can access, but yet they are now gone.  Yeah, it's probably not
> > > > ever going to really happen, but the lifecycles of dynamic devices are
> > > > tough to "prove" at times, and I worry that freeing things this early is
> > > > going to cause odd disconnect issues.
> > >
> > > Not exactly sure what you mean here, but trying to fix all the driver
> > > bugs we have in drm is why I'm doing this. We have a massive amount of
> > > gaps still, but we're slowly closing them all off with stuff like
> > > drm_dev_enter/exit, to make sure there's no races possible between
> > > driver hotunplug and concurrent access by userspace.
> >
> > Again, look at what v4l did, I think it might work out for you all as
> > well.
>
> I think we're talking past each another quite badly here ...

So I've read through the v4l register/unregister code for the cdev,
and in spirit it matches what we're doing with drm_device. In practice
the drm version is a lot more complicated, mostly because our uapi is
kinda hilarious and our single userspace-facing object is exposed
through an entire set of of different cdev (the one I have here has
like at 5 different cdev registered ...).

The other big difference that I'm seeing is that our equivalent of
video_unregister_device is split up, i.e. we're not using the
equivalent of device_unregister, but the device_del and put_device
that one boils down is split up, with some things happening in
between. Mostly because developers insist that when they remove/unbind
a driver, we're supposed to shut down the display hardware, instead of
just continuing to scan out garbage. Otherwise we could just use the
merged device_unregister().

tldr; I'm not seeing what we're doing totally wrong compared to v4l ...
-Daniel

Daniel Vetter April 6, 2020, 12:32 p.m. UTC | #8

On Fri, Apr 3, 2020 at 3:58 PM Daniel Vetter <[e-mail]> wrote:
>
> In drm we've added nice drm_device (the main gpu driver thing, which
> also represents the userspace interfaces and has everything else
> dangling off it) init functions using devres, devm_drm_dev_init and
> soon devm_drm_dev_alloc (this patch series adds that).
>
> A slight trouble is that drm_device itself holds a reference on the
> struct device it's sitting on top (for sysfs links and dmesg debug and
> lots of other things), so there's a reference loop. For real drivers
> this is broken at remove/unplug time, where all devres resources are
> released device_release_driver(), before the final device reference is
> dropped. So far so good.
>
> There's 2 exceptions:
> - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
>   platform device to make them look more like normal devices to
>   userspace. These aren't drivers in the driver model sense, we simple
>   create a platform_device and register it.
>
> - drm/i915/selftests, where we create minimal mock devices, and again
>   the selftests aren't proper drivers in the driver model sense.
>
> For these two cases the reference loop isn't broken, because devres is
> only cleaned up when the last device reference is dropped. But that's
> not happening, because the drm_device holds that last struct device
> reference.
>
> Thus far this wasn't a problem since the above cases simply
> hand-rolled their cleanup code. But I want to convert all drivers over
> to the devm_ versions, hence it would be really nice if these
> virtual/fake/mock uses-cases could also be managed with devres
> cleanup.
>
> I see three possible approaches:

Restarting this at the top level, because the discussion thus far just
ended in a long "you're doing it wrong", despite that I think we're
doing what v4l is doing (plus/minus that we can't do an exact matching
handling in drm because our uapi has a lot more warts, which we can't
change because no breaking userspace).

So which one of the three below is the right approach?

Aside, looking at the v4l solution I think there's also a confusion
about struct device representing a char device (which v4l directly
uses as its userspace interface refcounted thing, and which drm does
_not_ directly). And a struct device embedded into something like
platform_device or a virtual device, where a driver can bind to. My
question here is about the former, I don't care how cdev struct device
are cleaned up one bit. Now if other subsystems relies on the devres
cleanup behaviour we currently have because of such cdev usage, then
yeah first approach doesn't work (and I have a big surprised that use
case, but hey would actually learn something).

End of aside, since again I want to figure out which of the tree
approaches it the right one. Not about how wrong one of them is,
ignoring the other three I laid out. And maybe there's even more
options for this.
-Daniel

> - Clean up devres from device_del (or platform_device_unregister) even
>   when no driver is bound. This seems like the simplest solution, but
>   also the one with the widest impact, and what this patch implements.
>   We might want to include more of the cleanup than just
>   devres_release_all, but this is all I need to get my use case going.
>
> - Create a devres group and release that when we unbind. The code in
>   virtual drivers gets a bit ugly, since every error case has a
>   different cleanup code until we've chained everything
>   (platform_device, devres group and then drm_device) together and a
>   devres_release_group takes care of everything. Doable, but a bit
>   confusing when I typed it out.
>
> - Convert the virtual drivers to real (in the device model sense)
>   drivers. Feels like too much boilerplate for not much gain. And
>   especially with the mock objects minimal mocking is kinda the goal,
>   to limit the amount of accidentally pulled in code into our unit
>   tests as much as possible.
>
> Either way I think time to discuss this a bit and figure out what's
> the recommended approach here.

>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: "Rafael J. Wysocki" <[e-mail]>
> ---
>  drivers/base/dd.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b25bcab2a26b..1bcfb0ff5f44 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1155,6 +1155,8 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>                                                      dev);
>
>                 kobject_uevent(&dev->kobj, KOBJ_UNBIND);
> +       } else {
> +               devres_release_all(dev);
>         }
>  }
>
> --
> 2.25.1
>

Greg KH April 6, 2020, 1:38 p.m. UTC | #9

On Mon, Apr 06, 2020 at 02:32:51PM +0200, Daniel Vetter wrote:
> On Fri, Apr 3, 2020 at 3:58 PM Daniel Vetter <[e-mail]> wrote:
> >
> > In drm we've added nice drm_device (the main gpu driver thing, which
> > also represents the userspace interfaces and has everything else
> > dangling off it) init functions using devres, devm_drm_dev_init and
> > soon devm_drm_dev_alloc (this patch series adds that).
> >
> > A slight trouble is that drm_device itself holds a reference on the
> > struct device it's sitting on top (for sysfs links and dmesg debug and
> > lots of other things), so there's a reference loop. For real drivers
> > this is broken at remove/unplug time, where all devres resources are
> > released device_release_driver(), before the final device reference is
> > dropped. So far so good.
> >
> > There's 2 exceptions:
> > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> >   platform device to make them look more like normal devices to
> >   userspace. These aren't drivers in the driver model sense, we simple
> >   create a platform_device and register it.
> >
> > - drm/i915/selftests, where we create minimal mock devices, and again
> >   the selftests aren't proper drivers in the driver model sense.
> >
> > For these two cases the reference loop isn't broken, because devres is
> > only cleaned up when the last device reference is dropped. But that's
> > not happening, because the drm_device holds that last struct device
> > reference.
> >
> > Thus far this wasn't a problem since the above cases simply
> > hand-rolled their cleanup code. But I want to convert all drivers over
> > to the devm_ versions, hence it would be really nice if these
> > virtual/fake/mock uses-cases could also be managed with devres
> > cleanup.
> >
> > I see three possible approaches:
> 
> Restarting this at the top level, because the discussion thus far just
> ended in a long "you're doing it wrong", despite that I think we're
> doing what v4l is doing (plus/minus that we can't do an exact matching
> handling in drm because our uapi has a lot more warts, which we can't
> change because no breaking userspace).
> 
> So which one of the three below is the right approach?
> 
> Aside, looking at the v4l solution I think there's also a confusion
> about struct device representing a char device (which v4l directly
> uses as its userspace interface refcounted thing, and which drm does
> _not_ directly). And a struct device embedded into something like
> platform_device or a virtual device, where a driver can bind to. My
> question here is about the former, I don't care how cdev struct device
> are cleaned up one bit. Now if other subsystems relies on the devres
> cleanup behaviour we currently have because of such cdev usage, then
> yeah first approach doesn't work (and I have a big surprised that use
> case, but hey would actually learn something).
> 
> End of aside, since again I want to figure out which of the tree
> approaches it the right one. Not about how wrong one of them is,
> ignoring the other three I laid out. And maybe there's even more
> options for this.

Sorry, been swamped with other things, give me a few days to get back to
this, I need to dig into how you all are dealing with the virtual
drivers.

Doing this in the middle of the merge window is a bit rough :)

thanks,

greg k-h

Daniel Vetter April 6, 2020, 1:55 p.m. UTC | #10

On Mon, Apr 6, 2020 at 3:38 PM Greg Kroah-Hartman
<[e-mail]> wrote:
>
> On Mon, Apr 06, 2020 at 02:32:51PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 3, 2020 at 3:58 PM Daniel Vetter <[e-mail]> wrote:
> > >
> > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > also represents the userspace interfaces and has everything else
> > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > soon devm_drm_dev_alloc (this patch series adds that).
> > >
> > > A slight trouble is that drm_device itself holds a reference on the
> > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > lots of other things), so there's a reference loop. For real drivers
> > > this is broken at remove/unplug time, where all devres resources are
> > > released device_release_driver(), before the final device reference is
> > > dropped. So far so good.
> > >
> > > There's 2 exceptions:
> > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > >   platform device to make them look more like normal devices to
> > >   userspace. These aren't drivers in the driver model sense, we simple
> > >   create a platform_device and register it.
> > >
> > > - drm/i915/selftests, where we create minimal mock devices, and again
> > >   the selftests aren't proper drivers in the driver model sense.
> > >
> > > For these two cases the reference loop isn't broken, because devres is
> > > only cleaned up when the last device reference is dropped. But that's
> > > not happening, because the drm_device holds that last struct device
> > > reference.
> > >
> > > Thus far this wasn't a problem since the above cases simply
> > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > to the devm_ versions, hence it would be really nice if these
> > > virtual/fake/mock uses-cases could also be managed with devres
> > > cleanup.
> > >
> > > I see three possible approaches:
> >
> > Restarting this at the top level, because the discussion thus far just
> > ended in a long "you're doing it wrong", despite that I think we're
> > doing what v4l is doing (plus/minus that we can't do an exact matching
> > handling in drm because our uapi has a lot more warts, which we can't
> > change because no breaking userspace).
> >
> > So which one of the three below is the right approach?
> >
> > Aside, looking at the v4l solution I think there's also a confusion
> > about struct device representing a char device (which v4l directly
> > uses as its userspace interface refcounted thing, and which drm does
> > _not_ directly). And a struct device embedded into something like
> > platform_device or a virtual device, where a driver can bind to. My
> > question here is about the former, I don't care how cdev struct device
> > are cleaned up one bit. Now if other subsystems relies on the devres
> > cleanup behaviour we currently have because of such cdev usage, then
> > yeah first approach doesn't work (and I have a big surprised that use
> > case, but hey would actually learn something).
> >
> > End of aside, since again I want to figure out which of the tree
> > approaches it the right one. Not about how wrong one of them is,
> > ignoring the other three I laid out. And maybe there's even more
> > options for this.
>
> Sorry, been swamped with other things, give me a few days to get back to
> this, I need to dig into how you all are dealing with the virtual
> drivers.

Sure, no problem.

> Doing this in the middle of the merge window is a bit rough :)

Ah I always forget ... we freeze drm at -rc6, so merge window is
actually my most relaxed time since everyone is busy and no one has
time to report drm bugs :-)

Thanks, Daniel

Daniel Vetter April 28, 2020, 1:15 p.m. UTC | #11

On Mon, Apr 06, 2020 at 03:55:28PM +0200, Daniel Vetter wrote:
> On Mon, Apr 6, 2020 at 3:38 PM Greg Kroah-Hartman
> <[e-mail]> wrote:
> >
> > On Mon, Apr 06, 2020 at 02:32:51PM +0200, Daniel Vetter wrote:
> > > On Fri, Apr 3, 2020 at 3:58 PM Daniel Vetter <[e-mail]> wrote:
> > > >
> > > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > > also represents the userspace interfaces and has everything else
> > > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > > soon devm_drm_dev_alloc (this patch series adds that).
> > > >
> > > > A slight trouble is that drm_device itself holds a reference on the
> > > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > > lots of other things), so there's a reference loop. For real drivers
> > > > this is broken at remove/unplug time, where all devres resources are
> > > > released device_release_driver(), before the final device reference is
> > > > dropped. So far so good.
> > > >
> > > > There's 2 exceptions:
> > > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > > >   platform device to make them look more like normal devices to
> > > >   userspace. These aren't drivers in the driver model sense, we simple
> > > >   create a platform_device and register it.
> > > >
> > > > - drm/i915/selftests, where we create minimal mock devices, and again
> > > >   the selftests aren't proper drivers in the driver model sense.
> > > >
> > > > For these two cases the reference loop isn't broken, because devres is
> > > > only cleaned up when the last device reference is dropped. But that's
> > > > not happening, because the drm_device holds that last struct device
> > > > reference.
> > > >
> > > > Thus far this wasn't a problem since the above cases simply
> > > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > > to the devm_ versions, hence it would be really nice if these
> > > > virtual/fake/mock uses-cases could also be managed with devres
> > > > cleanup.
> > > >
> > > > I see three possible approaches:
> > >
> > > Restarting this at the top level, because the discussion thus far just
> > > ended in a long "you're doing it wrong", despite that I think we're
> > > doing what v4l is doing (plus/minus that we can't do an exact matching
> > > handling in drm because our uapi has a lot more warts, which we can't
> > > change because no breaking userspace).
> > >
> > > So which one of the three below is the right approach?
> > >
> > > Aside, looking at the v4l solution I think there's also a confusion
> > > about struct device representing a char device (which v4l directly
> > > uses as its userspace interface refcounted thing, and which drm does
> > > _not_ directly). And a struct device embedded into something like
> > > platform_device or a virtual device, where a driver can bind to. My
> > > question here is about the former, I don't care how cdev struct device
> > > are cleaned up one bit. Now if other subsystems relies on the devres
> > > cleanup behaviour we currently have because of such cdev usage, then
> > > yeah first approach doesn't work (and I have a big surprised that use
> > > case, but hey would actually learn something).
> > >
> > > End of aside, since again I want to figure out which of the tree
> > > approaches it the right one. Not about how wrong one of them is,
> > > ignoring the other three I laid out. And maybe there's even more
> > > options for this.
> >
> > Sorry, been swamped with other things, give me a few days to get back to
> > this, I need to dig into how you all are dealing with the virtual
> > drivers.
> 
> Sure, no problem.
> 
> > Doing this in the middle of the merge window is a bit rough :)
> 
> Ah I always forget ... we freeze drm at -rc6, so merge window is
> actually my most relaxed time since everyone is busy and no one has
> time to report drm bugs :-)

Hi Greg,

Since -rc3 is out, had any to ponder this? Otherwise we'll be right back
in the next merge window ...

Cheers, Daniel

Greg KH May 15, 2020, 12:55 p.m. UTC | #12

On Tue, Apr 28, 2020 at 03:15:12PM +0200, Daniel Vetter wrote:
> On Mon, Apr 06, 2020 at 03:55:28PM +0200, Daniel Vetter wrote:
> > On Mon, Apr 6, 2020 at 3:38 PM Greg Kroah-Hartman
> > <[e-mail]> wrote:
> > >
> > > On Mon, Apr 06, 2020 at 02:32:51PM +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 3, 2020 at 3:58 PM Daniel Vetter <[e-mail]> wrote:
> > > > >
> > > > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > > > also represents the userspace interfaces and has everything else
> > > > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > > > soon devm_drm_dev_alloc (this patch series adds that).
> > > > >
> > > > > A slight trouble is that drm_device itself holds a reference on the
> > > > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > > > lots of other things), so there's a reference loop. For real drivers
> > > > > this is broken at remove/unplug time, where all devres resources are
> > > > > released device_release_driver(), before the final device reference is
> > > > > dropped. So far so good.
> > > > >
> > > > > There's 2 exceptions:
> > > > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > > > >   platform device to make them look more like normal devices to
> > > > >   userspace. These aren't drivers in the driver model sense, we simple
> > > > >   create a platform_device and register it.
> > > > >
> > > > > - drm/i915/selftests, where we create minimal mock devices, and again
> > > > >   the selftests aren't proper drivers in the driver model sense.
> > > > >
> > > > > For these two cases the reference loop isn't broken, because devres is
> > > > > only cleaned up when the last device reference is dropped. But that's
> > > > > not happening, because the drm_device holds that last struct device
> > > > > reference.
> > > > >
> > > > > Thus far this wasn't a problem since the above cases simply
> > > > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > > > to the devm_ versions, hence it would be really nice if these
> > > > > virtual/fake/mock uses-cases could also be managed with devres
> > > > > cleanup.
> > > > >
> > > > > I see three possible approaches:
> > > >
> > > > Restarting this at the top level, because the discussion thus far just
> > > > ended in a long "you're doing it wrong", despite that I think we're
> > > > doing what v4l is doing (plus/minus that we can't do an exact matching
> > > > handling in drm because our uapi has a lot more warts, which we can't
> > > > change because no breaking userspace).
> > > >
> > > > So which one of the three below is the right approach?
> > > >
> > > > Aside, looking at the v4l solution I think there's also a confusion
> > > > about struct device representing a char device (which v4l directly
> > > > uses as its userspace interface refcounted thing, and which drm does
> > > > _not_ directly). And a struct device embedded into something like
> > > > platform_device or a virtual device, where a driver can bind to. My
> > > > question here is about the former, I don't care how cdev struct device
> > > > are cleaned up one bit. Now if other subsystems relies on the devres
> > > > cleanup behaviour we currently have because of such cdev usage, then
> > > > yeah first approach doesn't work (and I have a big surprised that use
> > > > case, but hey would actually learn something).
> > > >
> > > > End of aside, since again I want to figure out which of the tree
> > > > approaches it the right one. Not about how wrong one of them is,
> > > > ignoring the other three I laid out. And maybe there's even more
> > > > options for this.
> > >
> > > Sorry, been swamped with other things, give me a few days to get back to
> > > this, I need to dig into how you all are dealing with the virtual
> > > drivers.
> > 
> > Sure, no problem.
> > 
> > > Doing this in the middle of the merge window is a bit rough :)
> > 
> > Ah I always forget ... we freeze drm at -rc6, so merge window is
> > actually my most relaxed time since everyone is busy and no one has
> > time to report drm bugs :-)
> 
> Hi Greg,
> 
> Since -rc3 is out, had any to ponder this? Otherwise we'll be right back
> in the next merge window ...

I owe you a response to this.  I'm going to try to carve out some time
on Monday to do this, sorry for the delay :(

greg k-h

Daniel Vetter May 15, 2020, 2:05 p.m. UTC | #13

On Fri, May 15, 2020 at 02:55:38PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 28, 2020 at 03:15:12PM +0200, Daniel Vetter wrote:
> > On Mon, Apr 06, 2020 at 03:55:28PM +0200, Daniel Vetter wrote:
> > > On Mon, Apr 6, 2020 at 3:38 PM Greg Kroah-Hartman
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Apr 06, 2020 at 02:32:51PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Apr 3, 2020 at 3:58 PM Daniel Vetter <[e-mail]> wrote:
> > > > > >
> > > > > > In drm we've added nice drm_device (the main gpu driver thing, which
> > > > > > also represents the userspace interfaces and has everything else
> > > > > > dangling off it) init functions using devres, devm_drm_dev_init and
> > > > > > soon devm_drm_dev_alloc (this patch series adds that).
> > > > > >
> > > > > > A slight trouble is that drm_device itself holds a reference on the
> > > > > > struct device it's sitting on top (for sysfs links and dmesg debug and
> > > > > > lots of other things), so there's a reference loop. For real drivers
> > > > > > this is broken at remove/unplug time, where all devres resources are
> > > > > > released device_release_driver(), before the final device reference is
> > > > > > dropped. So far so good.
> > > > > >
> > > > > > There's 2 exceptions:
> > > > > > - drm/vkms|vgem: Virtual drivers for which we create a fake/virtual
> > > > > >   platform device to make them look more like normal devices to
> > > > > >   userspace. These aren't drivers in the driver model sense, we simple
> > > > > >   create a platform_device and register it.
> > > > > >
> > > > > > - drm/i915/selftests, where we create minimal mock devices, and again
> > > > > >   the selftests aren't proper drivers in the driver model sense.
> > > > > >
> > > > > > For these two cases the reference loop isn't broken, because devres is
> > > > > > only cleaned up when the last device reference is dropped. But that's
> > > > > > not happening, because the drm_device holds that last struct device
> > > > > > reference.
> > > > > >
> > > > > > Thus far this wasn't a problem since the above cases simply
> > > > > > hand-rolled their cleanup code. But I want to convert all drivers over
> > > > > > to the devm_ versions, hence it would be really nice if these
> > > > > > virtual/fake/mock uses-cases could also be managed with devres
> > > > > > cleanup.
> > > > > >
> > > > > > I see three possible approaches:
> > > > >
> > > > > Restarting this at the top level, because the discussion thus far just
> > > > > ended in a long "you're doing it wrong", despite that I think we're
> > > > > doing what v4l is doing (plus/minus that we can't do an exact matching
> > > > > handling in drm because our uapi has a lot more warts, which we can't
> > > > > change because no breaking userspace).
> > > > >
> > > > > So which one of the three below is the right approach?
> > > > >
> > > > > Aside, looking at the v4l solution I think there's also a confusion
> > > > > about struct device representing a char device (which v4l directly
> > > > > uses as its userspace interface refcounted thing, and which drm does
> > > > > _not_ directly). And a struct device embedded into something like
> > > > > platform_device or a virtual device, where a driver can bind to. My
> > > > > question here is about the former, I don't care how cdev struct device
> > > > > are cleaned up one bit. Now if other subsystems relies on the devres
> > > > > cleanup behaviour we currently have because of such cdev usage, then
> > > > > yeah first approach doesn't work (and I have a big surprised that use
> > > > > case, but hey would actually learn something).
> > > > >
> > > > > End of aside, since again I want to figure out which of the tree
> > > > > approaches it the right one. Not about how wrong one of them is,
> > > > > ignoring the other three I laid out. And maybe there's even more
> > > > > options for this.
> > > >
> > > > Sorry, been swamped with other things, give me a few days to get back to
> > > > this, I need to dig into how you all are dealing with the virtual
> > > > drivers.
> > > 
> > > Sure, no problem.
> > > 
> > > > Doing this in the middle of the merge window is a bit rough :)
> > > 
> > > Ah I always forget ... we freeze drm at -rc6, so merge window is
> > > actually my most relaxed time since everyone is busy and no one has
> > > time to report drm bugs :-)
> > 
> > Hi Greg,
> > 
> > Since -rc3 is out, had any to ponder this? Otherwise we'll be right back
> > in the next merge window ...
> 
> I owe you a response to this.  I'm going to try to carve out some time
> on Monday to do this, sorry for the delay :(

No worries, I got myself plenty occupied with other fun stuff in graphics
for now. And the part of the series which doesn't need this here is all
landed, so new drivers already look a notch cleaner in their code.

I'd have poked you earlier, but ofc very much appreciated if you can look
into this a bit before the next merge window.

Thanks, Daniel

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index b25bcab2a26b..1bcfb0ff5f44 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1155,6 +1155,8 @@  static void __device_release_driver(struct device *dev, struct device *parent)
 						     dev);
 
 		kobject_uevent(&dev->kobj, KOBJ_UNBIND);
+	} else {
+		devres_release_all(dev);
 	}
 }