From 96a610b736abccd610e47fd543cb6e66765ee1b9 Mon Sep 17 00:00:00 2001
From: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Date: Thu, 22 Nov 2018 11:34:41 +0100
Subject: [PATCH] shell: Fix log messages queueing for multiple instances

Shell log backend was using k_fifo to enqueue log messages.
It was using field in log message that was used for same
purpose in log_core before passing message to backends.
However, this method supported only single shell as
other shell was corruption the fifo because field was
reused.

Modified shell log backend to use k_msgq for pending
messages.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
---
 include/shell/shell_log_backend.h |  7 ++--
 subsys/shell/shell_log_backend.c  | 53 +++++++++++++++++++++----------
 2 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/include/shell/shell_log_backend.h b/include/shell/shell_log_backend.h
index 190230196c0..5b35090630c 100644
--- a/include/shell/shell_log_backend.h
+++ b/include/shell/shell_log_backend.h
@@ -34,7 +34,7 @@ struct shell_log_backend_control_block {
 /** @brief Shell log backend instance structure (RO data). */
 struct shell_log_backend {
 	const struct log_backend *backend;
-	struct k_fifo *fifo;
+	struct k_msgq *msgq;
 	const struct log_output *log_output;
 	struct shell_log_backend_control_block *control_block;
 };
@@ -57,13 +57,14 @@ int shell_log_backend_output_func(u8_t *data, size_t length, void *ctx);
 #if CONFIG_LOG
 #define SHELL_LOG_BACKEND_DEFINE(_name, _buf, _size)			     \
 	LOG_BACKEND_DEFINE(_name##_backend, log_backend_shell_api, false);   \
-	K_FIFO_DEFINE(_name##_fifo);					     \
+	K_MSGQ_DEFINE(_name##_msgq, sizeof(void *),			     \
+			CONFIG_SHELL_MAX_LOG_MSG_BUFFERED, sizeof(void *));  \
 	LOG_OUTPUT_DEFINE(_name##_log_output, shell_log_backend_output_func, \
 			  _buf, _size);					     \
 	static struct shell_log_backend_control_block _name##_control_block; \
 	static const struct shell_log_backend _name##_log_backend = {	     \
 		.backend = &_name##_backend,				     \
-		.fifo = &_name##_fifo,					     \
+		.msgq = &_name##_msgq,					     \
 		.log_output = &_name##_log_output,			     \
 		.control_block = &_name##_control_block			     \
 	}
diff --git a/subsys/shell/shell_log_backend.c b/subsys/shell/shell_log_backend.c
index 15a4fbdb7b2..8423852c667 100644
--- a/subsys/shell/shell_log_backend.c
+++ b/subsys/shell/shell_log_backend.c
@@ -26,41 +26,62 @@ void shell_log_backend_enable(const struct shell_log_backend *backend,
 
 static struct log_msg *msg_from_fifo(const struct shell_log_backend *backend)
 {
-	struct log_msg *msg = k_fifo_get(backend->fifo, K_NO_WAIT);
+	struct log_msg *msg;
+	int err;
 
-	if (msg) {
-		atomic_dec(&backend->control_block->cnt);
-	}
+	err = k_msgq_get(backend->msgq, &msg, K_NO_WAIT);
 
-	return msg;
+	return (err == 0) ? msg : NULL;
 }
 
 static void fifo_flush(const struct shell_log_backend *backend)
 {
-	struct log_msg *msg = msg_from_fifo(backend);
+	struct log_msg *msg;
 
 	/* Flush log messages. */
-	while (msg) {
+	while ((msg = msg_from_fifo(backend)) != NULL) {
 		log_msg_put(msg);
-		msg = msg_from_fifo(backend);
 	}
 }
 
 static void msg_to_fifo(const struct shell *shell,
 			struct log_msg *msg)
 {
-	atomic_val_t cnt;
+	int err;
 
-	k_fifo_put(shell->log_backend->fifo, msg);
+	err = k_msgq_put(shell->log_backend->msgq, &msg, K_NO_WAIT);
 
-	cnt = atomic_inc(&shell->log_backend->control_block->cnt);
+	switch (err) {
+	case 0:
+		break;
+	case -ENOMSG:
+	{
+		struct log_msg *old_msg;
 
-	/* If there is too much queued free the oldest one. */
-	if (cnt >= CONFIG_SHELL_MAX_LOG_MSG_BUFFERED) {
-		log_msg_put(msg_from_fifo(shell->log_backend));
-		if (IS_ENABLED(CONFIG_SHELL_STATS)) {
-			shell->stats->log_lost_cnt++;
+		/* drop one message */
+		old_msg = msg_from_fifo(shell->log_backend);
+
+		if (old_msg) {
+			log_msg_put(old_msg);
+
+			if (IS_ENABLED(CONFIG_SHELL_STATS)) {
+				shell->stats->log_lost_cnt++;
+			}
 		}
+
+		err = k_msgq_put(shell->log_backend->msgq, &msg, K_NO_WAIT);
+		if (err) {
+			/* Rather unusual sitaution as we just freed one element
+			 * and there is no other context that puts into the
+			 * mesq. */
+			__ASSERT_NO_MSG(0);
+		}
+		break;
+	}
+	default:
+		/* Other errors are not expected. */
+		__ASSERT_NO_MSG(0);
+		break;
 	}
 }