uart: chargen: Fix chargen cmd on Zephyr

The chargen command is hanging the console because it
attempts to stop the thread that it is running in. The shell
thread should only be stopped by a separate thread (it should not
attempt to stop itself).

Modify chargen to execute as a deferred task in the work queue.  Stop
the console(legacy), or activate the console bypass feature(Zephyr)
while chargen is running to prevent it from hijacking reads of the 'x'
character.

BUG=b:251872865
BRANCH=none
TEST=chargen 36 50000 (run to completion)
     chargen 36 50000 (interrupt by sending 'x')
     uart_stress_tester.py -t 10 -d <raw_ec_uart>
     Tested on Skyrim(Zephyr)
     Tested on Nipperkin(legacy with and without this commit in fw)
     "nereid" builds with CONFIG_UART_INTERRUPT_DRIVEN=n
LOW_COVERAGE_REASON=The test shell does not include rx_bypass support.
The test serial driver does not support CONFIG_UART_INTERRUPT_DRIVEN=y
See b/255847699

Cq-Depend: chromium:3964352
Change-Id: If536a53f646846f307a7a49dfde993329950b0c5
Signed-off-by: Mark Hasemeyer <markhas@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3943262
Reviewed-by: Keith Short <keithshort@chromium.org>
Code-Coverage: Zoss <zoss-cl-coverage@prod.google.com>

3 files changed, 139 insertions, 64 deletions

diff --git a/common/chargen.c b/common/chargen.c
index cfbf8bb2ca..715e4da5e8 100644
--- a/common/chargen.c
+++ b/common/chargen.c
@@ -11,6 +11,8 @@
 #include "usb_console.h"
 #include "util.h"
 #include "watchdog.h"
+#include "hooks.h"
+#include "task.h"
 
 #ifndef SECTION_IS_RO
 /*
@@ -20,57 +22,57 @@
 #define BUFFER_DRAIN_TIME_US \
 	(1000000UL * 10 * CONFIG_UART_TX_BUF_SIZE / CONFIG_UART_BAUD_RATE)
 
-/*
- * Generate a stream of characters on the UART (and USB) console.
- *
- * The stream is an ever incrementing pattern of characters from the following
- * set: 0..9A..Za..z.
- *
- * The two optional integer command line arguments work as follows:
- *
- * argv[1] - reset the pattern after this many characters have been printed.
- *           Setting this value to the width of the terminal window results
- *           in a very regular stream showing on the terminal, where it is
- *           easy to observe disruptions.
- * argv[2] - limit number of printed characters to this amount. If not
- *           specified - keep printing indefinitely.
- *
- * Hitting 'x' on the keyboard stops the generator.
- */
-static int command_chargen(int argc, const char **argv)
-{
-	int wrap_value = 0;
-	int wrap_counter = 0;
-	uint8_t c;
-	uint32_t seq_counter = 0;
-	uint32_t seq_number = 0;
-	timestamp_t prev_watchdog_time;
+struct deferred_chargen_ctx {
+	int wrap_value;
+	uint32_t seq_number;
+	int (*putc)(int c);
+	int (*tx_is_blocked)(void);
+};
+static struct deferred_chargen_ctx chargen_ctx;
 
-	int (*putc_)(int c) = uart_putc;
-	int (*tx_is_blocked_)(void) = uart_buffer_full;
-
-	while (uart_getc() != -1 || usb_getc() != -1)
-		; /* Drain received characters, if any. */
+static void acquire_console(void)
+{
+#if !defined(CONFIG_USB_CONSOLE) && !defined(CONFIG_USB_CONSOLE_STREAM)
+	uart_shell_rx_bypass(true);
+#endif
+#if !defined(CONFIG_ZEPHYR) && !defined(BOARD_HOST)
+	/* The legacy fw console does not have an rx bypass feature (it is
+	 * stubbed out).  Disable the console task so that it does not
+	 * steal character reads from chargen.
+	 */
+	if (task_start_called())
+		task_disable_task(TASK_ID_CONSOLE);
+#endif /* !CONFIG_ZEPHYR  && !BOARD_HOST */
+}
 
-	if (argc > 1)
-		wrap_value = atoi(argv[1]);
+static void release_console(void)
+{
+#if !defined(CONFIG_USB_CONSOLE) && !defined(CONFIG_USB_CONSOLE_STREAM)
+	uart_shell_rx_bypass(false);
+#endif
+#if !defined(CONFIG_ZEPHYR) && !defined(BOARD_HOST)
+	if (task_start_called())
+		task_enable_task(TASK_ID_CONSOLE);
+#endif /* !CONFIG_ZEPHYR  && !BOARD_HOST */
+}
 
-	if (argc > 2)
-		seq_number = atoi(argv[2]);
+static void run_chargen(void)
+{
+	int wrap_value = chargen_ctx.wrap_value;
+	uint32_t seq_number = chargen_ctx.seq_number;
+	int (*putc_)(int c) = chargen_ctx.putc;
+	int (*tx_is_blocked_)(void) = chargen_ctx.tx_is_blocked;
 
-#if defined(CONFIG_USB_CONSOLE) || defined(CONFIG_USB_CONSOLE_STREAM)
-	if (argc > 3) {
-		if (memcmp(argv[3], "usb", 3))
-			return EC_ERROR_PARAM3;
+	timestamp_t prev_watchdog_time;
+	uint8_t c = '0';
+	uint32_t seq_counter = 0;
+	int wrap_counter = 0;
 
-		putc_ = usb_putc;
-		tx_is_blocked_ = usb_console_tx_blocked;
-	}
-#endif
+	acquire_console();
 
-	uart_shell_stop();
+	while (uart_getc() != -1 || usb_getc() != -1)
+		; /* Drain received characters, if any. */
 
-	c = '0';
 	prev_watchdog_time = get_time();
 	while (uart_getc() != 'x' && usb_getc() != 'x') {
 		timestamp_t current_time;
@@ -116,9 +118,57 @@ static int command_chargen(int argc, const char **argv)
 
 	putc_('\n');
 
-	uart_shell_start();
+	release_console();
+}
+DECLARE_DEFERRED(run_chargen);
+
+/*
+ * Generate a stream of characters on the UART (and USB) console.
+ *
+ * The stream is an ever incrementing pattern of characters from the following
+ * set: 0..9A..Za..z.
+ *
+ * The two optional integer command line arguments work as follows:
+ *
+ * argv[1] - reset the pattern after this many characters have been printed.
+ *           Setting this value to the width of the terminal window results
+ *           in a very regular stream showing on the terminal, where it is
+ *           easy to observe disruptions.
+ * argv[2] - limit number of printed characters to this amount. If not
+ *           specified - keep printing indefinitely.
+ *
+ * Hitting 'x' on the keyboard stops the generator.
+ */
+static int command_chargen(int argc, const char **argv)
+{
+	int wrap_value = 0;
+	uint32_t seq_number = 0;
+
+	int (*putc_)(int c) = uart_putc;
+	int (*tx_is_blocked_)(void) = uart_buffer_full;
+
+	if (argc > 1)
+		wrap_value = atoi(argv[1]);
+
+	if (argc > 2)
+		seq_number = atoi(argv[2]);
+
+#if defined(CONFIG_USB_CONSOLE) || defined(CONFIG_USB_CONSOLE_STREAM)
+	if (argc > 3) {
+		if (memcmp(argv[3], "usb", 3))
+			return EC_ERROR_PARAM3;
+
+		putc_ = usb_putc;
+		tx_is_blocked_ = usb_console_tx_blocked;
+	}
+#endif
+
+	chargen_ctx.wrap_value = wrap_value;
+	chargen_ctx.seq_number = seq_number;
+	chargen_ctx.putc = putc_;
+	chargen_ctx.tx_is_blocked = tx_is_blocked_;
 
-	return EC_SUCCESS;
+	return hook_call_deferred(&run_chargen_data, 0);
 }
 DECLARE_SAFE_CONSOLE_COMMAND(chargen, command_chargen,
 #if defined(CONFIG_USB_CONSOLE) || defined(CONFIG_USB_CONSOLE_STREAM)
diff --git a/include/console.h b/include/console.h
index 4e40eddac8..ddfba89926 100644
--- a/include/console.h
+++ b/include/console.h
@@ -21,15 +21,17 @@ extern "C" {
 #endif
 
 /*
- * Define uart_shell_stop() and uart_shell_start() functions to start/stop the
- * running shell. To avoid having a guard on the build type, non-Zephyr builds
- * will have a stubbed function for these which is safe to call. These functions
- * will stop/start the Zephyr shell from processing, they should be used for
- * briefly taking control of the uart.
+ * Define uart_shell_stop(), uart_shell_start(), and
+ * uart_shell_rx_bypass(enable) functions to start/stop/bypass the running
+ * shell. To avoid having a guard on the build type, non-Zephyr builds will
+ * have a stubbed function for these which is safe to call. These functions
+ * will stop/start/stop the Zephyr shell from processing, they should be used
+ * for briefly taking control of the uart.
  */
 #ifdef CONFIG_ZEPHYR
 int uart_shell_stop(void);
 void uart_shell_start(void);
+void uart_shell_rx_bypass(bool enable);
 
 #ifdef TEST_BUILD
 /* Gets the pointer to the zephyr shell, since it might not always be
@@ -45,6 +47,9 @@ static inline int uart_shell_stop(void)
 static inline void uart_shell_start(void)
 {
 }
+static inline void uart_shell_rx_bypass(bool enable)
+{
+}
 #endif
 
 /*
diff --git a/zephyr/shim/src/console.c b/zephyr/shim/src/console.c
index 91f9014dac..602e539dc4 100644
--- a/zephyr/shim/src/console.c
+++ b/zephyr/shim/src/console.c
@@ -53,9 +53,10 @@ static struct k_poll_signal shell_init_signal;
  * (which requires locking the shell).
  */
 static bool shell_stopped;
+static bool rx_bypass_enabled;
+RING_BUF_DECLARE(rx_buffer, CONFIG_UART_RX_BUF_SIZE);
 
 #if defined(CONFIG_UART_INTERRUPT_DRIVEN)
-RING_BUF_DECLARE(rx_buffer, CONFIG_UART_RX_BUF_SIZE);
 
 static void uart_rx_handle(const struct device *dev)
 {
@@ -114,6 +115,19 @@ static void shell_uninit_callback(const struct shell *shell, int res)
 	k_poll_signal_raise(&shell_uninit_signal, res);
 }
 
+void bypass_cb(const struct shell *shell, uint8_t *data, size_t len)
+{
+	if (!ring_buf_put(&rx_buffer, data, len)) {
+		printk("Failed to write to uart ring buf\n");
+	}
+}
+
+void uart_shell_rx_bypass(bool enable)
+{
+	shell_set_bypass(shell_zephyr, enable ? bypass_cb : NULL);
+	rx_bypass_enabled = enable;
+}
+
 int uart_shell_stop(void)
 {
 	struct k_poll_event event = K_POLL_EVENT_INITIALIZER(
@@ -325,23 +339,29 @@ void uart_tx_flush(void)
 
 int uart_getc(void)
 {
-#if defined(CONFIG_UART_INTERRUPT_DRIVEN)
 	uint8_t c;
+	int rv = -1;
 
-	if (ring_buf_get(&rx_buffer, &c, 1)) {
-		return c;
+	/*
+	 * Don't try to read from the uart when the console
+	 * owns it.
+	 */
+	if (!shell_stopped && !rx_bypass_enabled) {
+		LOG_ERR("Shell must be stopped or rx bypass enabled");
+		return -1;
 	}
-	return -1;
-#else
-	uint8_t c;
-	int rv;
 
-	rv = uart_poll_in(uart_shell_dev, &c);
-	if (rv) {
-		return rv;
+	if (IS_ENABLED(CONFIG_UART_INTERRUPT_DRIVEN) || rx_bypass_enabled) {
+		if (ring_buf_get(&rx_buffer, &c, 1)) {
+			rv = c;
+		}
+	} else {
+		rv = uart_poll_in(uart_shell_dev, &c);
+		if (!rv) {
+			rv = c;
+		}
 	}
-	return c;
-#endif
+	return rv;
 }
 
 void uart_clear_input(void)