Well, here goes - hope I don't put my foot in my mouth anywhere below ...

Michael Hitch wrote:

> In dec_3max_intr(), the interrupt bits are
> extracted and masked to get the active interrupt request.  They are
> then dispatched in two switch statements - which will work as long as
> there is only a single interrupt active in the group of interrupts in
> each switch arguement.  If two or more interrupts are active, the switch
> statement will fail, and the interrupt will not be processed.

Thanks!  Now that I know what's broken, I fixed it as in the attached context
diff.  The MI SCSI kernel boots now, and my keyboard and mouse are no longer
broken, either!

Analysis:

While the original code looks confused, what with the ifound variable and
the do loop, it really isn't (though it *is* broken, as Michael points out).
The original author is clearly looking for speed, trying to bum a few in-
structions out of the interrupt dispatch code by using a couple of switch
statements rather than the chain of if statements in my version.

In fact, though, the switch() hack is wrongheaded, in that it becomes more
and more expensive if two or more interrupts are pending.

Since this *is* interrupt code (translation:  it gets executed *a lot*), it
is worth some time to inspect the generated code to make sure we aren't hur-
ting ourselves too much by doing things the new way.

I won't bother with the details here, partly because I'm no expert and any-
thing I say about the generated code might be wrong.  But, due to some extra
instructions that appear to be generated due to the declaration of ifound
inside an if block, the old switch()-based code is actually slower than the
chained-if code I have below, both in getting to the first interrupt and in
getting to the last interrupt in the chain.

Conclusion:

The proposed code is faster in all cases.  Additionally, it's logically
simpler, straightforward, and doesn't use hard-coded knowledge of where
the interrupt bits live in the csr.

While I was at it, I removed the unnecessary do from inside the CALLINTR
macro - I useta know why folks did that, but I've forgotten and its not
necessary here in any case.

Terry R. Friedrichsen

terry@venus.sunquest.com



*** dec_3max.c.dist	Mon Mar  6 05:13:58 2000
--- dec_3max.c.trf	Tue Mar  7 21:13:18 2000
***************
*** 234,244 ****
  
  
  #define CALLINTR(vvv)						\
! 	do {							\
! 		ifound = 1;					\
  		intrcnt[vvv] += 1;				\
  		(*intrtab[vvv].ih_func)(intrtab[vvv].ih_arg);	\
! 	} while (0)
  
  static int
  dec_3max_intr(cpumask, pc, status, cause)
--- 234,243 ----
  
  
  #define CALLINTR(vvv)						\
! 	{							\
  		intrcnt[vvv] += 1;				\
  		(*intrtab[vvv].ih_func)(intrtab[vvv].ih_arg);	\
! 	}
  
  static int
  dec_3max_intr(cpumask, pc, status, cause)
***************
*** 278,306 ****
  	_splset(MIPS_SR_INT_IE | (status & MIPS_INT_MASK_1));
  
  	if (cpumask & MIPS_INT_MASK_0) {
!                 int ifound;
! 
!                 do {
!                         ifound = 0;
!                         csr = *(u_int32_t *)MIPS_PHYS_TO_KSEG1(KN02_SYS_CSR);
!                         csr &= (csr >> KN02_CSR_IOINTEN_SHIFT);
!                         switch (csr & 0xf0) {
!                         case KN02_IP_DZ:
!                                 CALLINTR(SYS_DEV_SCC0); break;
!                         case KN02_IP_LANCE:
!                                 CALLINTR(SYS_DEV_LANCE); break;
!                         case KN02_IP_SCSI:
!                                 CALLINTR(SYS_DEV_SCSI); break;
!                         }
!                         switch (csr & 0x0f) {
!                         case KN02_IP_SLOT2:
!                                 CALLINTR(SYS_DEV_OPT2); break;
!                         case KN02_IP_SLOT1:
!                                 CALLINTR(SYS_DEV_OPT1); break;
!                         case KN02_IP_SLOT0:
!                                 CALLINTR(SYS_DEV_OPT0); break;
!                         }
!                 } while (ifound);
  	}
  	if (cpumask & MIPS_INT_MASK_3) {
  		intrcnt[ERROR_INTR]++;
--- 277,296 ----
  	_splset(MIPS_SR_INT_IE | (status & MIPS_INT_MASK_1));
  
  	if (cpumask & MIPS_INT_MASK_0) {
!                 csr = *(u_int32_t *)MIPS_PHYS_TO_KSEG1(KN02_SYS_CSR);
!                 csr &= (csr >> KN02_CSR_IOINTEN_SHIFT);
!                 if (csr & KN02_IP_DZ)
!                         CALLINTR(SYS_DEV_SCC0);
!                 if (csr & KN02_IP_LANCE)
!                         CALLINTR(SYS_DEV_LANCE);
!                 if (csr & KN02_IP_SCSI)
!                         CALLINTR(SYS_DEV_SCSI);
!                 if (csr & KN02_IP_SLOT2)
!                         CALLINTR(SYS_DEV_OPT2);
!                 if (csr & KN02_IP_SLOT1)
!                         CALLINTR(SYS_DEV_OPT1);
!                 if (csr & KN02_IP_SLOT0)
!                         CALLINTR(SYS_DEV_OPT0);
  	}
  	if (cpumask & MIPS_INT_MASK_3) {
  		intrcnt[ERROR_INTR]++;