pussuw commented on code in PR #14761:
URL: https://github.com/apache/nuttx/pull/14761#discussion_r1840419593
##########
drivers/syslog/syslog_channel.c:
##########
@@ -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);
Review Comment:
> because we don't have a spinlock-interlocked blocking api? (do we?)
Not one that knows to yield.
In SMP case using enter/exit_critical does exactly the same thing as a spin
lock but it locks many unrelated (not serial related) APIs in the kernel (like
semaphores etc.) as well, as they use enter/exit_critical.
> i'm not sure what you mean here. is it about this PR?
Just that in non-SMP case using spin_lock_irqsave would revert back to a
local interrupt disable, like it does when using enter/exit_critical (if
someone is worried about non-SMP perf.)
I seem to remember seeing a discussion about serial/syslog locking and where
it should be, but IMO the mutual exclusion (whatever the chosen mechanism is..)
should be on the device level as the shared resource here is the serial port
device? So replacing the critical section from both serial.c and syslog.c by a
single spinlock should not be an issue. As it would be the serial port lock.
Also, doesn't syslog support other devices than serial port as well ? Are
they protected by enter/exit_critcal (I honestly don't know).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
