Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751948AbdHGO7r (ORCPT ); Mon, 7 Aug 2017 10:59:47 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34569 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751247AbdHGO7o (ORCPT ); Mon, 7 Aug 2017 10:59:44 -0400 Date: Mon, 7 Aug 2017 16:59:39 +0200 From: Daniel Vetter To: Laurent Pinchart , Greg Kroah-Hartman Cc: Daniel Vetter , Noralf =?iso-8859-1?Q?Tr=F8nnes?= , Ilia Mirkin , Eric Anholt , Daniel Vetter , Thierry Reding , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" Subject: Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge. Message-ID: <20170807145939.bx23xyejkcfp76ze@phenom.ffwll.local> Mail-Followup-To: Laurent Pinchart , Greg Kroah-Hartman , Noralf =?iso-8859-1?Q?Tr=F8nnes?= , 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <30057693.2anM9zYF0F@avalon> X-Operating-System: Linux phenom 4.11.0-2-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3773 Lines: 79 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 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) 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