NetBSD Problem Report #44965

From www@NetBSD.org  Fri May 13 23:11:43 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 8C9E463C606
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 13 May 2011 23:11:43 +0000 (UTC)
Message-Id: <20110513231142.8104463C504@www.NetBSD.org>
Date: Fri, 13 May 2011 23:11:42 +0000 (UTC)
From: dmills@63bit.com
Reply-To: dmills@63bit.com
To: gnats-bugs@NetBSD.org
Subject: Interrupts not being handled correctly on Kirkwood systems
X-Send-Pr-Version: www-1.0

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri May 13 23:15:00 +0000 2011
>Closed-Date:    Wed Aug 10 19:17:56 +0000 2011
>Last-Modified:  Wed Aug 10 19:17:56 +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);
 }

 /* 


>Release-Note:

>Audit-Trail:
From: "Matt Thomas" <matt@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44965 CVS commit: src/sys/arch/arm/marvell
Date: Tue, 24 May 2011 17:45:49 +0000

 Module Name:	src
 Committed By:	matt
 Date:		Tue May 24 17:45:49 UTC 2011

 Modified Files:
 	src/sys/arch/arm/marvell: kirkwood.c

 Log Message:
 Merge in the patch in PR/44965 with some improvements (removing ifs when
 possible).


 To generate a diff of this commit:
 cvs rdiff -u -r1.3 -r1.4 src/sys/arch/arm/marvell/kirkwood.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 10 Aug 2011 15:10:07 +0000
State-Changed-Why:
Is this fixed?


From: Dave Mills <dmills@63bit.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: port-arm/44965 (Interrupts not being handled correctly on Kirkwood
 systems)
Date: Wed, 10 Aug 2011 20:11:37 +0100

 On 10/08/2011 16:10, dholland@NetBSD.org wrote:
 > Synopsis: Interrupts not being handled correctly on Kirkwood systems
 >
 > State-Changed-From-To: open->feedback
 > State-Changed-By: dholland@NetBSD.org
 > State-Changed-When: Wed, 10 Aug 2011 15:10:07 +0000
 > State-Changed-Why:
 > Is this fixed?
 >
 >
 >
 It seems to be working correctly now

State-Changed-From-To: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 10 Aug 2011 19:17:56 +0000
State-Changed-Why:
Confirmed fixed; thanks for the quick response.


>Unformatted:

Home	PR Database Search