Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756367Ab0BOTuU (ORCPT ); Mon, 15 Feb 2010 14:50:20 -0500 Received: from mail-yw0-f176.google.com ([209.85.211.176]:35723 "EHLO mail-yw0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756338Ab0BOTuQ (ORCPT ); Mon, 15 Feb 2010 14:50:16 -0500 MIME-Version: 1.0 In-Reply-To: <20100209191427.GA18263@oksana.dev.rtsoft.ru> References: <20100205204949.GA2575@oksana.dev.rtsoft.ru> <20100205205045.GC4178@oksana.dev.rtsoft.ru> <20100209191427.GA18263@oksana.dev.rtsoft.ru> From: Grant Likely Date: Mon, 15 Feb 2010 12:49:56 -0700 X-Google-Sender-Auth: 27327af48699f128 Message-ID: Subject: Re: [PATCH 3/3] of/gpio: Introduce of_put_gpio(), add ref counting for OF GPIO chips To: avorontsov@ru.mvista.com Cc: David Brownell , Benjamin Herrenschmidt , David Miller , Michal Simek , linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, microblaze-uclinux@itee.uq.edu.au Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1591 Lines: 46 On Tue, Feb 9, 2010 at 12:14 PM, Anton Vorontsov wrote: > On Tue, Feb 09, 2010 at 10:28:15AM -0700, Grant Likely wrote: > [...] >> Rather than having a lock at the device tree data pointer level which >> mixes usage with potentially many other drivers; wouldn't it make more >> sense to use a mutex at the of_gc subsystem context? > > I don't think so. > > of_gc = np->data; > lock(of_gc); (or lock(devtree)) > > > doesn't provide us what we need, i.e. it doesn't guarantee that > np->data (of_gc) is still alive. > > And here: > > lock(np->data); (or lock(devtree)) > of_gc = np->data; > lock(of_gc); > > > The second lock becomes useless (unless you also refcount np->data > usage and can drop the devtree/np->data lock, and grab some other > kind of lock, e.g. mutex, but this is silly). Okay, I'm convinced now. The model is wrong. struct of_gc does need to be a member of struct gpio_chip and conditionally compiled against CONFIG_OF_GPIO. This locking requirement is just too plain ugly, and dereferencing the np->data pointer in this way is dangerous (what if something that is not struct of_gc is stored there). Put of_gc into struct gpio_chip, and I'll completely support that approach. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/