Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703997 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-04 15:50 +0200
    Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-04 17:00 +0200
    Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Eric Anholt <eric@anholt.net> - 2017-08-04 22:50 +0200
      Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-04 23:30 +0200
      Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Ilia Mirkin <imirkin@alum.mit.edu> - 2017-08-05 00:30 +0200
        Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel  bridge. Noralf Trønnes <noralf@tronnes.org> - 2017-08-05 13:10 +0200
          Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel  bridge. Noralf Trønnes <noralf@tronnes.org> - 2017-08-05 16:50 +0200
          Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel  bridge. Daniel Vetter <daniel@ffwll.ch> - 2017-08-07 11:30 +0200
            Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-07 12:30 +0200
              Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel  bridge. Noralf Trønnes <noralf@tronnes.org> - 2017-08-07 16:40 +0200
              Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel  bridge. Daniel Vetter <daniel@ffwll.ch> - 2017-08-07 17:00 +0200
                Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-08-08 00:00 +0200

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

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

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 :-/

> Signed-off-by: Eric Anholt <eric@anholt.net>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h       |  3 +++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 685c1a480201..292ee92a9c97 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
> devm_kfree(panel_bridge->panel->dev, bridge);
>  }
>  EXPORT_SYMBOL(drm_panel_bridge_remove);
> +
> +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> +{
> +	struct drm_bridge **bridge = res;
> +
> +	drm_panel_bridge_remove(*bridge);
> +}
> +
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type)
> +{
> +	struct drm_bridge **ptr, *bridge;
> +
> +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge = drm_panel_bridge_add(panel, connector_type);
> +	if (!IS_ERR(bridge)) {
> +		*ptr = bridge;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1dc94d5392e2..6522d4cbc9d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>  					u32 connector_type);
>  void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type);
>  #endif
> 
>  #endif

-- 
Regards,

Laurent Pinchart

[toc] | [next] | [standalone]

#1704055

Now CC'ing Daniel with his correct address :-/ I'll blame auto-completion in 
my e-mail client. Sorry for the noise.

On Friday 04 Aug 2017 16:46:24 Laurent Pinchart wrote:
> Hi Eric,
> 
> (CC'ing Daniel)
> 
> Thank you for the patch.
> 
> 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 :-/
> 
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h       |  3 +++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/panel.c
> > b/drivers/gpu/drm/bridge/panel.c index 685c1a480201..292ee92a9c97 100644
> > --- a/drivers/gpu/drm/bridge/panel.c
> > +++ b/drivers/gpu/drm/bridge/panel.c
> > @@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge
> > *bridge) devm_kfree(panel_bridge->panel->dev, bridge);
> > 
> >  }
> >  EXPORT_SYMBOL(drm_panel_bridge_remove);
> > 
> > +
> > +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> > +{
> > +	struct drm_bridge **bridge = res;
> > +
> > +	drm_panel_bridge_remove(*bridge);
> > +}
> > +
> > +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> > +					     struct drm_panel *panel,
> > +					     u32 connector_type)
> > +{
> > +	struct drm_bridge **ptr, *bridge;
> > +
> > +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	bridge = drm_panel_bridge_add(panel, connector_type);
> > +	if (!IS_ERR(bridge)) {
> > +		*ptr = bridge;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return bridge;
> > +}
> > +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 1dc94d5392e2..6522d4cbc9d9 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
> > 
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >  
> >  					u32 connector_type);
> >  
> >  void drm_panel_bridge_remove(struct drm_bridge *bridge);
> > 
> > +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> > +					     struct drm_panel *panel,
> > +					     u32 connector_type);
> > 
> >  #endif
> >  
> >  #endif

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1704215

[Multipart message — attachments visible in raw view] — view raw

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Eric,
>
> (CC'ing Daniel)
>
> Thank you for the patch.
>
> 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.

[toc] | [prev] | [next] | [standalone]

#1704226

Hi Eric,

On Friday 04 Aug 2017 13:43:25 Eric Anholt wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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.

It encourages driver writers to write code that they will have to fix later. 
That's certainly a simplification, but certainly not a good thing in my 
opinion :-)

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1704262

On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> Hi Eric,
>>
>> (CC'ing Daniel)
>>
>> Thank you for the patch.
>>
>> 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.

[toc] | [prev] | [next] | [standalone]

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

(I had to switch to Daniel's Intel address to get this sent)

Den 05.08.2017 00.19, skrev Ilia Mirkin:
> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>>
>>> Hi Eric,
>>>
>>> (CC'ing Daniel)
>>>
>>> Thank you for the patch.
>>>
>>> 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?

[toc] | [prev] | [next] | [standalone]

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

Den 05.08.2017 12.59, skrev Noralf Trønnes:
> (I had to switch to Daniel's Intel address to get this sent)
>
> Den 05.08.2017 00.19, skrev Ilia Mirkin:
>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>>>
>>>> Hi Eric,
>>>>
>>>> (CC'ing Daniel)
>>>>
>>>> Thank you for the patch.
>>>>
>>>> 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?
>

It seems plausible looking at the code:

void device_initialize(struct device *dev)
{
[...]
     kobject_init(&dev->kobj, &device_ktype);
[...]
}

static struct kobj_type device_ktype = {
     .release    = device_release,
};

/**
  * device_release - free device structure.
  * @kobj: device's kobject.
  *
  * This is called once the reference count for the object
  * reaches 0. We forward the call to the device's release
  * method, which should handle actually freeing the structure.
  */
static void device_release(struct kobject *kobj)
{
[...]
     devres_release_all(dev);
[...]
}

Last put call chain:
put_device() -> kobject_put() -> kref_put() -> kobject_release() ->
kobject_cleanup() -> device_release() -> devres_release_all()

But I haven't actually tried it, so I might be mistaken.

[toc] | [prev] | [next] | [standalone]

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

On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> (I had to switch to Daniel's Intel address to get this sent)
> 
> Den 05.08.2017 00.19, skrev Ilia Mirkin:
> > On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > > 
> > > > Hi Eric,
> > > > 
> > > > (CC'ing Daniel)
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1705330

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@anholt.net> wrote:
> >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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)

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

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

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

Den 07.08.2017 12.22, skrev Laurent Pinchart:
> 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@anholt.net> wrote:
>>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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)
>
> It's the first one that makes devm_* allocation unsuitable for any structure
> that is accessible from userspace (such as in file operation handlers).
>

I see, when the device is removed, the driver is released. No help
holding a ref on struct device.
I did a test and this is the stack trace in devm_tinydrm_release():

[  129.433179] [<c0016148>] (unwind_backtrace) from [<c0013c90>] 
(show_stack+0x20/0x24)
[  129.433213] [<c0013c90>] (show_stack) from [<c0319e78>] 
(dump_stack+0x20/0x28)
[  129.433242] [<c0319e78>] (dump_stack) from [<c0021d18>] 
(__warn+0xe4/0x10c)
[  129.433264] [<c0021d18>] (__warn) from [<c0021e0c>] 
(warn_slowpath_null+0x30/0x38)
[  129.433324] [<c0021e0c>] (warn_slowpath_null) from [<bf2ca3cc>] 
(devm_tinydrm_release+0x24/0x48 [tinydrm])
[  129.433389] [<bf2ca3cc>] (devm_tinydrm_release [tinydrm]) from 
[<c03bf7f0>] (devm_action_release+0x1c/0x20)
[  129.433414] [<c03bf7f0>] (devm_action_release) from [<c03bfc70>] 
(release_nodes+0x188/0x21c)
[  129.433437] [<c03bfc70>] (release_nodes) from [<c03c076c>] 
(devres_release_all+0x44/0x64)
[  129.433461] [<c03c076c>] (devres_release_all) from [<c03bcdb8>] 
(__device_release_driver+0x9c/0x118)
[  129.433480] [<c03bcdb8>] (__device_release_driver) from [<c03bce60>] 
(device_release_driver+0x2c/0x38)
[  129.433499] [<c03bce60>] (device_release_driver) from [<c03bbe04>] 
(bus_remove_device+0xe4/0x114)
[  129.433532] [<c03bbe04>] (bus_remove_device) from [<c03b8980>] 
(device_del+0x118/0x22c)
[  129.433554] [<c03b8980>] (device_del) from [<c03b8ab0>] 
(device_unregister+0x1c/0x30)
[  129.433592] [<c03b8ab0>] (device_unregister) from [<c04062c0>] 
(spi_unregister_device+0x60/0x64)
[  129.433617] [<c04062c0>] (spi_unregister_device) from [<c0409438>] 
(of_spi_notify+0x70/0x164)
[  129.433642] [<c0409438>] (of_spi_notify) from [<c0040148>] 
(notifier_call_chain+0x54/0x94)
[  129.433664] [<c0040148>] (notifier_call_chain) from [<c00405dc>] 
(__blocking_notifier_call_chain+0x58/0x70)
[  129.433683] [<c00405dc>] (__blocking_notifier_call_chain) from 
[<c004061c>] (blocking_notifier_call_chain+0x28/0x30)
[  129.433721] [<c004061c>] (blocking_notifier_call_chain) from 
[<c04b7058>] (__of_changeset_entry_notify+0x90/0xe0)
[  129.433748] [<c04b7058>] (__of_changeset_entry_notify) from 
[<c04b7aec>] (__of_changeset_revert+0x70/0xcc)
[  129.433774] [<c04b7aec>] (__of_changeset_revert) from [<c04bb46c>] 
(of_overlay_destroy+0x10c/0x1bc)
[  129.433795] [<c04bb46c>] (of_overlay_destroy) from [<c04b6954>] 
(cfs_overlay_release+0x2c/0x50)
[  129.433832] [<c04b6954>] (cfs_overlay_release) from [<c01be83c>] 
(config_item_release+0x68/0x8c)
[  129.433859] [<c01be83c>] (config_item_release) from [<c01be7d0>] 
(config_item_put+0x44/0x48)
[  129.433879] [<c01be7d0>] (config_item_put) from [<c01bcf50>] 
(configfs_rmdir+0x190/0x220)
[  129.433902] [<c01bcf50>] (configfs_rmdir) from [<c0152204>] 
(vfs_rmdir+0x80/0x118)
[  129.433922] [<c0152204>] (vfs_rmdir) from [<c01547f8>] 
(do_rmdir+0x144/0x1a0)
[  129.433941] [<c01547f8>] (do_rmdir) from [<c01551f0>] 
(SyS_rmdir+0x20/0x24)
[  129.433966] [<c01551f0>] (SyS_rmdir) from [<c000fe40>] 
(ret_fast_syscall+0x0/0x1c)

This means that tinydrm won't work with usb devices.
I need to look into moving cleanup to drm_driver->cleanup.

Noralf.

[toc] | [prev] | [next] | [standalone]

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

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@anholt.net> wrote:
> > >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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

[toc] | [prev] | [next] | [standalone]

#1705888

Hi Daniel,

On Monday 07 Aug 2017 16:59:39 Daniel Vetter wrote:
> On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> > 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@anholt.net> wrote:
> >>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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.

If we wanted to go that way, we should decouple devm_* from struct device (or 
even struct kobject) and tie it to a new resource tracker structure. The 
current devm_* API would remain the same, embedding that resource tracker 
object in struct device, but we could also use the machinery with the resource 
tracker object directly.

However, I'm not convince we should do so. We don't have any automatic memory 
allocation system for DRM objects (such as framebuffers or planes). Instead, 
drivers must implement a destroy operation for the object, and the core calls 
it when the last reference to the object goes away. This works fine, and I 
don't see what would prevent use from implementing the same mechanism for 
bridges or other similar structures.

> 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 ...

It wouldn't, as unbind/rebind cycles would allocate new objects each time 
without freeing the previous ones until the device is deleted.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web