----Next_Part(Sat_Jul_24_10:08:56_2004_589)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi folks,

Our interrupt handler pending list has issue that it has
possibilities to link same handler twice, since we use same value
0 to indicate the list end and a interrupt handler in-use flag.

I found this issue about two years ago, and then I has been used
attached patch (intr_pend-fix1.diff). This patch simply changes
the list end value from 0 to -1.

It seems that OpenBSD fixed this issue a year ago to introduce
ih_busy in interrupt handler. Here is the commit log of OpenBSD.

| Changes by: henric at cvs.openbsd.org 2003/03/20 16:05:30
| 
| Modified files:
|         sys/arch/sparc64/include: cpu.h
|         sys/arch/sparc64/sparc64: genassym.cf intr.c locore.s
| 
| Log message:
| The current code tries to use the same field in the interrupt handler as
| both a "next" pointer for a singly-linked list and as an in-use flag.
| This obviously does not work all that well. This change adds a separate
| ih_busy flag to mark the handler as in-use, leaving ih_pending for use by
| the list code.

I adopt this changes to -current, then make a patch attached
(intr_pend-fix2.diff).

I think the latter patch is easy to understand and some more
changes. So I would like to commit this patch. Commnets?

Note: attached patches can apply cleanly both to netbsd-1-6 and
to netbsd-2-0.

-- Takeshi Nakayama

----Next_Part(Sat_Jul_24_10:08:56_2004_589)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="intr_pend-fix1.diff"

--- sparc64/locore.s.orig	Thu Jul 22 13:09:23 2004
+++ sparc64/locore.s	Thu Jul 22 13:08:07 2004
@@ -3984,7 +3984,7 @@
 	.data
 	.globl	intrpending
 intrpending:
-	.space	16 * 8 * PTRSZ
+	.space	16 * 8 * PTRSZ, -1
 
 #ifdef DEBUG
 #define INTRDEBUG_VECTOR	0x1
@@ -4443,9 +4443,9 @@
 1:
 	membar	#StoreLoad		! Make sure any failed casxa insns complete
 	LDPTR	[%l4], %l2		! Check a slot
-	brz,pn	%l2, intrcmplt		! Empty list?
-
-	 clr	%l7
+	cmp	%l2, -1
+	beq,pn	CCCR, intrcmplt		! Empty list?
+	 mov	-1, %l7
 	membar	#LoadStore
 	CASPTR	[%l4] ASI_N, %l2, %l7	! Grab the entire list
 	cmp	%l7, %l2
@@ -4468,7 +4468,8 @@
 	stx	%g0, [%l1]		! Clear intr source
 	membar	#Sync			! Should not be needed
 0:
-	brnz,pn	%l7, 2b			! 'Nother?
+	cmp	%l7, -1
+	bne,pn	CCCR, 2b		! 'Nother?
 	 mov	%l7, %l2
 
 #else /* INTRLIST */

----Next_Part(Sat_Jul_24_10:08:56_2004_589)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="intr_pend-fix2.diff"

--- include/cpu.h.orig	Sat Jun 19 20:26:30 2004
+++ include/cpu.h	Thu Jul 22 13:53:31 2004
@@ -293,8 +293,9 @@
 	short			ih_number;	/* interrupt number */
 						/* the H/W provides */
 	char			ih_pil;		/* interrupt priority */
+	volatile char		ih_busy;	/* handler is on list */
 	struct intrhand		*ih_next;	/* global list */
-	struct intrhand		*ih_pending;	/* interrupt queued */
+	struct intrhand		*ih_pending;	/* pending list */
 	volatile u_int64_t	*ih_map;	/* Interrupt map reg */
 	volatile u_int64_t	*ih_clr;	/* clear interrupt reg */
 };
--- sparc64/clock.c.orig	Thu Mar 18 09:55:50 2004
+++ sparc64/clock.c	Thu Jul 22 13:59:04 2004
@@ -649,6 +649,7 @@
 	schedint.ih_pil = PIL_SCHED;
 	schedint.ih_clr = NULL;
 	schedint.ih_arg = 0;
+	schedint.ih_busy = 0;
 	schedint.ih_pending = 0;
 	schedhz = stathz/4;
 
--- sparc64/genassym.cf.orig	Sun Mar 28 17:44:32 2004
+++ sparc64/genassym.cf	Thu Jul 22 13:54:40 2004
@@ -256,6 +256,7 @@
 define	IH_ARG		offsetof(struct intrhand, ih_arg)
 define	IH_NUMBER	offsetof(struct intrhand, ih_number)
 define	IH_PIL		offsetof(struct intrhand, ih_pil)
+define	IH_BUSY		offsetof(struct intrhand, ih_busy)
 define	IH_PEND		offsetof(struct intrhand, ih_pending)
 define	IH_NEXT		offsetof(struct intrhand, ih_next)
 define	IH_MAP		offsetof(struct intrhand, ih_map)
--- sparc64/intr.c.orig	Tue Nov 11 11:45:12 2003
+++ sparc64/intr.c	Thu Jul 22 13:58:48 2004
@@ -243,6 +243,7 @@
 	 * and we do want to preserve order.
 	 */
 	ih->ih_pil = level; /* XXXX caller should have done this before */
+	ih->ih_busy = 0;    /* XXXX caller should have done this before */
 	ih->ih_pending = 0; /* XXXX caller should have done this before */
 	ih->ih_next = NULL;
 
@@ -317,6 +318,7 @@
 	ih->ih_fun = (int (*) __P((void *)))fun;	/* XXX */
 	ih->ih_arg = arg;
 	ih->ih_pil = level;
+	ih->ih_busy = 0;
 	ih->ih_pending = 0;
 	ih->ih_clr = NULL;
 	return (void *)ih;
--- sparc64/locore.s.orig	Thu Jul 22 13:09:23 2004
+++ sparc64/locore.s	Thu Jul 22 14:12:43 2004
@@ -4076,7 +4076,8 @@
 
 setup_sparcintr:
 #ifdef	INTR_INTERLOCK
-	LDPTR	[%g5+IH_PEND], %g6	! Read pending flag
+	ldstub  [%g5+IH_BUSY], %g6	! Check if already in use
+	membar #LoadLoad | #LoadStore
 	brnz,pn	%g6, ret_from_intr_vector ! Skip it if it's running
 #endif
 	 ldub	[%g5+IH_PIL], %g6	! Read interrupt mask
@@ -4452,21 +4453,37 @@
 	bne,pn	%icc, 1b
 	 add	%sp, CC64FSZ+STKB, %o2	! tf = %sp + CC64FSZ + STKB
 2:
+	LDPTR	[%l2 + IH_PEND], %l7	! Load next pending
 	LDPTR	[%l2 + IH_FUN], %o4	! ih->ih_fun
 	LDPTR	[%l2 + IH_ARG], %o0	! ih->ih_arg
+	LDPTR	[%l2 + IH_CLR], %l1	! ih->ih_clear
+
+	STPTR	%g0, [%l2 + IH_PEND]	! Unlink from list
+
+	! Note that the function handler itself or an interrupt
+	! may add handlers to the pending pending. This includes
+	! the current entry in %l2 and entries held on our local
+	! pending list in %l7.  The former is ok because we are
+	! done with it now and the latter because they are still
+	! marked busy. We may also be able to do this by having
+	! the soft interrupts use a variation of the hardware
+	! interrupts' ih_clr scheme.  Note:  The busy flag does
+	! not itself prevent the handler from being entered
+	! recursively.  It only indicates that the handler is
+	! about to be invoked and that it should not be added
+	! to the pending table.
+	membar	#StoreStore | #LoadStore
+	stb	%g0, [%l2 + IH_BUSY]	! Allow the ih to be reused
+
+	! At this point, the current ih could already be added
+	! back to the pending table.
 
 	jmpl	%o4, %o7		! handled = (*ih->ih_fun)(...)
 	 movrz	%o0, %o2, %o0		! arg = (arg == 0) ? arg : tf
-	LDPTR	[%l2 + IH_PEND], %l7	! Clear pending flag
-	LDPTR	[%l2 + IH_CLR], %l1
-	membar	#LoadStore
-	STPTR	%g0, [%l2 + IH_PEND]	! Clear pending flag
-	membar	#Sync
 
 	brz,pn	%l1, 0f
-	 add	%l5, %o0, %l5
+	 add	%l5, %o0, %l5		! Add handler return value
 	stx	%g0, [%l1]		! Clear intr source
-	membar	#Sync			! Should not be needed
 0:
 	brnz,pn	%l7, 2b			! 'Nother?
 	 mov	%l7, %l2
@@ -11632,15 +11649,21 @@
  */
 ENTRY(send_softint)
 	rdpr	%pil, %g1	! s = splx(level)
+#if 1
+	wrpr	%g0, PIL_HIGH, %pil
+#else
 	cmp	%g1, %o1
 	bge,pt	%icc, 1f
 	 nop
 	wrpr	%o1, 0, %pil
+#endif
 1:
 	brz,pn	%o2, 1f
-	 set	intrpending, %o3
-	LDPTR	[%o2 + IH_PEND], %o5
-	mov	8, %o4			! Number of slots to search
+	 mov	8, %o4			! Number of slots to search
+	set	intrpending, %o3
+
+	ldstub	[%o2 + IH_BUSY], %o5
+	membar #LoadLoad | #LoadStore
 #ifdef INTR_INTERLOCK
 	brnz	%o5, 1f
 #endif

----Next_Part(Sat_Jul_24_10:08:56_2004_589)----