libcr: Begone with PRE_RUNNABLE

7 files changed, 73 insertions, 78 deletions

diff --git a/libcr/coroutine.c b/libcr/coroutine.c
index 6549070..a468df0 100644
--- a/libcr/coroutine.c
+++ b/libcr/coroutine.c
@@ -155,7 +155,7 @@
 /*====================================================================
  * Interrupt management routines.  */
 #if __unix__
-	#include <signal.h> /* for sig*, SIG_* */
+	#include <signal.h> /* for sig*, SIG* */
 
 	/* For a signal to be *in* the mask means that the signal is
 	 * *blocked*.  */
@@ -166,10 +166,11 @@
 		sigfillset(&set);
 		sigprocmask(SIG_SETMASK, &set, NULL);
 	}
-	void _cr_plat_disable_interrupts(void) {
-		sigset_t all;
+	bool _cr_plat_save_and_disable_interrupts(void) {
+		sigset_t all, old;
 		sigfillset(&all);
-		sigprocmask(SIG_SETMASK, &all, NULL);
+		sigprocmask(SIG_SETMASK, &all, &old);
+		return !sigismember(&old, SIGRTMIN);
 	}
 	void _cr_plat_enable_interrupts(void) {
 		sigset_t zero;
@@ -181,8 +182,13 @@
 	static ALWAYS_INLINE void cr_plat_wait_for_interrupt(void) {
 		asm volatile ("wfi":::"memory");
 	}
-	void _cr_plat_disable_interrupts(void) {
-		asm volatile ("cpsid i":::"memory");
+	bool _cr_plat_save_and_disable_interrupts(void) {
+		uint32_t primask;
+		asm volatile ("mrs %0, PRIMASK\n"
+		              "cpsid i"
+		              : /* %0 */"=r"(primask)
+		              );
+		return primask == 0;
 	}
 	void _cr_plat_enable_interrupts(void) {
 		asm volatile ("cpsie i":::"memory");
@@ -273,8 +279,6 @@ enum coroutine_state {
 	CR_NONE = 0,     /* this slot in the table is empty */
 	CR_INITIALIZING, /* running, before cr_begin() */
 	CR_RUNNING,      /* running, after cr_begin() */
-	CR_PRE_RUNNABLE, /* running, after cr_unpause_from_intrhandler()
-	                  *          but before cr_pause() */
 	CR_RUNNABLE,     /* not running, but runnable */
 	CR_PAUSED,       /* not running, and not runnable */
 };
@@ -301,7 +305,6 @@ const char *coroutine_state_strs[] = {
 	[CR_NONE]         = "CR_NONE",
 	[CR_INITIALIZING] = "CR_INITIALIZING",
 	[CR_RUNNING]      = "CR_RUNNING",
-	[CR_PRE_RUNNABLE] = "CR_PRE_RUNNABLE",
 	[CR_RUNNABLE]     = "CR_RUNNABLE",
 	[CR_PAUSED]       = "CR_PAUSED",
 };
@@ -472,6 +475,11 @@ cid_t coroutine_add_with_stack_size(size_t stack_size,
 	assert_cid_state(child, state == CR_RUNNABLE);
 	if (parent)
 		assert_cid_state(parent, state == CR_RUNNING);
+	/* Restore interrupts because cr_begin() disables interrupts
+	 * before the context switch.  XXX: This assumes that
+	 * interrupts were enabled when _add() was called, which we
+	 * didn't actually check.  */
+	cr_restore_interrupts(true);
 	coroutine_running = parent;
 
 	return child;
@@ -484,9 +492,10 @@ cid_t coroutine_add(const char *name, cr_fn_t fn, void *args) {
 
 /* coroutine_main() ***********************************************************/
 
-void coroutine_main(void) {
+__attribute__ ((noreturn)) void coroutine_main(void) {
 	debugf("coroutine_main()\n");
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
+	assert(saved);
 	coroutine_running = 0;
 	for (;;) {
 		cid_t next;
@@ -518,12 +527,13 @@ void cr_begin(void) {
 	debugf("cid=%zu: cr_begin()\n", coroutine_running);
 	assert_cid_state(coroutine_running, state == CR_INITIALIZING);
 
+	bool saved = cr_save_and_disable_interrupts();
 	coroutine_table[coroutine_running-1].state = CR_RUNNABLE;
 	coroutine_ringbuf_push(coroutine_running);
 	coroutine_table[coroutine_running-1].sp = cr_plat_get_sp();
 	if (!cr_plat_setjmp(coroutine_table[coroutine_running-1].env)) /* point=c1 */
 		cr_plat_longjmp(coroutine_add_env, 1); /* jump to point=a */
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 }
 
 static inline void _cr_yield() {
@@ -551,37 +561,33 @@ void cr_yield(void) {
 	debugf("cid=%zu: cr_yield()\n", coroutine_running);
 	assert_cid_state(coroutine_running, state == CR_RUNNING);
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	coroutine_table[coroutine_running-1].state = CR_RUNNABLE;
 	coroutine_ringbuf_push(coroutine_running);
 	_cr_yield();
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 }
 
 void cr_pause_and_yield(void) {
 	debugf("cid=%zu: cr_pause_and_yield()\n", coroutine_running);
-	assert_cid_state(coroutine_running, state == CR_RUNNING || state == CR_PRE_RUNNABLE);
-
-	cr_disable_interrupts();
-	if (coroutine_table[coroutine_running-1].state == CR_PRE_RUNNABLE) {
-		coroutine_table[coroutine_running-1].state = CR_RUNNABLE;
-		coroutine_ringbuf_push(coroutine_running);
-	} else
-		coroutine_table[coroutine_running-1].state = CR_PAUSED;
+	assert_cid_state(coroutine_running, state == CR_RUNNING);
+
+	bool saved = cr_save_and_disable_interrupts();
+	coroutine_table[coroutine_running-1].state = CR_PAUSED;
 	_cr_yield();
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 }
 
-void cr_exit(void) {
+__attribute__ ((noreturn)) void cr_exit(void) {
 	debugf("cid=%zu: cr_exit()\n", coroutine_running);
 	assert_cid_state(coroutine_running, state == CR_RUNNING);
 
-	cr_disable_interrupts();
+	(void)cr_save_and_disable_interrupts();
 	coroutine_table[coroutine_running-1].state = CR_NONE;
 	cr_plat_longjmp(coroutine_main_env, 1); /* jump to point=b */
 }
 
-void cr_unpause(cid_t cid) {
+void cr_unpause_from_intrhandler(cid_t cid) {
 	debugf("cr_unpause(%zu)\n", cid);
 	assert_cid_state(cid, state == CR_PAUSED);
 
@@ -589,19 +595,10 @@ void cr_unpause(cid_t cid) {
 	coroutine_ringbuf_push(cid);
 }
 
-void cr_unpause_from_intrhandler(cid_t cid) {
-	debugf("cr_unpause_from_intrhandler(%zu)\n", cid);
-	assert_cid_state(cid, state == CR_RUNNING || state == CR_PAUSED);
-
-	if (coroutine_table[cid-1].state == CR_RUNNING) {
-		assert(cid == coroutine_running);
-		debugf("... raced, deferring unpause\n");
-		coroutine_table[cid-1].state = CR_PRE_RUNNABLE;
-	} else {
-		debugf("... actual unpause\n");
-		coroutine_table[cid-1].state = CR_RUNNABLE;
-		coroutine_ringbuf_push(cid);
-	}
+void cr_unpause(cid_t cid) {
+	bool saved = cr_save_and_disable_interrupts();
+	cr_unpause_from_intrhandler(cid);
+	cr_restore_interrupts(saved);
 }
 
 cid_t cr_getcid(void) {
diff --git a/libcr/include/libcr/coroutine.h b/libcr/include/libcr/coroutine.h
index 95f9ba0..8aeab7e 100644
--- a/libcr/include/libcr/coroutine.h
+++ b/libcr/include/libcr/coroutine.h
@@ -94,12 +94,8 @@ cid_t coroutine_add(const char *name, cr_fn_t fn, void *args);
 
 /**
  * The main scheduler loop.
- *
- * "Should" never return, but will print a message and return if there
- * are no coroutines (there were no calls to coroutine_add(), or all
- * coroutines cr_exit()).
  */
-void coroutine_main(void);
+__attribute__ ((noreturn)) void coroutine_main(void);
 
 /* inside of coroutines *******************************************************/
 
@@ -132,34 +128,33 @@ cid_t cr_getcid(void);
  *
  * This is fast on bare-metal, but slow on an OS (because on an OS it
  * uses a syscall).
+ *
+ * Returns whether interrupts were enabled before the call.
  */
-#define cr_disable_interrupts() do {           \
-		_cr_plat_disable_interrupts(); \
-		asm volatile ("":::"memory");  \
-	} while (0)
-void _cr_plat_disable_interrupts(void);
+#define cr_save_and_disable_interrupts() ({                                \
+		bool was_enabled = _cr_plat_save_and_disable_interrupts(); \
+		asm volatile ("":::"memory");                              \
+		was_enabled;                                               \
+	})
+bool _cr_plat_save_and_disable_interrupts(void);
 
 /**
- * Enable interrupts.  Any "pending" interrupts that came in while
+ * Re-enable interrupts.  Any "pending" interrupts that came in while
  * interrupts were disabled will have their handlers called.
  *
  * This is fast on bare-metal, but slow on an OS (because on an OS it
  * uses a syscall).
  */
-#define cr_enable_interrupts() do {           \
-		asm volatile ("":::"memory"); \
-		_cr_plat_enable_interrupts(); \
+#define cr_restore_interrupts(enable) do {            \
+		asm volatile ("":::"memory");         \
+		if (enable)                           \
+			_cr_plat_enable_interrupts(); \
 	} while (0)
 void _cr_plat_enable_interrupts(void);
 
 /**
  * cr_unpause_from_intrhandler() is like cr_unpause(), but safe to
- * call from a interrupt handler that might race with the coroutine
- * actually pausing itself.
- *
- * It is also safe to call from a regular coroutine, but compared to
- * regular cr_unpause() it is less capable of detecting programming
- * errors.  So don't do that?
+ * call from a interrupt handler.
  */
 void cr_unpause_from_intrhandler(cid_t);
 
diff --git a/libcr_ipc/include/libcr_ipc/sema.h b/libcr_ipc/include/libcr_ipc/sema.h
index 9958364..0895a22 100644
--- a/libcr_ipc/include/libcr_ipc/sema.h
+++ b/libcr_ipc/include/libcr_ipc/sema.h
@@ -38,14 +38,14 @@ typedef struct {
 static inline void cr_sema_signal(cr_sema_t *sema) {
 	assert(sema);
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	sema->cnt++;
 	if (sema->waiters.front) {
 		sema->cnt--;
 		cr_unpause(sema->waiters.front->cid);
 		_cr_ipc_sll_pop_from_front(&sema->waiters);
 	}
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 }
 
 /**
@@ -74,18 +74,17 @@ static inline void cr_sema_signal_from_intrhandler(cr_sema_t *sema)  {
 static inline void cr_sema_wait(cr_sema_t *sema) {
 	assert(sema);
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	if (sema->cnt) { /* non-blocking */
 		sema->cnt--;
-		cr_enable_interrupts();
 	} else { /* blocking */
 		struct _cr_sema_waiter self = {
 			.cid    = cr_getcid(),
 		};
 		_cr_ipc_sll_push_to_rear(&sema->waiters, &self);
-		cr_enable_interrupts();
 		cr_pause_and_yield();
 	}
+	cr_restore_interrupts(saved);
 }
 
 #endif /* _LIBCR_IPC_SEMA_H_ */
diff --git a/libhw/host_alarmclock.c b/libhw/host_alarmclock.c
index 4f43fc7..1d26f1a 100644
--- a/libhw/host_alarmclock.c
+++ b/libhw/host_alarmclock.c
@@ -110,7 +110,7 @@ static bool hostclock_add_trigger(implements_alarmclock *_alarmclock,
 	trigger->cb         = cb;
 	trigger->cb_arg     = cb_arg;
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	struct alarmclock_trigger **dst = &alarmclock->queue;
 	while (*dst && fire_at_ns >= (*dst)->fire_at_ns)
 		dst = &(*dst)->next;
@@ -145,7 +145,7 @@ static bool hostclock_add_trigger(implements_alarmclock *_alarmclock,
 		if (timer_settime(alarmclock->timer_id, TIMER_ABSTIME, &alarmspec, NULL) != 0)
 			error(1, errno, "timer_settime");
 	}
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 
 	return false;
 }
@@ -158,7 +158,7 @@ static void hostclock_del_trigger(implements_alarmclock *_alarmclock,
 	assert(alarmclock);
 	assert(trigger);
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	if (trigger->alarmclock == alarmclock) {
 		if (!trigger->prev)
 			alarmclock->queue = trigger->next;
@@ -170,5 +170,5 @@ static void hostclock_del_trigger(implements_alarmclock *_alarmclock,
 		trigger->prev = NULL;
 		trigger->next = NULL;
 	}
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 }
diff --git a/libhw/rp2040_hwtimer.c b/libhw/rp2040_hwtimer.c
index 6f37d85..9f0d901 100644
--- a/libhw/rp2040_hwtimer.c
+++ b/libhw/rp2040_hwtimer.c
@@ -115,7 +115,7 @@ static bool rp2040_hwtimer_add_trigger(implements_alarmclock     *_alarmclock,
 	trigger->cb         = cb;
 	trigger->cb_arg     = cb_arg;
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	struct alarmclock_trigger **dst = &alarmclock->queue;
 	while (*dst && fire_at_ns >= (*dst)->fire_at_ns)
 		dst = &(*dst)->next;
@@ -135,14 +135,15 @@ static bool rp2040_hwtimer_add_trigger(implements_alarmclock     *_alarmclock,
 		/* Force the interrupt handler to trigger as soon as
 		 * we enable interrupts.  This handles the case of
 		 * when fire_at_ns is before when we called
-		 * cr_disable_interrupts().  We could check
+		 * cr_save_and_disable_interrupts().  We could check
 		 * timer_time_us_64() again after calling
-		 * cr_disable_interrupts() and do this conditionally,
-		 * but I don't think that would be any more efficient
-		 * than just letting the interrupt fire.  */
+		 * cr_save_and_disable_interrupts() and do this
+		 * conditionally, but I don't think that would be any
+		 * more efficient than just letting the interrupt
+		 * fire.  */
 		hw_set_bits(&timer_hw->intf, 1 << alarmclock->alarm_num);
 	}
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 
 	return false;
 }
@@ -154,7 +155,7 @@ static void rp2040_hwtimer_del_trigger(implements_alarmclock     *_alarmclock,
 	assert(alarmclock);
 	assert(trigger);
 
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	if (trigger->alarmclock == alarmclock) {
 		if (!trigger->prev)
 			alarmclock->queue = trigger->next;
@@ -166,5 +167,5 @@ static void rp2040_hwtimer_del_trigger(implements_alarmclock     *_alarmclock,
 		trigger->prev = NULL;
 		trigger->next = NULL;
 	}
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 }
diff --git a/libhw/w5500.c b/libhw/w5500.c
index 08486d4..5678ffd 100644
--- a/libhw/w5500.c
+++ b/libhw/w5500.c
@@ -340,14 +340,14 @@ void _w5500_init(struct w5500 *chip,
 	w5500_hard_reset(chip);
 
 	/* Finally, wire in the interrupt handler.  */
-	cr_disable_interrupts();
+	bool saved = cr_save_and_disable_interrupts();
 	for (size_t i = 0; i < ARRAY_LEN(w5500_chips); i++) {
 		if (w5500_chips[i] == NULL) {
 			w5500_chips[i] = chip;
 			break;
 		}
 	}
-	cr_enable_interrupts();
+	cr_restore_interrupts(saved);
 	coroutine_add("w5500_irq", w5500_irq_cr, chip);
 }
 
diff --git a/libhw_generic/alarmclock.c b/libhw_generic/alarmclock.c
index 3364f72..a16f2f6 100644
--- a/libhw_generic/alarmclock.c
+++ b/libhw_generic/alarmclock.c
@@ -15,7 +15,10 @@ static void alarmclock_sleep_intrhandler(void *_arg) {
 }
 
 void alarmclock_sleep_until_ns(implements_alarmclock *clock, uint64_t abstime_ns) {
+	bool saved = cr_save_and_disable_interrupts();
 	cid_t cid = cr_getcid();
 	struct alarmclock_trigger trigger;
 	VCALL(clock, add_trigger, &trigger, abstime_ns, alarmclock_sleep_intrhandler, &cid);
+	cr_pause_and_yield();
+	cr_restore_interrupts(saved);
 }