From cf683d9866cd12205a1882fceacd44a123e18ec3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Roussel?= Date: Tue, 26 Aug 2014 14:28:01 +0200 Subject: [PATCH 1/4] Fix thread_yield by avoiding the (too) early re-enablement of IRQ, that is: before the newly selected thread's context is totally restored --- cpu/msp430-common/cpu.c | 11 +++++++---- cpu/msp430-common/include/cpu.h | 13 ++++++++++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/cpu/msp430-common/cpu.c b/cpu/msp430-common/cpu.c index 6442f873e2..d6c8c9e0e0 100644 --- a/cpu/msp430-common/cpu.c +++ b/cpu/msp430-common/cpu.c @@ -7,6 +7,7 @@ */ #include "cpu.h" +#include "irq.h" #include "kernel.h" #include "kernel_internal.h" #include "sched.h" @@ -20,12 +21,14 @@ void thread_yield(void) { __save_context(); - dINT(); + /* disable IRQ, remembering if they are + to be reactivated after context switch */ + unsigned int irqen = disableIRQ(); + /* have sched_active_thread point to the next thread */ sched_run(); - eINT(); - __restore_context(); + __restore_context(irqen); } NORETURN void cpu_switch_context_exit(void) @@ -33,7 +36,7 @@ NORETURN void cpu_switch_context_exit(void) sched_active_thread = sched_threads[0]; sched_run(); - __restore_context(); + __restore_context(GIE); UNREACHABLE(); } diff --git a/cpu/msp430-common/include/cpu.h b/cpu/msp430-common/include/cpu.h index d8e304fd75..adadfdc719 100644 --- a/cpu/msp430-common/include/cpu.h +++ b/cpu/msp430-common/include/cpu.h @@ -99,9 +99,20 @@ inline void __save_context(void) __save_context_isr(); } -inline void __restore_context(void) +inline void __restore_context(unsigned int irqen) { __restore_context_isr(); + + /* we want to enable if appropriate IRQs *just after* + quitting the interrupt handler; to that end, + we change the GIE bit in the value to be restored + in R2 (a.k.a. SR) by the next RETI instruction */ + if (irqen) { + __asm__("bis.w #8, 0(r1)"); + } else { + __asm__("bic.w #8, 0(r1)"); + } + __asm__("reti"); } From 3df5a2745f2535f95b80cbe9e3b12384051ace21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Roussel?= Date: Tue, 26 Aug 2014 14:50:40 +0200 Subject: [PATCH 2/4] Prevent the compiler to add an unwanted prologue in thread_yield() --- cpu/msp430-common/cpu.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cpu/msp430-common/cpu.c b/cpu/msp430-common/cpu.c index d6c8c9e0e0..1b755db760 100644 --- a/cpu/msp430-common/cpu.c +++ b/cpu/msp430-common/cpu.c @@ -17,7 +17,10 @@ volatile int __inISR = 0; char __isr_stack[MSP430_ISR_STACK_SIZE]; -void thread_yield(void) +/* we must prevent the compiler to generate a prologue or an epilogue + for thread_yield(), since we rely on RETI instruction at the end + of its execution, in inlined __restore_context() sub-function */ +__attribute__((naked)) void thread_yield(void) { __save_context(); @@ -29,6 +32,8 @@ void thread_yield(void) sched_run(); __restore_context(irqen); + + UNREACHABLE(); } NORETURN void cpu_switch_context_exit(void) From 4421de003bfc3ccce77106c3074744b292af41d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Roussel?= Date: Tue, 26 Aug 2014 16:50:45 +0200 Subject: [PATCH 3/4] Fix the race condition when an interruption occured at the beginning of thread_yield(), i.e.: just after SR(R2) was pushed but before the rest of the suspended thread's context was pushed! --- cpu/msp430-common/cpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpu/msp430-common/cpu.c b/cpu/msp430-common/cpu.c index 1b755db760..e9dde7266d 100644 --- a/cpu/msp430-common/cpu.c +++ b/cpu/msp430-common/cpu.c @@ -22,12 +22,12 @@ char __isr_stack[MSP430_ISR_STACK_SIZE]; of its execution, in inlined __restore_context() sub-function */ __attribute__((naked)) void thread_yield(void) { - __save_context(); - /* disable IRQ, remembering if they are to be reactivated after context switch */ unsigned int irqen = disableIRQ(); + __save_context(); + /* have sched_active_thread point to the next thread */ sched_run(); From a691d0798a1bdd7ff606f209d96d800013ab5085 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Roussel?= Date: Tue, 9 Sep 2014 12:16:26 +0200 Subject: [PATCH 4/4] Fixed comments --- cpu/msp430-common/cpu.c | 19 ++++++++++++------- cpu/msp430-common/include/cpu.h | 13 ++++++++----- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/cpu/msp430-common/cpu.c b/cpu/msp430-common/cpu.c index e9dde7266d..b332d2b46b 100644 --- a/cpu/msp430-common/cpu.c +++ b/cpu/msp430-common/cpu.c @@ -1,6 +1,7 @@ /* - * Copyright (C) 2013, Freie Universitaet Berlin (FUB). All rights reserved. - + * Copyright (C) 2014, Freie Universitaet Berlin (FUB) & INRIA. + * All rights reserved. + * * This file is subject to the terms and conditions of the GNU Lesser * General Public License v2.1. See the file LICENSE in the top level * directory for more details. @@ -17,13 +18,17 @@ volatile int __inISR = 0; char __isr_stack[MSP430_ISR_STACK_SIZE]; -/* we must prevent the compiler to generate a prologue or an epilogue - for thread_yield(), since we rely on RETI instruction at the end - of its execution, in inlined __restore_context() sub-function */ +/* + * we must prevent the compiler to generate a prologue or an epilogue + * for thread_yield(), since we rely on the RETI instruction at the end + * of its execution, in the inlined __restore_context() sub-function + */ __attribute__((naked)) void thread_yield(void) { - /* disable IRQ, remembering if they are - to be reactivated after context switch */ + /* + * disable IRQ, remembering if they are + * to be reactivated after context switch + */ unsigned int irqen = disableIRQ(); __save_context(); diff --git a/cpu/msp430-common/include/cpu.h b/cpu/msp430-common/include/cpu.h index adadfdc719..c2c8fef269 100644 --- a/cpu/msp430-common/include/cpu.h +++ b/cpu/msp430-common/include/cpu.h @@ -1,5 +1,6 @@ /* - * Copyright (C) 2013, Freie Universitaet Berlin (FUB). All rights reserved. + * Copyright (C) 2014, Freie Universitaet Berlin (FUB) & INRIA. + * All rights reserved. * * This file is subject to the terms and conditions of the GNU Lesser * General Public License v2.1. See the file LICENSE in the top level @@ -103,10 +104,12 @@ inline void __restore_context(unsigned int irqen) { __restore_context_isr(); - /* we want to enable if appropriate IRQs *just after* - quitting the interrupt handler; to that end, - we change the GIE bit in the value to be restored - in R2 (a.k.a. SR) by the next RETI instruction */ + /* + * we want to enable appropriate IRQs *just after* + * quitting the interrupt handler; to that end, + * we change the GIE bit in the value to be restored + * in R2 (a.k.a. SR) by the next RETI instruction + */ if (irqen) { __asm__("bis.w #8, 0(r1)"); } else {