Messages in this thread | | | Subject | Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. | From | Noralf Trønnes <> | Date | Mon, 7 Aug 2017 16:37:47 +0200 |
| |
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.
| |