tidy, comments

7 files changed, 294 insertions, 73 deletions

diff --git a/9psrv.c b/9psrv.c
index 5b8cf82..489b33e 100644
--- a/9psrv.c
+++ b/9psrv.c
@@ -4,8 +4,6 @@
 #include "net9p.h"
 
 int main() {
-	coroutine_init();
-
 	if (!coroutine_add(net9p_listen_cr, NULL))
 		error(1, 0, "coroutine_add(net9p_listen_cr, NULL)");
 
diff --git a/cfg_limits.h b/cfg_limits.h
deleted file mode 100644
index 40df3e0..0000000
--- a/cfg_limits.h
+++ /dev/null
@@ -1,15 +0,0 @@
-/* cfg_limits.h - Compile-time hard-coded limits
- *
- * Copyright (C) 2024  Luke T. Shumaker <lukeshu@lukeshu.com>
- * SPDX-Licence-Identifier: AGPL-3.0-or-later
- */
-
-#ifndef _CFG_LIMITS_H_
-#define _CFG_LIMITS_H_
-
-#define COROUTINE_NUM        5
-#define COROUTINE_STACK_SIZE 0x100000 /* 1MiB */
-
-#define DEBUG 1
-
-#endif /* _CFG_LIMITS_H_ */
diff --git a/coroutine.c b/coroutine.c
index 34d84c8..9a53731 100644
--- a/coroutine.c
+++ b/coroutine.c
@@ -1,4 +1,4 @@
-/* coroutine.c - Simple coroutine and request/response implementation
+/* coroutine.c - Simple embeddable coroutine implementation
  *
  * Copyright (C) 2024  Luke T. Shumaker <lukeshu@lukeshu.com>
  * SPDX-Licence-Identifier: AGPL-3.0-or-later
@@ -10,15 +10,106 @@
 #include <assert.h>
 #include <setjmp.h>
 
-#include "cfg_limits.h"
 #include "coroutine.h"
 
+/* Configuration **************************************************************/
+
+#define COROUTINE_NUM           5
+#define COROUTINE_MEASURE_STACK 1
+
+/* Implementation *************************************************************/
+
+/*
+ * Portability notes:
+ *
+ * - It uses GCC `__attribute__`s, though these can likely be easily
+ *   swapped for other compilers.
+ *
+ * - It has a small bit of CPU-specific code (assembly, and definition
+ *   of a STACK_GROWS_DOWNWARD={0,1} macro) in
+ *   coroutine.c:call_with_stack().  Other than this, it should be
+ *   portable to other CPUs.  It currently contains implementations
+ *   for __x86_64__ and __arm__, and should be fairly easy to add
+ *   implementations for other CPUs.
+ *
+ * - It uses setjmp()/longjmp() in "unsafe" ways.  POSIX-2017
+ *   longjmp(3) says
+ *
+ *   > If the most recent invocation of setjmp() with the
+ *   > corresponding jmp_buf ... or if the function containing the
+ *   > invocation of setjmp() has terminated execution in the interim,
+ *   > or if the invocation of setjmp() was within the scope of an
+ *   > identifier with variably modified type and execution has left
+ *   > that scope in the interim, the behavior is undefined.
+ *
+ *   We use longjmp() both of these scenarios, but make it OK by using
+ *   call_with_stack() to manage the stack ourselves, assuming the
+ *   sole reason that longjmp() behavior is undefined in such cases is
+ *   because the stack that its saved stack-pointer points to is no
+ *   longer around.  It seems absurd that an implementation would
+ *   choose to do something else, but I'm calling it out here because
+ *   you never know.
+ *
+ *   Note that setjmp()/longjmp() are defined in 3 places: in the libc
+ *   (glibc/newlib), as GCC intrinsics, and the lower-level GCC
+ *   __builtin_{setjmp,newlib} which the libc and intrinsic versions
+ *   likely use.  Our assumptions seem to be valid for
+ *   x86_64-pc-linux-gnu/gcc-14.2.1/glibc-2.40 and
+ *   arm-none-eabi/gcc-14.1.0/newlib-4.4.0.
+ *
+ * Why not use <ucontext.h>, the now-deprecated (was in POSIX.1-2001,
+ * is gone in POSIX-2008) predecesor to <setjmp.h>?  It would let us
+ * do this without any assembly or unsafe assumptions.  Simply:
+ * because newlib does not provide it.
+ */
+
+/*
+ * Design decisions and notes:
+ *
+ * - Coroutines are launched with a zeroed-out stack.  Because all
+ *   stack variables should be initialized before they are read, this
+ *   "shouldn't" make a difference, but: (1) Initializing it to a
+ *   known value allows us to measure how much of the stack was
+ *   written to, which is helpful to tune stack sizes.  (2) Leaving it
+ *   uninitialized just gives me the willies.
+ *
+ * - Because embedded programs should be adverse to using the heap,
+ *   COROUTINE_NUM is fixed, instead of having coroutine_add()
+ *   dynamically grow the coroutine_table as-needed.
+ *
+ * - On the flip-side, coroutine stacks are allocated on the heap
+ *   instead of having them be statically-allocated along with
+ *   coroutine_table.  (1) This reduced the blast-area of damage for a
+ *   stack-overflow; and indeed if the end of the stack alignes with a
+ *   page-boundary memory-protection can even detect the overflow for
+ *   us.  (2) Having different-looking addresses for stack-area vs
+ *   static-area is handy for making things jump out at you when
+ *   debugging.  (3) Given the above about wanting a zeroed-out stack,
+ *   this allows us to take advantage of optimizations in calloc()
+ *   instead of using memset, and this can likely also improve things
+ *   with being page-aligned.
+ *
+ * - Coroutines must use cr_exit() instead of returning because if
+ *   they return then they will return to call_with_stack() in
+ *   coroutine_add() (not to after the longjmp() call in
+ *   coroutine_main()), and besides being
+ *   wrong-for-our-desired-flow-control, that's a stack location that
+ *   no longer exists.
+ *
+ * Things to consider changing:
+ *
+ * - Consider having _cr_transition() go ahead and find the next
+ *   coroutine to run and longjmp() direcly to it, instead of first
+ *   jumping back to coroutine_main().  This could save a few cycles
+ *   and a few bytes.
+ */
+
 enum coroutine_state {
-	CR_NONE = 0,
-	CR_INITIALIZING,
-	CR_RUNNING,
-	CR_RUNNABLE,
-	CR_PAUSED,
+	CR_NONE = 0,     /* this slot in the table is empty */
+	CR_INITIALIZING, /* running, before cr_begin() */
+	CR_RUNNING,      /* running, after cr_begin() */
+	CR_RUNNABLE,     /* not running, but runnable */
+	CR_PAUSED,       /* not running, and not runnable */ 
 };
 
 struct coroutine {
@@ -33,13 +124,12 @@ static cid_t            coroutine_running              = 0;
 static jmp_buf          coroutine_add_env;
 static jmp_buf          coroutine_main_env;
 
-void coroutine_init(void) {}
-
 static void call_with_stack(void *stack, cr_fn_t fn, void *args) {
 	static void *saved_sp = NULL;
 
-	/* This only really exists for running on ARM-32, but being
-	 * able to run it on x86-64 is useful for debugging.  */
+	/* As part of sbc-harness, this only really needs to support
+	 * ARM-32, but being able to run it on x86-64 is useful for
+	 * debugging.  */
 #if __x86_64__
 #define STACK_GROWS_DOWNWARD 1
 	asm volatile ("movq %%rsp , %0\n\t"    /* saved_sp = sp */
@@ -56,11 +146,14 @@ static void call_with_stack(void *stack, cr_fn_t fn, void *args) {
 	              );
 #elif __arm__
 #define STACK_GROWS_DOWNWARD 1
+	/* str/ldr can only work with a "lo" register, which sp is
+	 * not, so we use r0 as an intermediate because we're going to
+	 * clobber it with args anyway.  */
 	asm volatile ("mov r0, sp\n\t" /* [saved_sp = sp */
-	              "str r0, %0\n\t" /* ]  */
+	              "str r0, %0\n\t" /* ] */
 	              "mov sp, %1\n\t" /* [sp = stack] */
-	              "mov r0, %1\n\t" /* [fn(arg0) */
-	              "blx %2\n\t"     /* ] */
+	              "mov r0, %1\n\t" /* [arg0 = args] */
+	              "blx %2\n\t"     /* [fn()] */
 	              "ldr r0, %0\n\t" /* [sp = staved_sp */
 	              "mov sp, r0"     /* ] */
 	              :
@@ -75,7 +168,7 @@ static void call_with_stack(void *stack, cr_fn_t fn, void *args) {
 #endif
 }
 
-cid_t coroutine_add(cr_fn_t fn, void *args) {
+cid_t coroutine_add_with_stack_size(size_t stack_size, cr_fn_t fn, void *args) {
 	static cid_t last_created = 0;
 
 	assert(coroutine_running == 0 || coroutine_table[coroutine_running-1].state == CR_RUNNING);
@@ -94,7 +187,7 @@ cid_t coroutine_add(cr_fn_t fn, void *args) {
 
 	last_created = child;
 
-	coroutine_table[child-1].stack_size = COROUTINE_STACK_SIZE;
+	coroutine_table[child-1].stack_size = stack_size;
 	coroutine_table[child-1].stack = calloc(1, coroutine_table[child-1].stack_size);
 
 	cid_t parent = coroutine_running;
@@ -127,7 +220,7 @@ void coroutine_main(void) {
 				assert(false); /* should cr_exit() instead of returning */
 			}
 			if (cr->state == CR_NONE) {
-#if DEBUG
+#if COROUTINE_MEASURE_STACK
 				size_t stack_used = cr->stack_size;
 				while (stack_used > 0 && ((uint8_t*)cr->stack)[STACK_GROWS_DOWNWARD ? cr->stack_size - stack_used : stack_used - 1] == 0)
 					stack_used--;
@@ -152,7 +245,7 @@ bool cr_begin(void) {
 }
 
 static inline __attribute__ ((no_split_stack)) void _cr_transition(enum coroutine_state state) {
-	assert(coroutine_table[coroutine_running-1].state == CR_RUNNING);
+	assert(coroutine_running && coroutine_table[coroutine_running-1].state == CR_RUNNING);
 	coroutine_table[coroutine_running-1].state = state;
 	if (!setjmp(coroutine_table[coroutine_running-1].env))  /* point=c2 */
 		longjmp(coroutine_main_env, 1); /* jump to point=b */
@@ -162,7 +255,7 @@ void cr_yield(void)           { _cr_transition(CR_RUNNABLE); }
 void cr_pause_and_yield(void) { _cr_transition(CR_PAUSED); }
 
 void cr_exit(void) {
-	assert(coroutine_table[coroutine_running-1].state == CR_RUNNING);
+	assert(coroutine_running && coroutine_table[coroutine_running-1].state == CR_RUNNING);
 	coroutine_table[coroutine_running-1].state = CR_NONE;
 	longjmp(coroutine_main_env, 1); /* jump to point=b */
 }
diff --git a/coroutine.h b/coroutine.h
index 13f9797..afe1b1a 100644
--- a/coroutine.h
+++ b/coroutine.h
@@ -1,68 +1,129 @@
-/* coroutine.h - Simple coroutine and request/response implementation
+/* coroutine.h - Simple embeddable coroutine implementation
  *
  * Copyright (C) 2024  Luke T. Shumaker <lukeshu@lukeshu.com>
  * SPDX-Licence-Identifier: AGPL-3.0-or-later
  */
 
+/**
+ * A coroutine is a cooperatively-multitasked form of threading.
+ *
+ * This coroutine.{h,c} form a lightweight coroutine implementation
+ * for non-multithreaded environments (only one coroutine may be
+ * running at a time (so no m:n scheduling), coroutine_{add,main} may
+ * not be called concurrently with eachother).
+ *
+ * Unlike many other lightweight or embeddable coroutine
+ * implementations, coroutine.{h,c} allow coroutines to use the stack
+ * as normal C functions and do not forbid switch() blocks in
+ * coroutines.
+ *
+ * See also: coroutine_chan.h is a request/response system built on
+ * top of coroutine.{h,c}.
+ */
 #ifndef _COROUTINE_H_
 #define _COROUTINE_H_
 
 #include <stddef.h>  /* for size_t */
 #include <stdbool.h> /* for bool */
 
+/* configuration **************************************************************/
+
+#define COROUTINE_DEFAULT_STACK_SIZE (8*1024)
+
 /* typedefs *******************************************************************/
 
-typedef size_t cid_t; /* 0=none; otherwise 1-indexed */
+/**
+ * A cid_t is a "coroutine ID", used to refer to a coroutine.
+ *
+ * May be reused if a coroutine exits.
+ *
+ * Valid IDs are `1 <= cid <= COROUTINE_NUM`; `0` is used for
+ * "none/invalid".
+ */
+typedef size_t cid_t;
 
-#define COROUTINE __attribute__ ((noreturn, no_split_stack)) void
+/**
+ * The root function for a coroutine; a coroutine function should be
+ * declared as
+ *
+ *    COROUTINE myfunc(void *_args) {
+ *        myargtype *args = args;
+ *        cr_begin();
+ *
+ *        ...
+ *
+ *        cr_end();
+ *    }
+ *
+ * The function MUST NOT ever return (GCC enforces this); call
+ * cr_exit() or cr_end() instead of returning.
+ *
+ * When creating a coroutine with coroutine_add(), the bit before
+ * cr_begin() runs before coroutine_add() returns; if `_args` points
+ * to a place on another coroutine's stack that may go away, then this
+ * is an opportunity to copy it to this coroutine's stack.  Otherwise
+ * you shouldn't do anything else before calling cr_begin().
+ * Specifically, coroutine_add() and
+ * cr_{yield,pause_and_yield,exit,end}() are explicitly forbidden to
+ * call from within a coroutine before cr_begin() (note that the
+ * cr_chan_*() macros call these functions).
+ */
 typedef void (*cr_fn_t)(void *args);
+#define COROUTINE __attribute__ ((noreturn, no_split_stack)) void
 
 /* managing coroutines ********************************************************/
 
-void coroutine_init(void);
-cid_t coroutine_add(cr_fn_t fn, void *args);
+/**
+ * Call `fn(args)` in a new coroutine with stack size `stack_size`.
+ *
+ * See the doc comment on c_fn_t for the requirements imposed on fn.
+ *
+ * May be called from outside any coroutine (before calling
+ * coroutine_main()) or from inside of a coroutine (after it cas
+ * called cr_begin()).
+ *
+ * Returns the cid of the newly-created coroutine.  May return 0 if
+ * there are already COROUTINE_NUM active coroutines.
+ */
+__attribute__ ((no_split_stack)) cid_t coroutine_add_with_stack_size(size_t stack_size, cr_fn_t fn, void *args);
+
+/**
+ * Like coroutine_add_with_stack_size(), but uses a default stack size so
+ * you don't need to think about it.
+ */
+#define coroutine_add(fn, args) coroutine_add_with_stack_size(COROUTINE_DEFAULT_STACK_SIZE, fn, args)
+
+/**
+ * The main scheduler loop.
+ *
+ * "Should" never return, but will print a message to stderr and
+ * return if the program has deadlocked and there are no runnable
+ * coroutines.  So be sure to call coroutine_add() at least once
+ * before calling this.
+ */
 void coroutine_main(void);
 
 /* inside of coroutines *******************************************************/
 
+/** cr_begin() goes at the beginning of a coroutine, after it has initialized its stack.  */
 __attribute__ ((no_split_stack))           bool cr_begin( void);
+/** cr_exit() terminates the currently-running coroutine.  */
 __attribute__ ((no_split_stack, noreturn)) void cr_exit(void);
+/** cr_yield() switches to another coroutine (if there is another runnable coroutine to switch to).  */
 __attribute__ ((no_split_stack))           void cr_yield(void);
+/** cr_pause_and_yield() marks the current coroutine as not-runnable and switches to another coroutine.  */
 __attribute__ ((no_split_stack))           void cr_pause_and_yield(void);
+/** cr_unpause() marks a coroutine as runnable that has previously marked itself as non-runnable with cr_pause_and_yield().  */
 __attribute__ ((no_split_stack))           void cr_unpause(cid_t);
+/** cr_end() is a counterpart to cr_begin(), but is really just cr_exit().  */
 #define cr_end cr_exit
 
+/** cr_getcid() returns the cid of the currently-running coroutine.  */
 cid_t cr_getcid(void);
 
-/* request/response channels **************************************************/
-
-#define cr_chan_t(req_t, resp_t) struct { \
-		cid_t requester;          \
-		cid_t responder;          \
-		req_t  req;               \
-		resp_t resp;              \
-	}
-#define cr_chan_req(ch, _resp_p, _req) do {                             \
-		(ch)->requester = cr_getcid();                          \
-		(ch)->req = (_req);                                     \
-		if ((ch)->responder != 0)                               \
-			cr_unpause((ch)->responder);                    \
-		cr_pause_and_yield();                                   \
-		if ((typeof(&(ch)->resp))(_resp_p))                     \
-			*((typeof(&(ch)->resp))(_resp_p)) = (ch)->resp; \
-	} while (0)
-#define cr_chan_have_req(ch) ((ch)->requester != 0)
-#define cr_chan_recv_req(ch, _req_p) do {      \
-		(ch)->responder = cr_getcid(); \
-		if ((ch)->requester == 0)      \
-			cr_pause_and_yield();  \
-		*(_req_p) = (ch)->req;         \
-	} while (0)
-#define cr_chan_send_resp(ch, _resp) do {    \
-		cr_unpause((ch)->requester); \
-		(ch)->responder = 0;         \
-		(ch)->requester = 0;         \
-		(ch)->resp = (_resp);        \
-	} while (0)
+/**
+ * It is not possible for one coroutine to kill another or to mark
+ * another as paused; a coroutine may only do those things to itself.
+ */
 
 #endif /* _COROUTINE_H_ */
diff --git a/coroutine_chan.h b/coroutine_chan.h
new file mode 100644
index 0000000..3dfccc5
--- /dev/null
+++ b/coroutine_chan.h
@@ -0,0 +1,85 @@
+/* coroutine_chan.h - Simple request/response system for coroutine.{h,c}
+ *
+ * Copyright (C) 2024  Luke T. Shumaker <lukeshu@lukeshu.com>
+ * SPDX-Licence-Identifier: AGPL-3.0-or-later
+ */
+
+/**
+ * The cr_chan_* macros form a simple request/response channel
+ * mechanism built on top of the cr_pause_and_yeild() and cr_unpause()
+ * primitives.
+ */
+#ifndef _COROUTINE_CHAN_H_
+#define _COROUTINE_CHAN_H_
+
+#include "coroutine.h"
+
+/**
+ * cr_chan_t(req_t, resp_t) returns the type definition for a channel
+ * on which the requester submits a value of type `req_t` and the
+ * responder responds with a value of type `resp_t`.
+ */
+#define cr_chan_t(req_t, resp_t) struct { \
+		cid_t requester;          \
+		cid_t responder;          \
+		req_t  req;               \
+		resp_t resp;              \
+	}
+
+/* These are "functions" are preprocessor macros instead of real C
+ * functions so that the compiler can do type-checking instead of
+ * having these functions take `void*`.  */ 
+
+/**
+ * ch_chan_req(cr_chan_t(req_t, resp_t) *ch, resp_t *resp, req_t req)
+ * submits the `req` request to `ch` puts the response in `*resp_p`.
+ *
+ * Blocks until the responder has called both cr_chan_recv_req() and
+ * cr_chan_send_resp().
+ */
+#define cr_chan_req(ch, _resp_p, _req) do {                             \
+		(ch)->requester = cr_getcid();                          \
+		(ch)->req = (_req);                                     \
+		if ((ch)->responder != 0)                               \
+			cr_unpause((ch)->responder);                    \
+		cr_pause_and_yield();                                   \
+		if ((typeof(&(ch)->resp))(_resp_p))                     \
+			*((typeof(&(ch)->resp))(_resp_p)) = (ch)->resp; \
+	} while (0)
+
+/**
+ * cr_chan_have_req(cr_chan_t(req_t, resp_t) *ch) allows a responder
+ * to check whether or not there is a request waiting to be received
+ * (with cr_chan_recv_req()) without blocking if there is not.
+ *
+ * Never blocks.
+ */
+#define cr_chan_have_req(ch) ((ch)->requester != 0)
+
+/**
+ * cr_chan_recv_req(cr_chan_t(req_t, resp_t) *ch, req_t *req_p) reads
+ * a request from ch into `*req_p`.
+ *
+ * If there is not a pending request on `ch`, blocks until there is.
+ */
+#define cr_chan_recv_req(ch, _req_p) do {      \
+		(ch)->responder = cr_getcid(); \
+		if ((ch)->requester == 0)      \
+			cr_pause_and_yield();  \
+		*(_req_p) = (ch)->req;         \
+	} while (0)
+
+/**
+ * cr_chan_send_resp(cr_chan_t(req_t, resp_t) *ch, resp_t resp) sends
+ * the reply to the most-recently-read request.
+ *
+ * Never blocks.
+ */
+#define cr_chan_send_resp(ch, _resp) do {    \
+		cr_unpause((ch)->requester); \
+		(ch)->responder = 0;         \
+		(ch)->requester = 0;         \
+		(ch)->resp = (_resp);        \
+	} while (0)
+
+#endif /* _COROUTINE_CHAN_H_ */
diff --git a/main.c b/main.c
index 098c3fd..1b1558e 100644
--- a/main.c
+++ b/main.c
@@ -36,7 +36,6 @@ int main() {
 	usb_common_earlyinit();
 	usb_keyboard_init();
 	usb_common_lateinit();
-	coroutine_init();
 
 	/* set up coroutines */
 	coroutine_add(usb_common_cr, NULL);
diff --git a/usb_keyboard.h b/usb_keyboard.h
index 40b0bb2..656add8 100644
--- a/usb_keyboard.h
+++ b/usb_keyboard.h
@@ -7,7 +7,7 @@
 #ifndef _USB_KEYBOARD_H_
 #define _USB_KEYBOARD_H_
 
-#include "coroutine.h"
+#include "coroutine_chan.h"
 
 typedef cr_chan_t(uint32_t, int) usb_keyboard_chan_t;