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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 7:21 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Monday 18 October 2010 09:44:33 Ohad Ben-Cohen wrote:
>> +  int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags);
>> ...
>> +     The flags parameter is a pointer to where the interrupts state of the
>> +     caller will be saved at.
>
> This seems to encourage sloppy coding: The only variant you allow is the one
> that corresponds to Linux's spin_lock_irqsave()

Yes, this is a hardware requirement to minimize the period of time in
which the hwspinlock is taken, in order to prevent lengthy polling on
the interconnect (preventing it from serving other transactions).

>, which is generally discouraged
> in all places where you know if you need to disable interrupts or not.
>
> IMHO the default should be a version that only allows locks that don't get
> taken at IRQ time and consequently don't require saving the interrupt flags.

Please note that the hwspinlocks should never be used to achieve
synchronization with Linux contexts running on the same cpu - it's
always about achieving mutual exclusion with a remote processor.

So whether the lock is taken at IRQ time or not does not affect the
requirement to disable interrupts while it is taken (very differently
from local spin_lock{_irqsave} synchronizations).

Thanks,
Ohad.

>
>        Arnd
>
--
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/