Skip to content

Commit

Permalink
SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section
Browse files Browse the repository at this point in the history
This would avoid the undesirable intertactions with the serial driver
described in apache#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.
  • Loading branch information
yamt committed Nov 11, 2024
1 parent 56c920c commit 9099fe4
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 10 deletions.
4 changes: 4 additions & 0 deletions drivers/serial/serial.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
23 changes: 13 additions & 10 deletions drivers/syslog/syslog_channel.c
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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 =
{
Expand Down Expand Up @@ -234,23 +229,31 @@ 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;
}

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);
Expand Down

0 comments on commit 9099fe4

Please sign in to comment.