[Intel-gfx] [PATCH 01/44] drivers/base: Always release devres on device_del

Fri Apr 3 15:33:17 UTC 2020

On Fri, Apr 3, 2020 at 5:15 PM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>
> On Fri, Apr 3, 2020 at 5:06 PM Greg Kroah-Hartman
> <gregkh at linuxfoundation.org> 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
> > > <gregkh at linuxfoundation.org> 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
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch