From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver Date: Thu, 21 Oct 2010 11:04:28 +0200 Message-ID: <201010211104.28555.arnd@arndb.de> References: <1287387875-14168-1-git-send-email-ohad@wizery.com> <201010192308.02018.arnd@arndb.de> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Ohad Ben-Cohen Cc: linux-arm-kernel@lists.infradead.org, Hari Kanigeri , Suman Anna , Benoit Cousson , Tony Lindgren , Greg KH , linux-kernel@vger.kernel.org, Grant Likely , akpm@linux-foundation.org, linux-omap@vger.kernel.org, "Krishnamoorthy, Balaji T" List-Id: linux-omap@vger.kernel.org On Thursday 21 October 2010, Ohad Ben-Cohen wrote: > This sounds like adding a set of API that resembles spin_{unlock,lock}_irq. > > My gut feeling here is that while this may be useful and simple at > certain places, it is somewhat error prone; a driver which would > erroneously use this at the wrong place will end up enabling > interrupts where it really shouldn't. > > Don't you feel that proving a simple spin_lock_irqsave-like API is > actually safer and less error prone ? > > I guess that is one of the reasons why spin_lock_irqsave is much more > popular than spin_lock_irq - people just know it will never screw up. People can screw that up in different ways, e.g. spin_lock_irqsave(&lock_a, flags); spin_lock_irqsave(&lock_b, flags); /* overwrites flags */ spin_lock_irqrestore(&lock_b, flags); spin_lock_irqrestore(&lock_a, flags); /* still disabled! */ I use the presence of spin_lock_irqsave in driver code as an indication of whether the author had any clue about locking. Most experienced coders use the right version intuitively, while beginners often just use _irqsave because they didn't understand the API and they were told that using this is safe. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Thu, 21 Oct 2010 11:04:28 +0200 Subject: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver In-Reply-To: References: <1287387875-14168-1-git-send-email-ohad@wizery.com> <201010192308.02018.arnd@arndb.de> Message-ID: <201010211104.28555.arnd@arndb.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thursday 21 October 2010, Ohad Ben-Cohen wrote: > This sounds like adding a set of API that resembles spin_{unlock,lock}_irq. > > My gut feeling here is that while this may be useful and simple at > certain places, it is somewhat error prone; a driver which would > erroneously use this at the wrong place will end up enabling > interrupts where it really shouldn't. > > Don't you feel that proving a simple spin_lock_irqsave-like API is > actually safer and less error prone ? > > I guess that is one of the reasons why spin_lock_irqsave is much more > popular than spin_lock_irq - people just know it will never screw up. People can screw that up in different ways, e.g. spin_lock_irqsave(&lock_a, flags); spin_lock_irqsave(&lock_b, flags); /* overwrites flags */ spin_lock_irqrestore(&lock_b, flags); spin_lock_irqrestore(&lock_a, flags); /* still disabled! */ I use the presence of spin_lock_irqsave in driver code as an indication of whether the author had any clue about locking. Most experienced coders use the right version intuitively, while beginners often just use _irqsave because they didn't understand the API and they were told that using this is safe. Arnd