| Message ID | 20170718210510.12229-2-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/18/2017 11:05 PM, 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 > > Signed-off-by: Eric Anholt <eric@anholt.net> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Philippe Cornu <philippe.cornu@st.com> Tested-by: Philippe Cornu <philippe.cornu@st.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 >
Philippe CORNU <philippe.cornu@st.com> writes: > On 07/18/2017 11:05 PM, 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 >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > Reviewed-by: Philippe Cornu <philippe.cornu@st.com> > Tested-by: Philippe Cornu <philippe.cornu@st.com> I've pushed this patch from the series, but hopefully I can get some review on the rest.
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
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
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.
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 :-)
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.
(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?
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.
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
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).
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.
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
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.
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