[ottf, dif/uart] Writing to the UART in ISR may race with UART sending checks
Nobody has claimed this yet.
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
### Description
# Problem
In [`dif_uart_send_byte(_polled)`](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/dif/dif_uart.c#L320), the logic is:
- wait for TX fifo to not be full,
- write byte to fifo,
- wait for TX fifo to drain (polled version).
This function is used in e.g. [`ottf_putbuf` for ujson](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/testing/test_framework/ujson_ottf.c#L13) (by the way not sure why we use the polled version for ujson?). But at the same time, all of this runs with interrupts enabled and one possible interrupt is [`ottf_console_flow_control_isr`](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/testing/test_framework/ottf_console.c#L198) that [may call](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/testing/test_framework/ottf_console.c#L193) [`dif_uart_byte_send`](https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/dif/dif_uart.c#L288) that does NOT drain the TX fifo. Therefore, it seems possible that in some cases, an interrupt happens right after the TX fifo check in either `dif_uart_send_byte_polled` or `dif_uart_send_byte`, and the flow control decides to send a character. On return from the ISR, the TX fifo may still be full and the code will try to write to full fifo. If that happens, then we might drop a UART character by mistake.
The first step would be to confirm that this really can happen and that I haven't missed something.
# Solutions
One possible solution is to disable interrupts when writing to the UART so that the TX fifo check + TX fifo write is atomic. However, as pointed out by @nbdd0121 , this doesn't help if an NMI writes to the UART.
Another solution would be that when writing a UART character to the FIFO in an interrupt context, to always wait for at least one character to drain from the FIFO. This guarantees that any TX fifo check done before the ISR still applies. The downside is that the ISR will have to wait at least one character length hence introduces a delay in an ISR which may not be ideal.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading sw/device/lib/dif/dif_uart.c at dif_uart_send_byte(_polled) and the callers in sw/device/lib/testing/test_framework/ujson_ottf.c. Trace the interaction with ottf_console_flow_control_isr in ottf_console.c and dif_uart_byte_send, then confirm whether the cited interrupt interleaving can occur. Done means documenting the confirmed behavior and implementing or clearly selecting a safe resolution.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100