Re: [PATCH 2.6.16] Shared interrupts sometimes lost

[Pavel Machek <pavel@suse.cz>]

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.