Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

[Ohad Ben-Cohen]

On Thu, Oct 21, 2010 at 11:04 AM, Arnd Bergmann <arnd@xxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/