xiaoxiang781216 commented on code in PR #14695:
URL: https://github.com/apache/nuttx/pull/14695#discussion_r1835422379
##########
drivers/syslog/Kconfig:
##########
@@ -267,19 +267,25 @@ config SYSLOG_STREAM
config SYSLOG_CONSOLE
bool "Log to /dev/console"
- default !ARCH_LOWPUTC && !SYSLOG_CHAR && !RAMLOG_SYSLOG &&
!SYSLOG_RPMSG && !SYSLOG_RTT
+ default !SYSLOG_CHAR && !RAMLOG_SYSLOG && !SYSLOG_RPMSG && !SYSLOG_RTT
Review Comment:
> > > it's a valid concern.
> > > IMO, using syslog from interrupt is a design mistake.
> >
> >
> > Why is it mistake? If syslog can't support the interrupt, what's other
kernel facility could do?
>
> syslog is an api for ordinary applications. it isn't free for an api to be
prepared to be used by interrupts. eg. complex locking.
if so, do you plan to remove the capability that sem can be posted and
mqueue can be sent from interrupt handler?
These extensions which make POSIX API can work from interrupt context are
compatible extension in OS context.
> yes, it's simpler to have a separate api for interrupts, IMO.
You still need fulfill the requirement I said before that the new function
need work in the interrupt context.
>
> > > anyway, it won't be a disaster as syslog_device.c already has a check
for that case. (syslog_dev_outputready)
> >
> >
> > Different syslog channel has the different capability:
uputc/ramlog/rpmsg support interrupt, console/device/file doesn't support.
Since the default syslog(uputc) support the interrupt before, the change need
keep this behavior, otherwise all board will break.
> > > for panic, maybe it's enough to make it fall back to up_putc if
g_nx_initstate == OSINIT_PANIC. as the panic logic should have already stopped
moving parts (eg. pause_all_cpu) at that point, it should be ok to use up_putc
in that case.
> >
> >
> > panic is just one case; driver may output log in interrupt too.
> > > how do you think?
> >
> >
> > I think the default syslog channel need support the output from
interrupt to handle all possible case. So, I would suggest improving syslog
console to support the interrupt before making it as default. serial driver
framework already contains the special code to handle the interrupt case, so
there is no block issue to improve it.
>
> have you read #14662?
You can fix up_putc in what you want, but syslog can be called from the
interrupt context must been keep as before until you provide a new api(e.g.
printk) which can be called from the interrupt context and replace all syslog
call in the code base.
>
> do you prefer to sprinkle more serialization in syslog and/or up_putc?
I also want to drop up_putc from syslog too, but I have to point out that
you can't simply switch the syslog channel to console by default since it
breaks ALL log from panic and interrupt.
So, let me change the patch to draft until you find a way to fix this
problem.
--
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]
