linux-kernel - Re: [PATCH 1/3] drivers: misc: add omap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 21 Oct 2010 12:13:42 +0200
From:	Ohad Ben-Cohen <ohad@...ery.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-arm-kernel@...ts.infradead.org,
	Hari Kanigeri <h-kanigeri2@...com>, Suman Anna <s-anna@...com>,
	Benoit Cousson <b-cousson@...com>,
	Tony Lindgren <tony@...mide.com>, Greg KH <greg@...ah.com>,
	linux-kernel@...r.kernel.org,
	Grant Likely <grant.likely@...retlab.ca>,
	akpm@...ux-foundation.org, linux-omap@...r.kernel.org,
	"Krishnamoorthy, Balaji T" <balajitk@...com>
Subject: Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

On Thu, Oct 21, 2010 at 11:04 AM, Arnd Bergmann <arnd@...db.de> wrote:
> 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.

Sure...

> 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.

Agree.

I'll add the _irq pair of APIs, this will make the users' code
cleaner. This is valuable especially since all of our current users
have their interrupts enabled anyway.

The change is also pretty trivial:

* move the internal locking implementation to raw_ methods
* the raw_ methods would save the current interrupt state only if
given a placeholder
* wrap those raw_ methods with the desired API (but only support the
_irq and _irqsave flavors)

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/