Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751904AbdHGOiX (ORCPT ); Mon, 7 Aug 2017 10:38:23 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:37954 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751775AbdHGOiV (ORCPT ); Mon, 7 Aug 2017 10:38:21 -0400 Subject: Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. To: Laurent Pinchart , Daniel Vetter Cc: Ilia Mirkin , Eric Anholt , Daniel Vetter , Thierry Reding , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" References: <20170718210510.12229-1-eric@anholt.net> <20170807092507.b735ribfpjm6oejk@phenom.ffwll.local> <30057693.2anM9zYF0F@avalon> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <0245ff59-ca89-8801-17d2-780e1837d203@tronnes.org> Date: Mon, 7 Aug 2017 16:37:47 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <30057693.2anM9zYF0F@avalon> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5835 Lines: 120 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 wrote: >>>>> Laurent Pinchart 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] [] (unwind_backtrace) from [] (show_stack+0x20/0x24) [ 129.433213] [] (show_stack) from [] (dump_stack+0x20/0x28) [ 129.433242] [] (dump_stack) from [] (__warn+0xe4/0x10c) [ 129.433264] [] (__warn) from [] (warn_slowpath_null+0x30/0x38) [ 129.433324] [] (warn_slowpath_null) from [] (devm_tinydrm_release+0x24/0x48 [tinydrm]) [ 129.433389] [] (devm_tinydrm_release [tinydrm]) from [] (devm_action_release+0x1c/0x20) [ 129.433414] [] (devm_action_release) from [] (release_nodes+0x188/0x21c) [ 129.433437] [] (release_nodes) from [] (devres_release_all+0x44/0x64) [ 129.433461] [] (devres_release_all) from [] (__device_release_driver+0x9c/0x118) [ 129.433480] [] (__device_release_driver) from [] (device_release_driver+0x2c/0x38) [ 129.433499] [] (device_release_driver) from [] (bus_remove_device+0xe4/0x114) [ 129.433532] [] (bus_remove_device) from [] (device_del+0x118/0x22c) [ 129.433554] [] (device_del) from [] (device_unregister+0x1c/0x30) [ 129.433592] [] (device_unregister) from [] (spi_unregister_device+0x60/0x64) [ 129.433617] [] (spi_unregister_device) from [] (of_spi_notify+0x70/0x164) [ 129.433642] [] (of_spi_notify) from [] (notifier_call_chain+0x54/0x94) [ 129.433664] [] (notifier_call_chain) from [] (__blocking_notifier_call_chain+0x58/0x70) [ 129.433683] [] (__blocking_notifier_call_chain) from [] (blocking_notifier_call_chain+0x28/0x30) [ 129.433721] [] (blocking_notifier_call_chain) from [] (__of_changeset_entry_notify+0x90/0xe0) [ 129.433748] [] (__of_changeset_entry_notify) from [] (__of_changeset_revert+0x70/0xcc) [ 129.433774] [] (__of_changeset_revert) from [] (of_overlay_destroy+0x10c/0x1bc) [ 129.433795] [] (of_overlay_destroy) from [] (cfs_overlay_release+0x2c/0x50) [ 129.433832] [] (cfs_overlay_release) from [] (config_item_release+0x68/0x8c) [ 129.433859] [] (config_item_release) from [] (config_item_put+0x44/0x48) [ 129.433879] [] (config_item_put) from [] (configfs_rmdir+0x190/0x220) [ 129.433902] [] (configfs_rmdir) from [] (vfs_rmdir+0x80/0x118) [ 129.433922] [] (vfs_rmdir) from [] (do_rmdir+0x144/0x1a0) [ 129.433941] [] (do_rmdir) from [] (SyS_rmdir+0x20/0x24) [ 129.433966] [] (SyS_rmdir) from [] (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.