port-arm/44965: Interrupts not being handled correctly on Kirkwood systems

[dmills%63bit.com@localhost]

>Number:         44965
>Category:       port-arm
>Synopsis:       Interrupts not being handled correctly on Kirkwood systems
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    port-arm-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri May 13 23:15:00 +0000 2011
>Originator:     Dave Mills
>Release:        NetBSD current 5.99.51
>Organization:
>Environment:
System: NetBSD 5.99.51 on evbarm Marvell orion/kirkwood systems
>Description:
It was reported in the port-arm mailing list using the serial port was causing 
the soc to hang.  This is due to incorrect handling of interrupts(not just the 
UART interrupts):

On the kirkwood series of chips there are two interrupt cause registers - a low 
one and a high one.
kirkwood.c splits the high and low register up into two pics with the high 
register pic being chained off the low register pic on BIT0 (MainHighSum) this 
is incorrect as masking off this bit does not stop interrupts on the high 
register from firing.  The correct way of doing this is to have one pic for 
both registers.

The reason the serial console hangs is that the processor is trying to execute 
atomic functions (atomic_or_32) in pic_mark_pending_sources which restart when 
returning from an interrupt (LOCK_CAS) and since the high register interrupt is 
not being masked off the atomic functions never complete as they are constantly 
being interrupted with interrupts from the UART.

Most of the time this isnt an issue as the current design establishes the "high 
register interrupt" as IPL_HIGH and is allowed to run the high register 
interrupt handler and block the relevant high interrupt bit - however when 
running other IPL_HIGH code it just masks off BIT0 (MainHighSum) in the low 
register and marks it as pending and doesnt immediately run the high register 
interrupt handler. 
>How-To-Repeat:
Although the issue will occur randomly through general use outputting a large 
amount of data to the serial port will quickly cause the the system to hang as 
the system spends more time in the UART interrupt handling code.  I was using 
"cat /etc/*" originally to provoke the failure however "cat /dev/urandom | 
strings" is probably a better way of doing it as /etc contains some binary 
files.
>Fix:
I have written a patch

The code uses a single pic for low and high registers:

Index: kirkwood.c
===================================================================
RCS file: /cvsroot/src/sys/arch/arm/marvell/kirkwood.c,v
retrieving revision 1.2
diff -u -r1.2 kirkwood.c
--- kirkwood.c 30 Oct 2010 06:37:49 -0000 1.2
+++ kirkwood.c 10 Mar 2011 15:44:32 -0000
@@ -49,12 +49,8 @@
 
 static void kirkwood_intr_init(void);
 
-static void kirkwood_pic_unblock_low_irqs(struct pic_softc *, size_t, 
uint32_t);
-static void kirkwood_pic_unblock_high_irqs(struct pic_softc *, size_t,
-   uint32_t);
-static void kirkwood_pic_block_low_irqs(struct pic_softc *, size_t, uint32_t);
-static void kirkwood_pic_block_high_irqs(struct pic_softc *, size_t, uint32_t);
-static int kirkwood_pic_find_pending_high_irqs(struct pic_softc *);
+static void kirkwood_pic_unblock_irqs(struct pic_softc *, size_t, uint32_t);
+static void kirkwood_pic_block_irqs(struct pic_softc *, size_t, uint32_t);
 static void kirkwood_pic_establish_irq(struct pic_softc *, struct intrsource 
*);
 static void kirkwood_pic_source_name(struct pic_softc *, int, char *, size_t);
 
@@ -80,31 +76,18 @@
     "Reserved(60)",    "Reserved(61)",    "Reserved(62)",    "Reserved(63)"
 };
 
-static struct pic_ops kirkwood_picops_low = {
- .pic_unblock_irqs = kirkwood_pic_unblock_low_irqs,
- .pic_block_irqs = kirkwood_pic_block_low_irqs,
+static struct pic_ops kirkwood_picops = {
+ .pic_unblock_irqs = kirkwood_pic_unblock_irqs,
+ .pic_block_irqs = kirkwood_pic_block_irqs,
  .pic_establish_irq = kirkwood_pic_establish_irq,
  .pic_source_name = kirkwood_pic_source_name,
 };
-static struct pic_ops kirkwood_picops_high = {
- .pic_unblock_irqs = kirkwood_pic_unblock_high_irqs,
- .pic_block_irqs = kirkwood_pic_block_high_irqs,
- .pic_find_pending_irqs = kirkwood_pic_find_pending_high_irqs,
- .pic_establish_irq = kirkwood_pic_establish_irq,
- .pic_source_name = kirkwood_pic_source_name,
-};
-static struct pic_softc kirkwood_pic_low = {
- .pic_ops = &kirkwood_picops_low,
- .pic_maxsources = 32,
- .pic_name = "kirkwood_low",
-};
-static struct pic_softc kirkwood_pic_high = {
- .pic_ops = &kirkwood_picops_high,
- .pic_maxsources = 32,
- .pic_name = "kirkwood_high",
+static struct pic_softc kirkwood_pic = {
+ .pic_ops = &kirkwood_picops,
+ .pic_maxsources = 64,
+ .pic_name = "kirkwood",
 };
 
-
 /*
  * kirkwood_intr_bootstrap:
  *
@@ -141,12 +124,7 @@
  extern struct pic_softc mvsoc_bridge_pic;
  void *ih;
 
- pic_add(&kirkwood_pic_low, 0);
-
- pic_add(&kirkwood_pic_high, 32);
- ih = intr_establish(KIRKWOOD_IRQ_HIGH, IPL_HIGH, IST_LEVEL_HIGH,
-    pic_handle_intr, &kirkwood_pic_high);
- KASSERT(ih != NULL);
+ pic_add(&kirkwood_pic, 0);
 
  pic_add(&mvsoc_bridge_pic, 64);
  ih = intr_establish(KIRKWOOD_IRQ_BRIDGE, IPL_HIGH, IST_LEVEL_HIGH,
@@ -158,55 +136,32 @@
 
 /* ARGSUSED */
 static void
-kirkwood_pic_unblock_low_irqs(struct pic_softc *pic, size_t irqbase,
+kirkwood_pic_unblock_irqs(struct pic_softc *pic, size_t irqbase,
       uint32_t irq_mask)
 {
-
- write_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR,
-    read_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR) | irq_mask);
+ if (irqbase == 0)
+ write_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR,
+     read_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR) | irq_mask);
+ else if (irqbase == 32)
+ write_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR,
+     read_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR) | irq_mask);
+ else
+ KASSERT(0);
 }
 
 /* ARGSUSED */
 static void
-kirkwood_pic_unblock_high_irqs(struct pic_softc *pic, size_t irqbase,
-       uint32_t irq_mask)
-{
-
- write_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR,
-    read_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR) | irq_mask);
-}
-
-/* ARGSUSED */
-static void
-kirkwood_pic_block_low_irqs(struct pic_softc *pic, size_t irqbase,
+kirkwood_pic_block_irqs(struct pic_softc *pic, size_t irqbase,
     uint32_t irq_mask)
 {
-
- write_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR,
-    read_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR) & ~irq_mask);
-}
-
-/* ARGSUSED */
-static void
-kirkwood_pic_block_high_irqs(struct pic_softc *pic, size_t irqbase,
-     uint32_t irq_mask)
-{
-
- write_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR,
-    read_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR) & ~irq_mask);
-}
-
-static int
-kirkwood_pic_find_pending_high_irqs(struct pic_softc *pic)
-{
- uint32_t pending;
-
- pending = read_mlmbreg(KIRKWOOD_MLMB_MICHR) &
-    read_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR);
- if (pending == 0)
- return 0;
- pic_mark_pending_sources(pic, 0, pending);
- return 1;
+ if (irqbase == 0)
+ write_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR,
+     read_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR) & ~irq_mask);
+ else if (irqbase == 32)
+ write_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR,
+     read_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR) & ~irq_mask);
+ else
+ KASSERT(0);
 }
 
 /* ARGSUSED */
@@ -229,14 +184,25 @@
 static int
 kirkwood_find_pending_irqs(void)
 {
- uint32_t pending;
+ uint32_t pendinglow, causelowreg;
+ uint32_t pendinghigh = 0, ipl_masklow = 0, ipl_maskhigh = 0;
 
- pending = read_mlmbreg(KIRKWOOD_MLMB_MICLR) &
+ causelowreg = read_mlmbreg(KIRKWOOD_MLMB_MICLR);
+ pendinglow = causelowreg &
     read_mlmbreg(KIRKWOOD_MLMB_MIRQIMLR);
- if (pending == 0)
- return 0;
 
- return pic_mark_pending_sources(&kirkwood_pic_low, 0, pending);
+ if ((causelowreg & KIRKWOOD_IRQ_HIGH)==KIRKWOOD_IRQ_HIGH)
+ {
+ pendinghigh = read_mlmbreg(KIRKWOOD_MLMB_MICHR) &
+ read_mlmbreg(KIRKWOOD_MLMB_MIRQIMHR);
+ }
+
+ if (pendinglow != 0)
+ ipl_masklow = pic_mark_pending_sources(&kirkwood_pic, 0, pendinglow);
+ if (pendinghigh !=0)
+ ipl_maskhigh = pic_mark_pending_sources(&kirkwood_pic, 32, pendinghigh);
+
+ return (ipl_masklow | ipl_maskhigh);
 }
 
 /*