Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Daniel Vetter]

On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> > On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> > > Den 05.08.2017 00.19, skrev Ilia Mirkin:
> > >> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@xxxxxxxxxx> wrote:
> > >>> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> writes:
> > >>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> > >>>>> This will let drivers reduce the error cleanup they need, in
> > >>>>> particular the "is_panel_bridge" flag.
> > >>>>> 
> > >>>>> v2: Slight cleanup of remove function by Andrzej
> > >>>> 
> > >>>> I just want to point out that, in the context of Daniel's work on
> > >>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> > >>>> the way. All DRM core objects that are accessible one way or
> > >>>> another from userspace will need to be properly reference-counted
> > >>>> and freed only when the last reference disappears, which could be
> > >>>> well after the corresponding device is removed. I believe this
> > >>>> could be one such objects :-/
> > >>> 
> > >>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> > >>> devices, like our SOC platform devices (current panel-bridge
> > >>> consumers), this still seems like an excellent simplification of
> > >>> memory management.
> > >> 
> > >> At that point you may as well make your module non-unloadable, and
> > >> return failure when trying to remove a device from management by the
> > >> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> > >> doesn't only happen when physically removing, it can happen for all
> > >> kinds of reasons... and userspace may still hold references in some of
> > >> those cases.
> > > 
> > > If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> > > won't that delay devm_* cleanup until userspace is done?
> > 
> > No. drm_device is the thing that is refcounted for userspace references
> > like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> > don't).
> > 
> > devm_ otoh is tied to the lifetime of the underlying device, and that one
> > can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
> > on unplug, and not when the final sw reference of the struct device
> > disappears.
> > 
> > Not sure tough, it's complicated.
> 
> It's complicated when you have to understand the behaviour by reading the 
> code, but the behaviour isn't that complex. devm resources are released both
> 
> 1. right after the driver's .remove() operation returns
> 2. when the device is deleted (last reference released)

Right, I had vague memories when reading the code ... But iirc there's
also some way to delay cleanup until it's deleted, maybe we could exploit
that (and wrap it up into a drm_devm_add wrapper)?

Plan B, but that is a lot more ugly, would be to create a fake struct
device that we never bind (hence remove can't be called) and use to track
the lifetime of drm_device stuff. Would avoid re-inventing all the devm_
functions, but would also result in a dummy device in sysfs.

Why is this stuff tied to struct device and not struct kobject anyway. Or
at least something we could embed in random cool place (like drm_device).

Greg, any ideas how we could simplify management of stuff that's created
at driver load, but its lifetime tied to something which isn't directly a
struct device?

> It's the first one that makes devm_* allocation unsuitable for any structure 
> that is accessible from userspace (such as in file operation handlers).

Yeah, if we could release at 2. it would be perfect I think ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch