[Xenomai] [RFC] [PATCH] Backport for chained interrupt handling fix - Linux 3.1.10 PowerPC

[Xenomai] [RFC] [PATCH] Backport for chained interrupt handling fix - Linux 3.1.10 PowerPC

[Jouko Haapaluoma]

Hello

We have a platform with Freescale MPC8309 and we are using Linux 3.1.10
with the adeos-ipipe patch. We stumbled upon a problem where the chained
interrupts from the QE have large interrupt latencies. The problem was
also described in this mailing list topic:
http://comments.gmane.org/gmane.linux.real-time.xenomai.users/13917

We backported the fixes suggested by Philippe and Gilles to our platform.
We used Gilles' backport for ARM as a basis (because we have pre ipipe-core
version of the I-pipe) and integrated also some changes from Philippe's patches
(which were for the ipipe-core version).

The patch of the backport is attached in this email. The large interrupt latencies were fixed
and the system seems to be stable. Currently changes were done only to MPC83xx
specific parts because other PowerPC platforms are not available for testing.

BR,
Jouko Haapaluoma
Wapice Oy
Tampere, Finland


From: Jouko Haapaluoma <jouko.haapaluoma@wapice.com>
Date: Tue, 12 May 2015 15:35:22 +0300
Subject: [PATCH] Refactor chained interrupts handling to fix latency problems

--- arch/powerpc/include/asm/ipipe.h   | 42 +++++---------------------------------
 arch/powerpc/include/asm/qe_ic.h   | 20 ++++++------------
 arch/powerpc/kernel/ipipe.c        | 13 ++++++------
 arch/powerpc/sysdev/qe_lib/qe_ic.c |  6 ++----
 4 files changed, 20 insertions(+), 61 deletions(-)

diff --git a/arch/powerpc/include/asm/ipipe.h b/arch/powerpc/include/asm/ipipe.h
index 977028d..4fc5ab2 100644
--- a/arch/powerpc/include/asm/ipipe.h
+++ b/arch/powerpc/include/asm/ipipe.h
@@ -51,6 +51,9 @@
 #define IPIPE_MINOR_NUMBER	13
 #define IPIPE_PATCH_NUMBER	6
 
+#define IPIPE_IRQF_NOACK    0x1
+#define IPIPE_IRQF_NOSYNC   0x2
+
 #ifdef CONFIG_IPIPE_WANT_PREEMPTIBLE_SWITCH
 
 struct mm;
@@ -182,14 +185,12 @@ void __ipipe_register_ipi(unsigned int irq);
 
 DECLARE_PER_CPU(struct pt_regs, __ipipe_tick_regs);
 
-void __ipipe_handle_irq(int irq, struct pt_regs *regs);
+void __ipipe_handle_irq(int irq, int flags);
 
 static inline void ipipe_handle_chained_irq(unsigned int irq)
 {
-	struct pt_regs regs;	/* dummy */
-
 	ipipe_trace_irq_entry(irq);
-	__ipipe_handle_irq(irq, &regs);
+	__ipipe_handle_irq(irq, IPIPE_IRQF_NOSYNC);
 	ipipe_trace_irq_exit(irq);
 }
 
@@ -227,30 +228,6 @@ void handle_one_irq(unsigned int irq);
 
 void check_stack_overflow(void);
 
-static inline void ipipe_pre_cascade_noeoi(struct irq_desc *desc)
-{
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	chip->irq_mask(&desc->irq_data);
-}
-
-static inline void ipipe_post_cascade_noeoi(struct irq_desc *desc)
-{
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	chip->irq_unmask(&desc->irq_data);
-}
-
-static inline void ipipe_pre_cascade_eoi(struct irq_desc *desc)
-{
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	chip->irq_eoi(&desc->irq_data); /* EOI will mask too. */
-}
-
-static inline void ipipe_post_cascade_eoi(struct irq_desc *desc)
-{
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	chip->irq_unmask(&desc->irq_data);
-}
-
 #else /* !CONFIG_IPIPE */
 
 #include <linux/interrupt.h>
@@ -261,15 +238,6 @@ static inline void ipipe_post_cascade_eoi(struct irq_desc *desc)
 
 #define ipipe_mm_switch_protect(flags)		do { (void)(flags); } while(0)
 #define ipipe_mm_switch_unprotect(flags)	do { (void)(flags); } while(0)
-#define	ipipe_pre_cascade_noeoi(desc)		do { } while (0)
-#define	ipipe_post_cascade_noeoi(desc)		do { } while (0)
-#define	ipipe_pre_cascade_eoi(desc)		do { } while (0)
-
-static inline void ipipe_post_cascade_eoi(struct irq_desc *desc)
-{
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	chip->irq_eoi(&desc->irq_data);
-}
 
 #define __ipipe_serial_debug(fmt, args...)	do { } while (0)
 
diff --git a/arch/powerpc/include/asm/qe_ic.h b/arch/powerpc/include/asm/qe_ic.h
index 9f6a441..b58da12 100644
--- a/arch/powerpc/include/asm/qe_ic.h
+++ b/arch/powerpc/include/asm/qe_ic.h
@@ -91,12 +91,8 @@ static inline void qe_ic_cascade_low_ipic(unsigned int irq,
 	struct qe_ic *qe_ic = irq_desc_get_handler_data(desc);
 	unsigned int cascade_irq = qe_ic_get_low_irq(qe_ic);
 
-	ipipe_pre_cascade_noeoi(desc);
-
 	if (cascade_irq != NO_IRQ)
 		qe_ic_cascade_irq(qe_ic, cascade_irq);
-
-	ipipe_post_cascade_noeoi(desc);
 }
 
 static inline void qe_ic_cascade_high_ipic(unsigned int irq,
@@ -105,12 +101,8 @@ static inline void qe_ic_cascade_high_ipic(unsigned int irq,
 	struct qe_ic *qe_ic = irq_desc_get_handler_data(desc);
 	unsigned int cascade_irq = qe_ic_get_high_irq(qe_ic);
 
-	ipipe_pre_cascade_noeoi(desc);
-
 	if (cascade_irq != NO_IRQ)
 		qe_ic_cascade_irq(qe_ic, cascade_irq);
-
-	ipipe_post_cascade_noeoi(desc);
 }
 
 static inline void qe_ic_cascade_low_mpic(unsigned int irq,
@@ -119,12 +111,12 @@ static inline void qe_ic_cascade_low_mpic(unsigned int irq,
 	struct qe_ic *qe_ic = irq_desc_get_handler_data(desc);
 	unsigned int cascade_irq = qe_ic_get_low_irq(qe_ic);
 
-	ipipe_pre_cascade_eoi(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 
 	if (cascade_irq != NO_IRQ)
 		qe_ic_cascade_irq(qe_ic, cascade_irq);
 
-	ipipe_post_cascade_eoi(desc);
+	chip->irq_eoi(&desc->irq_data);
 }
 
 static inline void qe_ic_cascade_high_mpic(unsigned int irq,
@@ -133,12 +125,12 @@ static inline void qe_ic_cascade_high_mpic(unsigned int irq,
 	struct qe_ic *qe_ic = irq_desc_get_handler_data(desc);
 	unsigned int cascade_irq = qe_ic_get_high_irq(qe_ic);
 
-	ipipe_pre_cascade_eoi(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 
 	if (cascade_irq != NO_IRQ)
 		qe_ic_cascade_irq(qe_ic, cascade_irq);
 
-	ipipe_post_cascade_eoi(desc);
+	chip->irq_eoi(&desc->irq_data);
 }
 
 static inline void qe_ic_cascade_muxed_mpic(unsigned int irq,
@@ -147,7 +139,7 @@ static inline void qe_ic_cascade_muxed_mpic(unsigned int irq,
 	struct qe_ic *qe_ic = irq_desc_get_handler_data(desc);
 	unsigned int cascade_irq;
 
-	ipipe_pre_cascade_eoi(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
 
 	cascade_irq = qe_ic_get_high_irq(qe_ic);
 	if (cascade_irq == NO_IRQ)
@@ -156,7 +148,7 @@ static inline void qe_ic_cascade_muxed_mpic(unsigned int irq,
 	if (cascade_irq != NO_IRQ)
 		qe_ic_cascade_irq(qe_ic, cascade_irq);
 
-	ipipe_post_cascade_eoi(desc);
+	chip->irq_eoi(&desc->irq_data);
 }
 
 #endif /* _ASM_POWERPC_QE_IC_H */
diff --git a/arch/powerpc/kernel/ipipe.c b/arch/powerpc/kernel/ipipe.c
index 7252c78..ee8c3ae 100644
--- a/arch/powerpc/kernel/ipipe.c
+++ b/arch/powerpc/kernel/ipipe.c
@@ -320,7 +320,7 @@ int ipipe_trigger_irq(unsigned irq)
 		return -EINVAL;
 #endif
 	local_irq_save_hw(flags);
-	__ipipe_handle_irq(irq, NULL);
+	__ipipe_handle_irq(irq, IPIPE_IRQF_NOACK);
 	local_irq_restore_hw(flags);
 
 	return 1;
@@ -331,14 +331,14 @@ int ipipe_trigger_irq(unsigned irq)
  * interrupt protection log is maintained here for each domain. Hw
  * interrupts are off on entry.
  */
-void __ipipe_handle_irq(int irq, struct pt_regs *regs)
+void __ipipe_handle_irq(int irq, int flags)
 {
 	struct ipipe_domain *this_domain, *next_domain;
 	struct list_head *head, *pos;
 	int m_ack;
 
 	/* Software-triggered IRQs do not need any ack. */
-	m_ack = (regs == NULL);
+	m_ack = (flags & IPIPE_IRQF_NOACK) != 0;
 
 #ifdef CONFIG_IPIPE_DEBUG
 	if (unlikely(irq >= IPIPE_NR_IRQS) ||
@@ -354,7 +354,8 @@ void __ipipe_handle_irq(int irq, struct pt_regs *regs)
 	else {
 		head = __ipipe_pipeline.next;
 		next_domain = list_entry(head, struct ipipe_domain, p_link);
-		if (likely(test_bit(IPIPE_WIRED_FLAG, &next_domain->irqs[irq].control))) {
+		if (!(flags & IPIPE_IRQF_NOSYNC)
+			&& likely(test_bit(IPIPE_WIRED_FLAG, &next_domain->irqs[irq].control))) {
 			if (!m_ack && next_domain->irqs[irq].acknowledge)
 				next_domain->irqs[irq].acknowledge(irq, irq_to_desc(irq));
 			__ipipe_dispatch_wired(next_domain, irq);
@@ -472,7 +473,7 @@ asmlinkage int __ipipe_grab_irq(struct pt_regs *regs)
 			__ipipe_ipi_demux(irq, regs);
 		else
 #endif /* CONFIG_SMP */
-			__ipipe_handle_irq(irq, regs);
+			__ipipe_handle_irq(irq, 0);
 	}
 
 	ipipe_trace_irq_exit(irq);
@@ -531,7 +532,7 @@ asmlinkage int __ipipe_grab_timer(struct pt_regs *regs)
 		 */
 		__ipipe_dispatch_wired_nocheck(head, IPIPE_TIMER_VIRQ);
 	else
-		__ipipe_handle_irq(IPIPE_TIMER_VIRQ, NULL);
+		__ipipe_handle_irq(IPIPE_TIMER_VIRQ, IPIPE_IRQF_NOACK);
 
 	ipipe_trace_irq_exit(IPIPE_TIMER_VIRQ);
 
diff --git a/arch/powerpc/sysdev/qe_lib/qe_ic.c b/arch/powerpc/sysdev/qe_lib/qe_ic.c
index d2f82df..9449e18 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_ic.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_ic.c
@@ -243,11 +243,9 @@ static void qe_ic_mask_irq(struct irq_data *d)
 
 void __ipipe_qe_ic_cascade_irq(struct qe_ic *qe_ic, unsigned int virq)
 {
-	struct pt_regs regs;    /* Contents not used. */
-
 	ipipe_trace_irq_entry(virq);
-        __ipipe_handle_irq(virq, &regs);
-        ipipe_trace_irq_exit(virq);
+	__ipipe_handle_irq(virq, IPIPE_IRQF_NOSYNC);
+	ipipe_trace_irq_exit(virq);
 }
 
 #endif
-- 
1.8.4

Re: [Xenomai] [RFC] [PATCH] Backport for chained interrupt handling fix - Linux 3.1.10 PowerPC

[Jouko Haapaluoma]

Actually this patch seems to crash the kernel from time to time if
lots of QE interrupts are handled (e.g. high Ethernet traffic).

Sometimes we get a kernel oops (same every time) and sometimes
the system seems to freeze somehow partially (user tasks
stop responding). The kernel oops is attached below.

We discovered that the latency problem is fixed and system is
stable (at least no crashes after a couple of days of continuous
test) if the patch is stripped of the changes to the file:
"arch/powerpc/kernel/ipipe.c". Therefore the only change is basically that
calls to the masking of the QE interrupt are removed
(ipipe_pre_cascade_noeoi).

That is actually what was suggested by Michael Pustylnik before:
http://www.xenomai.org/pipermail/xenomai/2012-April/025670.html
However, Philippe commented it would not work because there is a risk that
interrupts are re-enabled before "the handler" is invoked (which handler
actually?).

I have tried to understand where this actually can happen but I haven't
recognized the situation (and the system seems to be running fine).
To me it seems that the cascaded call to the __ipipe_handle_irq()
will always acknowledge (or actually mask since it is a level interrupt)
the demuxed IRQ in QE before  there are any calls to operations that
might re-enable interrupts (like syncing pipeline). If the demuxed IRQ is
masked before interrupts are re-enabled, do we have any problem?
According to QE datasheet, the interrupt request should stop if the interrupt
in the QE is masked.

If I understood correctly, the cascaded interrupt path (from QE) is like this:

1. Exception 0x501 occurs
2. __ipipe_grab_irq() <-- interrupt num 33 (QE low prio)
3. __ipipe_handle_irq() <-- handle the multiplex IRQ 33
4. qe_ic_cascade_low_ipic() <-- acknowledge handler of the multiplex IRQ
5. __ipipe_handle_irq() <-- handle the demuxed IRQ
6. __ipipe_ack_level_irq() <-- mask/ack the demuxed IRQ in QE
7. __ipipe_dispatch_wired() <-- demuxed IRQ is wired

Even if the demuxed IRQ would not be handled as wired, it would be
acked/masked before a call to __ipipe_walk_pipeline().

I know this is an old Ipipe/Linux version (adeos-ipipe-3.1.10-powerpc-2.13-07.patch)
but I'm very interested in this issue.

Philippe, could you elaborate where exactly the problem might occur?

BR,
Jouko Haapaluoma
Wapice Ltd
Tampere, Finland



*****************************

Oops: Kernel access of bad area, sig: 11 [#1]
MPC8309 UNIC2CORE
Modules linked in: flx_frs rtl2_prot rtvlan rtnetproxy rt_loopback rtpacket rtudp rt_ucc_geth rtipv4 rtnet
NIP: 00000000 LR: c0060df0 CTR: 00000000
REGS: cb4b1ba0 TRAP: 0400   Not tainted  (3.1.10-ipipe-HAL_8_1_5)
MSR: 20001032 <ME,IR,DR>  CR: 24044082  XER: 00000000
TASK = cbdceba0[1290] 'USMCoreTask' THREAD: cb4b0000
GPR00: c0060df0 cb4b1c50 cbdceba0 00000021 00000000 00000002 00000000 00000000
GPR08: 00000000 c050f240 00000000 c04d38c4 24044082 10021240 c050ee20 00009032
GPR16: ffffffff fffeffff cb4b1f50 00000000 00010000 c050ee20 00000010 c000e47c
GPR24: 00000025 c04d3888 c04f0000 c050ee20 c050a6e0 c050ee24 00000001 00000021
NIP [00000000]   (null)
LR [c0060df0] __ipipe_sync_stage+0x154/0x1f8
Call Trace:
[cb4b1c50] [c0060df0] __ipipe_sync_stage+0x154/0x1f8 (unreliable)
[cb4b1c90] [c000e810] __ipipe_handle_irq+0x124/0x178
[cb4b1cd0] [c000e8e0] __ipipe_grab_irq+0x3c/0x70
[cb4b1cf0] [c00107c4] __ipipe_ret_from_except+0x0/0xc
--- Exception: 501 at ipipe_unstall_pipeline_head+0x38/0x88
    LR = xnpod_suspend_thread+0x23c/0x298
[cb4b1db0] [c04d38c0] 0xc04d38c0 (unreliable)
[cb4b1de0] [c006f588] xnshadow_relax+0x7c/0x160
[cb4b1e80] [c00715b4] hisyscall_event+0x1d0/0x35c
[cb4b1ec0] [c00615a8] __ipipe_dispatch_event+0xac/0x1ec
[cb4b1f20] [c000ea5c] __ipipe_syscall_root+0x5c/0x140
[cb4b1f40] [c000fe60] DoSyscall+0x20/0x5c
--- Exception: c01 at 0x4849f7b4
    LR = 0x4826b9d0
Instruction dump:
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
Unable to handle kernel paging request for instruction fetch
Faulting instruction address: 0x00000000
---[ end trace e0653a9ce6b5e1c6 ]---

Re: [Xenomai] [RFC] [PATCH] Backport for chained interrupt handling fix - Linux 3.1.10 PowerPC

[Philippe Gerum]

On 05/27/2015 04:19 PM, Jouko Haapaluoma wrote:

> Oops: Kernel access of bad area, sig: 11 [#1]
> MPC8309 UNIC2CORE
> Modules linked in: flx_frs rtl2_prot rtvlan rtnetproxy rt_loopback rtpacket rtudp rt_ucc_geth rtipv4 rtnet
> NIP: 00000000 LR: c0060df0 CTR: 00000000

For some reason, the IRQ log syncer runs a NULL handler. You should try
understanding which IRQ is involved, and why this happens. At first
glance, I don't see any issue related to masking.

This might be the consequence of a domain mismatch. A related bug was
fixed a few months ago, you should at least check the IRQ log syncer in
the legacy pipeline code for this issue:
http://git.xenomai.org/ipipe.git/commit/?h=ipipe-3.18&id=3a71c688d0e33c2e1bbe3eee8cce898419043cac

I can't elaborate more, since this legacy code is pretty old and a lot
of context would need to be refreshed to discuss this properly. Hectic
schedule on my end prevents this unfortunately.

-- 
Philippe.

Re: [Xenomai] [RFC] [PATCH] Backport for chained interrupt handling fix - Linux 3.1.10 PowerPC

[Jouko Haapaluoma]

From: Philippe Gerum [mailto:rpm@xenomai.org] 
Sent: 27. toukokuuta 2015 22:44

> This might be the consequence of a domain mismatch. A related bug was
> fixed a few months ago, you should at least check the IRQ log syncer in
> the legacy pipeline code for this issue:
> http://git.xenomai.org/ipipe.git/commit/?h=ipipe-3.18&id=3a71c688d0e33c2e1bbe3eee8cce898419043cac

I tried the patch you posted. We had several boards running last night and
no crashes or freezes so far. It seems that this fixed the crashes we had
after the backported latency fix patch was applied (the one I posted before).
It also makes sense according to the kernel oops like you said.

After all, it seems that the backported latency fix was OK but we only 
saw this other bug after the latency fix was applied. 

Many thanks!

For those interested, the backported version
(adeos-ipipe-3.1.10-powerpc-2.13-07.patch) of the patch "handle
domain downgrading in slow path of IRQ syncer" can be found below in
this mail.

BR,
Jouko Haapaluoma
Wapice Ltd


From: Jouko Haapaluoma
Date: Thu, 28 May 2015 12:40:20 +0300
Subject: [PATCH] Fix domain downgrading in IRQ syncer
---
 include/linux/ipipe.h |  2 ++
 kernel/ipipe/core.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/linux/ipipe.h b/include/linux/ipipe.h
index 253dcdf..45bd89d 100644
--- a/include/linux/ipipe.h
+++ b/include/linux/ipipe.h
@@ -69,9 +69,11 @@ static inline void ipipe_context_check_off(void) { }
 #ifdef CONFIG_IPIPE_DEBUG_INTERNAL
 #define IPIPE_WARN(c)		WARN_ON(c)
 #define IPIPE_WARN_ONCE(c)	WARN_ON_ONCE(c)
+#define IPIPE_BUG_ON(c)		BUG_ON(c)
 #else
 #define IPIPE_WARN(c)		do { (void)(c); } while (0)
 #define IPIPE_WARN_ONCE(c)	do { (void)(c); } while (0)
+#define IPIPE_BUG_ON(c)		do { (void)(c); } while (0)
 #endif
 
 #ifdef CONFIG_IPIPE
diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
index 327ebff..9c096c7 100644
--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -1244,6 +1244,7 @@ void __ipipe_sync_stage(void)
 	struct ipipe_domain *ipd;
 	int irq;
 
+respin:
 	ipd = __ipipe_current_domain;
 	p = ipipe_cpudom_ptr(ipd);
 
@@ -1288,7 +1289,33 @@ void __ipipe_sync_stage(void)
 			local_irq_disable_hw();
 		}
 
+		/*
+		 * We may have migrated to a different CPU (1) upon
+		 * return from the handler, or downgraded from the
+		 * head domain to the root one (2), the opposite way
+		 * is NOT allowed though.
+		 *
+		 * (1) reload the current per-cpu context pointer, so
+		 * that we further pull pending interrupts from the
+		 * proper per-cpu log.
+		 *
+		 * (2) check the stall bit to know whether we may
+		 * dispatch any interrupt pending for the root domain,
+		 * and respin the entire dispatch loop if
+		 * so. Otherwise, immediately return to the caller,
+		 * _without_ affecting the stall state for the root
+		 * domain, since we do not own it at this stage.  This
+		 * case is basically reflecting what may happen in
+		 * dispatch_irq_head() for the fast path.
+		 */
+
 		p = ipipe_cpudom_ptr(__ipipe_current_domain);
+		if (__ipipe_current_domain != ipd) {
+			IPIPE_BUG_ON(ipd == ipipe_root_domain);
+			if (test_bit(IPIPE_STALL_FLAG, &p->status))
+				return;
+			goto respin;
+		}
 	}
 
 	if (ipd == ipipe_root_domain)
-- 
1.8.4