[BUG] low level console i/o (up_putc) can interfere interrupt-based uart i/o #14662
Comments
github-actions
bot
added
Arch: all
|
@patacongo what do you think? I know this implementation exists since initial NuttX versions |
up_putc was only added originally to get some very early output to assist in CPU bringup and analyzing assertions. That was about 17 or more years ago. It normally just printed things like ABCDEF to let you know where the power-up reset logic failed.. For some of the early CPUs, I had only LEDs and low level serial output to perform the entire bring-up. It is really amazing what you can do with very little if you have to. up_putc was used in various other, less appropriate ways since... Such as getting interrupt level serial output. Using serial output to support CPU bring-up with serial output is not so important with today's modern, cheap debuggers. Perhaps up_putc() could even just be removed. |
it's still useful for early in the boot, where i guess it's fine. (maybe until we boot secondary CPUs.) i guess we can avoid most of problematic cases by removing CONFIG_SYSLOG_DEFAULT. how do you think? |
|
@yamt I agree! It still useful and we need to keep it. No idea why Syslog is using it by default, it could be an option, instead of default output channel |
Because the implementation is problematic. See apache#14662 for the discussion. Also, make it depend on EXPERIMENTAL to discourage it. Instead, enable SYSLOG_CONSOLE for a bit more cases. An alternative would be to introduce/extend some serialization mechanism (eg. enter_critical_section, which is currently used by the serial driver to interact with interrupts) to cover up_putc users including SYSLOG_DEFAULT. While it works, it doesn't sound attractive to me to introduce this kind of complexity to up_putc, which is supposed to be a very low-level machinary used early in the boot. Also, the implementation of such serialization can be complex because how up_putc works is basically device-specific and how it corresponds to other devices on the system (eg. uarts) isn't obvious to the upper layers.
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.
This would avoid the undesirable intertactions with the serial driver described in #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.
Because the implementation is problematic. See apache#14662 for the discussion. Also, make it depend on EXPERIMENTAL to discourage it. Instead, enable SYSLOG_CONSOLE for a bit more cases. An alternative would be to introduce/extend some serialization mechanism (eg. enter_critical_section, which is currently used by the serial driver to interact with interrupts) to cover up_putc users including SYSLOG_DEFAULT. While it works, it doesn't sound attractive to me to introduce this kind of complexity to up_putc, which is supposed to be a very low-level machinary used early in the boot. Also, the implementation of such serialization can be complex because how up_putc works is basically device-specific and how it corresponds to other devices on the system (eg. uarts) isn't obvious to the upper layers.
i guess this was not really SMP-specific.
#14761 should be enough to fix the case as well. |
|
but is uart_write protected by the critical section too? |
i'm not sure what you meant. but a literal answer is "yes". |
Description / Steps to reproduce the issue
for some configurations, it's possible for a system to use both of low-level up_putc and the uart (drivers/serial/serial.c) on the console.
for an example, printf(stdout) goes to the uart and syslog() goes to up_putc.
the uart uses a semaphore (dev->xmitsem) interlocked with the kernel critical section to deal with interrupts.
although how up_putc is implemented is device-specific, it usually disables interrupts, performs polling-based i/o, and then restores the interrupts.
unfortunately, with SMP, the latter sometimes can "consume" the hardware events which would otherwise have triggered tx interrupts, which the former might be waiting for. if it happens, the former can block forever.
i was mainly looking at the esp32 implementation. but other implementations look similar.
an obvious fix would be to wrap up_putc with the critical section. but i'm not feeling too happy with the approach.
any suggestions?
On which OS does this issue occur?
[OS: Mac]
What is the version of your OS?
macOS 14.7
NuttX Version
master
Issue Architecture
[Arch: all]
Issue Area
[Area: Drivers]
Verification
The text was updated successfully, but these errors were encountered: