Subject: Re: Race condition in generic soft interrupts code?
To: Naoto Shimazaki <igy@arhc.org>
From: Richard Earnshaw <rearnsha@arm.com>
List: port-arm
Date: 07/17/2003 15:11:27
> At Mon, 14 Jul 2003 22:50:08 +0100,
> Richard Earnshaw wrote:
> > It then enters the loop below that scans over hwpend bits.  While 
> > processing the statclock interrupt the SPL is effectively raised to 
> > IPL_STAT_CLOCK with
> > 
> > 		current_spl_level |= iq->iq_mask;
> > 		oldirqstate = enable_interrupts(I32_bit);
> > 
> > Interrupts are then enabled and the statclock handler code is called:
> > 
> > 		for (ih = TAILQ_FIRST(&iq->iq_list); ih != NULL;
> > 		     ih = TAILQ_NEXT(ih, ih_list)) {
> > 			(void) (*ih->ih_func)(ih->ih_arg ? ih->ih_arg : frame);
> > 		}
> 
> I think all of lower priority interrupts should masked by ICU hardware
> before handlers are invoked.  If they masked by hardware,
> XXX_intr_dispatch is never called by lower interrupts, and the
> following code become not needed.

I'm not sure I agree.   It takes quite a few cycles to block interrupts, 
especially if they are controlled by a memory-mapped device.  If we work 
on the principle that the a machine spends roughly 1% of the time in 
interrupt mode and that interrupt sources are statistically independent, 
then 99 out of every 100 interrupt calls would end up messing 
unnecessarily with the interrupt masks.  That sounds to me like an 
overhead that would be good to avoid.  The basic framework does this 
(modulo this problem under discussion).

I'm not entirely sure, but I think that if you did that, you'd also end up 
having to update the interrupt masks every time spl_xxx() or splx() was 
called.  That will significantly affect performance.

> 
> > 		if (pcpl & ibit) {
> > 			/*
> > 			 * IRQ is masked; mark it as pending and check
> > 			 * the next one.  Note: the IRQ is already disabled.
> > 			 */
> > 			ifpga_ipending |= ibit;
> > 			continue;
> > 		}
> 
> ixp12x0_intr_dispatch() in ixp12x0_intr.c does it.  It simply mask the
> lower interrupts before calling handler and simply restore mask before
> leaving intr_dispatch().  If lower interrupts are occurred during
> handler running, ixp12x0_intr_dispatch() is immediately called when
> mask is restored.

That shouldn't be necessary.

It seems that this "bug" has been found by several people porting this 
code, and in each case a different hack has been used to work around it.  
No-one else has posted about the issue here, which means that several 
ports may still be suffering quietly from the same problem.  That's a 
pity: we have this list for a reason... let's make better use of it!

R.