On Fre, 2012-06-01 at 12:44 +0200, Christian König wrote:
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.
Yeah, I also realized in the meantime that the race can't happen. I blame it on lack of caffeine. :)
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.
Glad to hear that!