Thanks for all the answers.
I had done a mistype in the pseudo code, in the i2c_read/write
functions it was supposed to unlock lock 1 (this is how it was in my
real code, should have used that one I guess and just cut away
anything unnecessary).
The reason why I need locks in i2c_read/2rie is that these might be
called from other places than ust avr_read and interrupt.
Anyway I took the advice to serialize and to use a local variable for
the flag instead of a global one and now it work. I guess the reason
why the interrupt disabling didn't seem to work was that I somehow
messed the flag up.
Thanks again.
/Leon
On Mon, Oct 27, 2008 at 6:25 PM, Johannes Weiner <hannes@xxxxxxxxxxxx> wrote:
> "Leon Ljunggren" <leon.ljunggren@xxxxxxxxx> writes:
>
>> Hi,
>> I working on a driver for a embedded system running Linux 2.6.20.3 and
>> I'm having a bit of problem getting spin_lock to work as I'd expect it
>> to.
>>
>> The driver reads from an AVR and before it does that one need do a
>> write to tell it were to read is to take place. The problem is that I
>> have a interupt function that will also perform such a read and if it
>> interupts the user induced read after i2c_write but before i2c_read it
>> can cause the read to happen in the wrong place.
>>
>> To ensure that this doesn't happen I'm trying to use
>> spin_lock_irqsave, but I'm getting rather unexpected results. First of
>> all it doesn't seem to disable the interupt, the interupt function is
>> still called and secondly it just seems to ignore the lock. Even if
>> avr_read has aquired lock2 and then gets interupted (which it
>> shouldn't since interupts should be disabled, right?) avr_interupt
>> will not care and still call the write/read functions instead of
>> blocking as expected.
>>
>> Any pointers to what I might be doing wrong?
>>
>> Thanks
>> /Leon Ljunggren
>>
>> ----- pseudo code ----
>>
>> spinlock_t lock1 = SPIN_LOCK_UNLOCKED;
>> spinlock_t lock2 = SPIN_LOCK_UNLOCKED;
>>
>> i2c_read()
>> {
>> spin_lock_irqsave(&lock1, lock1_flag);
>> ...
>> spin_unlock_irqrestore(&lock2, lock2_flag);
>> }
>>
>> i2c_write()
>> {
>> spin_lock_irqsave(&lock1, lock1_flag);
>> ...
>> spin_unlock_irqrestore(&lock2, lock2_flag);
>> }
>
> Where do you unlock lock1? And why do you have 2 locks at all?
>
>> i2c_irq_read()
>> {
>> spin_lock_irqsave(&lock1, lock1_flag);
>> ...
>> spin_unlock_irqrestore(&lock2, lock2_flag);
>> }
>>
>> i2c_irq_write()
>> {
>> spin_lock_irqsave(&lock1, lock1_flag);
>> ...
>> spin_unlock_irqrestore(&lock2, lock2_flag);
>> }
>>
>> avr_read()
>> {
>> spin_lock_irqsave(&lock2, lock2_flag);
>> i2c_write();
>
> After that, lock2 is unlocked, you re-enabled irqs and lock1 is taken.
> At this time an interrupt may fire and deadlock on lock1 (because there
> is no way, the spinlock is unlocked by the interrupted context). This
> makes not much sense to me.
>
> What you basically want is serializing a write->read sequence between
> process and interrupt context, right? So you need one lock these two
> paths serialize against:
>
> spin_lock_t avr_lock = UNLOCKED
>
> avr_read()
> {
> spin_lock_irqsave(&avr_lock, flags)
> i2c_write()
> i2c_read()
> spin_unlock_irqrestore(&avr_lock, flags)
> }
>
> avr_interrupt()
> {
> avr_read()
> }
>
> where i2c_read/write don't do any extra locking for that matter.
>
> If avr_read() is already serialized by a higher order lock with respect
> to concurrent user processes, local_irq_save()/_restore() would also be
> enough. But with the spin lock versions you should be on the safe side.
>
> HTH,
>
> Hannes
>
--
To unsubscribe from this list: send an email with
"unsubscribe kernelnewbies" to ecartis@xxxxxxxxxxxx
Please read the FAQ at http://kernelnewbies.org/FAQ
