Re: [PATCH 2.6.16] Shared interrupts sometimes lost

Pavel Machek
2006-04-11T17:07:25Z

Hi!

> This is my first real introduction to the IRQ handling code in Linux,
> so please forgive any little errors. I'm fairly sure the big picture
> is right, partly because the patch helps so much.
>
> To explain what I think is happening, let me start with a very simple
> case. A number of PCI devices (this one included) have a number of
> events which can trigger an interrupt. The events which are current
> are presented as bits in a register, and are ORed together (and
> possibly masked by another register) to make the IRQ line.
> When 1's are written to any bits in this register, it acknowledges
> the event and clears the bit.
> A typical code fragment is
> events = read_register(INTERRUPTS);
> write_register(INTERRUPTS, events);
> ... handle each 1 bits in events ....
>
> This would normally clear all pending events and cause the interrupt
> line to go low (or at least to not be asserted).
>
> However there is room for a race here. If an event occurs between
> the read and the write, then this will NOT de-assert the IRQ line.
> It will remain asserted throughout.
>
> Now if the IRQ is handled as an edge-triggered line (which I believe
> they are in Linux), then losing this race will mean that we don't see
> any more interrupts on this line.

I believe that

a) any shared interrupts should be level-triggered. It is not okay to
share edge-triggered interrupt

b) your patch does not fix that issue. It only makes race window
smaller.

> if (!(action->flags & SA_INTERRUPT))
> local_irq_enable();
>
> do {
> ret = action->handler(irq, action->dev_id, regs);
> - if (ret == IRQ_HANDLED)
> + if (ret == IRQ_HANDLED) {
> status |= action->flags;
> + repeat = 1;
> + }
> retval |= ret;
> action = action->next;
> + if (!action &&
> + repeat &&
> + safeirq &&
> + (actionlist->flags & SA_SHIRQ)) {
> + /* at least one handler on the list did something,
> + * and the interrupt is sharable, so give
> + * every handler another chance, incase a new event
> + * came in and is holding the irq line asserted.
> + */
> + action = actionlist;
> + repeat = 0;
> + }
> } while (action);

I think it is still racy. What if another interrupt comes here?

> if (status & SA_SAMPLE_RANDOM)

Pavel
--
Thanks for all the (sleeping) penguins.