From 3f59b45fdc3995800c312b675fbd5873130d0c65 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Wed, 13 Nov 2024 18:33:58 +0900 Subject: [PATCH 1/2] Reapply "SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section" This reverts commit 35240d77faa0b5e00d0bb3e9024ac2dde7c8362a. --- drivers/serial/serial.c | 4 ++++ drivers/syslog/syslog_channel.c | 23 +++++++++++++---------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c index 6915e8d1f3516..d475f44d12075 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -268,6 +268,10 @@ static int uart_putxmitchar(FAR uart_dev_t *dev, int ch, bool oktoblock) { /* The following steps must be atomic with respect to serial * interrupt handling. + * + * This critical section is also used for the serialization + * with the up_putc-based syslog channels. + * See https://github.com/apache/nuttx/issues/14662 */ flags = enter_critical_section(); diff --git a/drivers/syslog/syslog_channel.c b/drivers/syslog/syslog_channel.c index a7893359d7732..8038271d5944d 100644 --- a/drivers/syslog/syslog_channel.c +++ b/drivers/syslog/syslog_channel.c @@ -33,7 +33,6 @@ #include #include -#include #ifdef CONFIG_RAMLOG_SYSLOG # include @@ -72,10 +71,6 @@ static ssize_t syslog_default_write(FAR syslog_channel_t *channel, * Private Data ****************************************************************************/ -#if defined(CONFIG_SYSLOG_DEFAULT) && defined(CONFIG_ARCH_LOWPUTC) -static mutex_t g_lowputs_lock = NXMUTEX_INITIALIZER; -#endif - #ifdef CONFIG_RAMLOG_SYSLOG static const struct syslog_channel_ops_s g_ramlog_channel_ops = { @@ -234,11 +229,17 @@ g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] = #ifdef CONFIG_SYSLOG_DEFAULT static int syslog_default_putc(FAR syslog_channel_t *channel, int ch) { - UNUSED(channel); - # ifdef CONFIG_ARCH_LOWPUTC + /* See https://github.com/apache/nuttx/issues/14662 + * about what this critical section is for. + */ + + irqstate_t flags = enter_critical_section(); up_putc(ch); + leave_critical_section(flags); # endif + + UNUSED(channel); return ch; } @@ -246,11 +247,13 @@ static ssize_t syslog_default_write(FAR syslog_channel_t *channel, FAR const char *buffer, size_t buflen) { # ifdef CONFIG_ARCH_LOWPUTC - nxmutex_lock(&g_lowputs_lock); + /* See https://github.com/apache/nuttx/issues/14662 + * about what this critical section is for. + */ + irqstate_t flags = enter_critical_section(); up_nputs(buffer, buflen); - - nxmutex_unlock(&g_lowputs_lock); + leave_critical_section(flags); # endif UNUSED(channel); From c055cc89c8855f0f38247938677b7cc11cc155b8 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Thu, 14 Nov 2024 16:45:41 +0900 Subject: [PATCH 2/2] SYSLOG_DEFAULT: avoid using critical section in certain cases Note that SYSLOG_DEFAULT can be used in extreme situations, including very early in the boot. --- drivers/syslog/syslog_channel.c | 63 ++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 12 deletions(-) diff --git a/drivers/syslog/syslog_channel.c b/drivers/syslog/syslog_channel.c index 8038271d5944d..a9bc9e87ef501 100644 --- a/drivers/syslog/syslog_channel.c +++ b/drivers/syslog/syslog_channel.c @@ -33,6 +33,7 @@ #include #include +#include #ifdef CONFIG_RAMLOG_SYSLOG # include @@ -217,6 +218,30 @@ g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] = * Private Functions ****************************************************************************/ +#if defined(CONFIG_SYSLOG_DEFAULT) && defined(CONFIG_ARCH_LOWPUTC) + +/**************************************************************************** + * Name: csection_available + * + * Description: + * Return true if the critical section is available. + * + ****************************************************************************/ + +static bool csection_available(void) +{ + /* Degrade the critical section in a few cases: + * + * a) early in the boot, where tasks are not available + * + * b) after a panic, where taking a lock can make the situation worse + */ + + return OSINIT_TASK_READY() && g_nx_initstate != OSINIT_PANIC; +} + +#endif /* defined(CONFIG_SYSLOG_DEFAULT) && defined(CONFIG_ARCH_LOWPUTC) */ + /**************************************************************************** * Name: syslog_default_putc * @@ -230,13 +255,20 @@ g_syslog_channel[CONFIG_SYSLOG_MAX_CHANNELS] = static int syslog_default_putc(FAR syslog_channel_t *channel, int ch) { # ifdef CONFIG_ARCH_LOWPUTC - /* See https://github.com/apache/nuttx/issues/14662 - * about what this critical section is for. - */ + if (csection_available()) + { + /* See https://github.com/apache/nuttx/issues/14662 + * about what this critical section is for. + */ - irqstate_t flags = enter_critical_section(); - up_putc(ch); - leave_critical_section(flags); + irqstate_t flags = enter_critical_section(); + up_putc(ch); + leave_critical_section(flags); + } + else + { + up_putc(ch); + } # endif UNUSED(channel); @@ -247,13 +279,20 @@ static ssize_t syslog_default_write(FAR syslog_channel_t *channel, FAR const char *buffer, size_t buflen) { # ifdef CONFIG_ARCH_LOWPUTC - /* See https://github.com/apache/nuttx/issues/14662 - * about what this critical section is for. - */ + if (csection_available()) + { + /* See https://github.com/apache/nuttx/issues/14662 + * about what this critical section is for. + */ - irqstate_t flags = enter_critical_section(); - up_nputs(buffer, buflen); - leave_critical_section(flags); + irqstate_t flags = enter_critical_section(); + up_nputs(buffer, buflen); + leave_critical_section(flags); + } + else + { + up_nputs(buffer, buflen); + } # endif UNUSED(channel);