On 01.06.2012 08:30, Michel D?nzer wrote:
> On Fre, 2012-06-01 at 08:19 +0200, Michel D?nzer wrote:
>> I think this might introduce a race condition:
>>
>> Thread 0 Thread 1
>> -------- --------
>> atomic_inc_return() returns 1
>> spin_lock_irqsave()
>> atomic_dec_and_test()
>> radeon_irq_set()
>>
>> => the interrupt won't be enabled.
> Hrmm, I messed up the formatting there, let me try one more time:
>
> Thread 0 Thread 1
> -------- --------
> atomic_inc_return() returns 1
> spin_lock_irqsave()
> atomic_dec_and_test()
> radeon_irq_set()
>
>
Nope that isn't a problem, cause what you really get in your example is:
Thread 0 Thread 1
-------- --------
atomic_inc_return() returns 1
spin_lock_irqsave()
atomic_dec_and_test()
radeon_irq_set()
spin_unlock_irqrestore()
spin_lock_irqsave()
radeon_irq_set()
spin_unlock_irqrestore()
So testing the atomic counters just determines if we need an update of
the irq registers or not, and since a significant change will always
trigger an update we can make sure that the irq regs are always set to
the last known state. We might call radeon_irq_set more often than
necessary, but that won't hurt us and is really unlikely.
Also I have found the real reason why using the atomic for preventing ih
recursion didn't worked as expected - it was just a stupid typo in my
patch. But thanks for the comment anyway, it got me to look into the
right direction for the bug.
Cheers,
Christian.
