From patchwork Mon Aug 1 15:42:14 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Hilman X-Patchwork-Id: 1026212 Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p71FgdmK009696 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 1 Aug 2011 15:43:00 GMT Received: from canuck.infradead.org ([2001:4978:20e::1]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1QnudS-0007Jp-3t; Mon, 01 Aug 2011 15:42:30 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1QnudR-0005QR-Kf; Mon, 01 Aug 2011 15:42:29 +0000 Received: from na3sys009aog109.obsmtp.com ([74.125.149.201]) by canuck.infradead.org with smtps (Exim 4.76 #1 (Red Hat Linux)) id 1QnudN-0005Q6-S8 for linux-arm-kernel@lists.infradead.org; Mon, 01 Aug 2011 15:42:27 +0000 Received: from mail-pz0-f41.google.com ([209.85.210.41]) (using TLSv1) by na3sys009aob109.postini.com ([74.125.148.12]) with SMTP ID DSNKTjbJWi3j8tpp0gmpzCN7F18fHpeBUd1h@postini.com; Mon, 01 Aug 2011 08:42:25 PDT Received: by mail-pz0-f41.google.com with SMTP id 4so11603462pzk.0 for ; Mon, 01 Aug 2011 08:42:18 -0700 (PDT) Received: by 10.68.29.35 with SMTP id g3mr502422pbh.79.1312213338303; Mon, 01 Aug 2011 08:42:18 -0700 (PDT) Received: from localhost (c-24-19-7-36.hsd1.wa.comcast.net [24.19.7.36]) by mx.google.com with ESMTPS id d3sm5665918pbh.21.2011.08.01.08.42.15 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 01 Aug 2011 08:42:15 -0700 (PDT) From: Kevin Hilman To: Russell King - ARM Linux Subject: Re: [RFC/PATCH 7/7] WIP: HACK/RFC: omap_device: begin to decouple platform_device from omap_device Organization: Texas Instruments, Inc. References: <1311292338-11830-1-git-send-email-khilman@ti.com> <1311292338-11830-9-git-send-email-khilman@ti.com> <20110730120332.GA15539@n2100.arm.linux.org.uk> <20110731025807.GA24334@ponder.secretlab.ca> <20110731150540.GA3019@n2100.arm.linux.org.uk> Date: Mon, 01 Aug 2011 08:42:14 -0700 In-Reply-To: <20110731150540.GA3019@n2100.arm.linux.org.uk> (Russell King's message of "Sun, 31 Jul 2011 16:05:40 +0100") Message-ID: <871ux5nnop.fsf@ti.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (gnu/linux) MIME-Version: 1.0 X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110801_114226_259915_4512508D X-CRM114-Status: GOOD ( 41.82 ) X-Spam-Score: -2.3 (--) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (-2.3 points) pts rule name description ---- ---------------------- -------------------------------------------------- -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, medium trust [74.125.149.201 listed in list.dnswl.org] Cc: Paul Walmsley , "G. Manjunath Kondaiah" , devicetree-discuss@lists.ozlabs.org, Grant Likely , linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Mon, 01 Aug 2011 15:43:00 +0000 (UTC) Russell King - ARM Linux writes: > On Sat, Jul 30, 2011 at 08:58:07PM -0600, Grant Likely wrote: >> On Sat, Jul 30, 2011 at 01:03:32PM +0100, Russell King - ARM Linux wrote: >> > On Thu, Jul 21, 2011 at 04:52:18PM -0700, Kevin Hilman wrote: >> > > Rather than embedding a struct platform_device inside a struct >> > > omap_device, decouple them, leaving only a pointer to the >> > > platform_device inside the omap_device. >> > > >> > > This patch uses devres to allocate and attach the omap_device to the >> > > struct device, so finding an omap_device from a struct device means >> > > using devres_find(), and the to_omap_device() helper function was >> > > modified accordingly. >> > > >> > > RFC/Hack alert: >> > > >> > > Currently the driver core (drivers/base/dd.c) doesn't expect any >> > > devres resources to exist before the driver's ->probe() is called. In >> > > this patch, I just comment out the warning, but we'll need to >> > > understand why that limitation exists, and if it's a real limitation. >> > > A first glance suggests that it's not really needed. If this is a >> > > true limitation, we'll need to find some way other than devres to >> > > attach an omap_device to a struct device. >> > > >> > > On OMAP, we will need an omap_device attached to a struct device >> > > before probe because device HW may be disabled in probe and drivers >> > > are expected to use runtime PM in ->probe() to activate the hardware >> > > before access. Because the runtime PM API calls use omap_device (via >> > > our PM domain layer), we need omap_device attached to a >> > > platform_device before probe. >> > >> > This feels really wrong to overload devres with this. devres purpose is >> > to provide the device's _drivers_ with a way to allocate and free resources >> > in such a way to avoid leaks on .remove or probe failure. So I think that >> > overloading it with something that has a different lifetime is completely >> > wrong. >> >> I disagree; extending devres to also handle device lifetime scoped >> resources makes perfect sense. It is a clean extension, and struct device >> is really getting rather huge. I certainly prefer re-scoping devres >> to adding more elements to struct device. > > The point is you're asking something which is designed to have a well > defined lifetime of driver-bind...driver-unbind to do a lot more than > that - extending its lifetime conditional on some flag to the entire > device lifetime instead. > > Such extensions tend to be a disaster - and a recipe for confusion as > people will no longer have a clear idea of the lifetime issues. We > already know that people absolutely struggle to understand lifed > objects - we see it time and time again where people directly kfree > stuff like platform devices without thinking about whether they're > still in use. > > So no, extending devres is a _big_ mistake and is far from clean. > > Not only that, but if most of the platform devices are omap devices, > (as it would be on OMAP), you'll consume more memory through having to > have the additional management structs for the omap_device stuff than > a simple pointer. > > As for struct device getting rather huge, last time I looked it contained > a load of stuff which was there whether or not a platform used it. Eg, > you get 4 bytes wasted for an of_node whether you have DT support or not. > If sizeof(struct device) really is a problem, then it needs the unused > stuff ifdef'd away. So I don't buy the "its getting rather huge" argument. > >> > We could add a new member to struct dev_archdata or pdev_archdata to carry >> > a pointer to this data, which I think would be a far cleaner (and saner) >> > way to deal with this. In much the same way as we already have an of_node >> > member in struct device. >> >> Since this is just for omap_device, which by definition is arm-only, I >> don't have any strong objection to using pdev_archdata. If it was >> cross-architecture, then I'd argue strongly for the devres approach. > > Indeed, which is why I think putting it in the platform device archdata > makes total sense, more sense than buggering up the clean devres lifetime > rules that we have today. OK, so I'll continue this approach using pdev_archdata, which would work fine for me. It's currently empty, so I'll just add a 'void *' if it's OK with you Russell: diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h index 9f390ce..bb777cd 100644 --- a/arch/arm/include/asm/device.h +++ b/arch/arm/include/asm/device.h @@ -13,6 +13,7 @@ struct dev_archdata { }; struct pdev_archdata { + void *p; }; #endif