SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section

This would avoid the undesirable intertactions with the serial driver
described in https://github.com/apache/nuttx/issues/14662.

Although I'm not entirely happy with this fix because it assumes
the particular implementations of up_putc/up_nputc and its association
to the serial devices, I haven't come up with better ideas for now.

An alternative is to place some serializations inside the target
specific serial (and/or whatever provides up_putc api) implementaitons.
But it isn't too attractive to put potentially complex logic into the
low-level machinaries, especially when we have a lot of similar copies
of it.

Another alternative is to deprecate up_putc. (at least for the purpose
of syslog.) But it seems at least some of users are relying on what
the current implementation provides heavily.

This commit also removes g_lowputs_lock because the critical section
would serve the purpose of the lock as well.
This commit is contained in:
YAMAMOTO Takashi
2024-11-11 18:00:23 +09:00
committed by Xiang Xiao
parent 2464b3207d
commit f2aeb5e56f
2 changed files with 17 additions and 10 deletions
+4
View File
@@ -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();
+13 -10
View File
@@ -33,7 +33,6 @@
#include <nuttx/syslog/syslog.h>
#include <nuttx/compiler.h>
#include <nuttx/mutex.h>
#ifdef CONFIG_RAMLOG_SYSLOG
# include <nuttx/syslog/ramlog.h>
@@ -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);