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

["Noralf Trønnes" <noralf@tronnes.org>]

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.