libtty: add support for TCXONC and TCSBRK#671
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the TCXONC (flow control) and TCSBRK (send break) ioctls in libtty, implementing the libtty_flow and libtty_sendbreak helper functions and updating output/ready checks to respect the new TF_OOFF and TF_SBRK flags. Feedback on these changes highlights several critical issues: first, using volatile sig_atomic_t for t_flags does not guarantee atomic read-modify-write operations, so standard C11 atomics should be used instead; second, a negative duration in libtty_sendbreak can cause an integer overflow and block usleep indefinitely, requiring input validation; third, libtty_flow lacks a default case to handle invalid actions; and finally, the return value of libtty_sendbreak should be propagated back through libtty_ioctl.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /* cached optimizations */ | ||
| char breakchars[4]; /* enough to hold \n, VEOF and VEOL. */ | ||
| unsigned int t_flags; | ||
| volatile sig_atomic_t t_flags; |
There was a problem hiding this comment.
Using volatile sig_atomic_t does not guarantee atomic read-modify-write operations (such as bitwise OR/AND). Since t_flags is accessed and modified across different execution contexts (e.g., user-space threads and potentially ISRs or driver threads) without a single shared lock, this can lead to race conditions where flag updates are lost. Please use standard C11 atomic types from <stdatomic.h> (e.g., atomic_uint or _Atomic unsigned int) to ensure thread-safe atomic operations.
_Atomic unsigned int t_flags;References
- When implementing atomic operations, prefer using standard library features (e.g., from
<stdatomic.h>) over compiler-specific built-ins to ensure code portability and adherence to standards. - To prevent hard-to-debug race conditions, always use atomic operations for read-modify-write access to variables shared between an Interrupt Service Routine (ISR) and another execution context, even if the chance of a race seems low.
There was a problem hiding this comment.
well this is what I tried to achieve here - the reads being "atomic" (going in one go). The writes are always performed under a lock, because they depend on libtty_write or other functions that should see only one state on them under the lock. Because of this, atomics here seem like an overkill.
Unit Test Results10 860 tests ±0 10 189 ✅ - 1 53m 54s ⏱️ +45s For more details on these failures, see this check. Results for commit 1d8e4d1. ± Comparison against base commit 4f6a072. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
2 times, most recently
from
0fd3083 to
725e4dc
Compare
|
/gemini review |
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
from
725e4dc to
821f53c
Compare
There was a problem hiding this comment.
Code Review
This pull request adds support for TTY flow control (TCXONC) and sending break signals (TCSBRK), including a concrete implementation for the 16550 UART driver. The review feedback identifies a critical issue where a mutex is held during a sleep call in uart_sendbreak, which could block the interrupt thread and cause RX FIFO overruns. Additionally, the reviewer points out a type mismatch in the sendbreak callback signature, recommends using standard atomic operations from <stdatomic.h> instead of volatile sig_atomic_t for thread-safe flag modifications, and notes an unused flag definition.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
2 times, most recently
from
06aadb3 to
919a9b1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements software flow control (IXON/IXOFF/IXANY), break signaling (TCSBRK), and input parity/break handling in the TTY library and the 16550 UART driver. While these additions significantly expand terminal capabilities, several critical issues were identified in the implementation. Specifically, the UART interrupt handler contains a potential buffer underflow and incorrectly associates parity/break errors with previously received characters. Additionally, the IXOFF flow control implementation introduces a deadlock risk and premature resume behavior, the libtty_flow function incorrectly returns positive values on success instead of zero, and the break transmitter busy-waits without yielding, which can cause high CPU utilization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
from
919a9b1 to
c7616a0
Compare
|
/gemini review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces software flow control (IXON, IXOFF, IXANY) and additional terminal flags (PARMRK, INPCK, IGNPAR, NOFLSH) to the TTY library and UART 16550 driver, utilizing atomic operations for thread-safe flag updates. However, several critical issues need to be addressed. In libtty, removing mutex locks in libtty_close and calling condBroadcast outside the mutex in TCOON introduce race conditions, and libtty_write is incorrectly called with a size of 0 instead of 1 when sending CSTART. In the UART driver, sleep(100) is used instead of usleep, an off-by-one error in the buffer check can cause an overflow, normal characters are completely ignored in the interrupt handler, and parity error handling is incorrectly coupled with hardware configuration instead of being processed in software based on INPCK.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
from
c7616a0 to
e361915
Compare
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
from
e361915 to
24c7dcc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds software flow control (IXON, IXOFF, IXANY), parity and break handling, and new ioctls (TCXONC, TCSBRK) to the TTY subsystem and UART 16550 driver, alongside migrating t_flags to atomic operations. The review feedback identifies critical issues, including a potential deadlock in libtty_putchar_helper from recursive locking, a race condition on REG_LCR in break_enable, and unconditional register reads in the UART interrupt handler that can discard valid characters. Additionally, the feedback points out a potential hang in libtty_flow due to blocking writes, a race condition on tx_fifo from unsynchronized access, and a duplicate header inclusion in libtty.h.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const char cstop = CSTOP; | ||
| tx_write_ifspace(tty, &cstop, 1); | ||
| } |
There was a problem hiding this comment.
Calling tx_write_ifspace here introduces a race condition on tty->tx_fifo. tx_write_ifspace modifies the transmit FIFO without acquiring tty->tx_mutex, which can run concurrently with a user-space thread calling libtty_write (which holds tty->tx_mutex). To ensure thread safety, use libtty_write with O_NONBLOCK instead, which properly synchronizes access to the transmit FIFO.
const char cstop = CSTOP;
libtty_write(tty, &cstop, 1, O_NONBLOCK);There was a problem hiding this comment.
true, but we already use it in libtty_disc.c all over the place without synchronization, so its a problem to solve in another PR.
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
2 times, most recently
from
c568f25 to
f8818e0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces software flow control (IXON, IXOFF, IXANY), parity/break handling, and atomic operations for t_flags in the TTY layer and UART 16550 driver, alongside new ioctls like TCXONC and TCSBRK. Key feedback highlights several critical issues: a buffer overflow in uart_interrupt under PARMRK due to an insufficient boundary check; swapped logic between TCSETS and TCSETSW ioctls; a lost wake-up race condition in libtty_close from removed mutex locks; unconditional clearing of TF_IOFF in libtty_read_helper even if CSTART fails to send; a potential deadlock/latency issue from holding uart->mutex while sleeping in break_enable; a missing callback signal when IXANY clears TF_OOFF; and an extremely low IXOFF threshold of 4 bytes that lacks hysteresis.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
YT: RTOS-1355
YT: RTOS-1355
YT: RTOS-1355
YT: RTOS-1355
TCSETSW and TCSETSF should both drain the output buffer. Additionally, TCSETSF should flush the input buffer. YT: RTOS-1355
YT: RTOS-1355
julianuziemblo
force-pushed
the
julianuziemblo/RTOS-1355
branch
from
f8818e0 to
1d8e4d1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements software flow control (IXON, IXOFF, IXANY) and break condition handling (TCSBRK, TCXONC) in the TTY line discipline and UART 16550 driver, and transitions t_flags to atomic operations. Feedback highlights several critical issues: removing mutex locks in libtty_close introduces a lost wake-up race condition; reading REG_LSR in break_enable and calling fifo_freespace in libtty_read_helper without holding their respective mutexes cause data races; TCSETSF should drain the output queue before flushing the input queue to avoid missing incoming characters; and the break condition check in the UART interrupt handler must precede the parity error check to prevent incorrect handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| /* IXOFF: send CSTART if we stopped the remote and the queue has space again */ | ||
| if (CMP_FLAG(i, IXOFF)) { | ||
| if (fifo_freespace(tty->rx_fifo) >= LIBTTYDISC_INPUT_ON_THRESHOLD && | ||
| (atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_IOFF) != 0) { | ||
| const char cstart = CSTART; | ||
| /* non-blocking write so that we don't deadlock */ | ||
| libtty_write(tty, &cstart, 1, O_NONBLOCK); | ||
| atomic_fetch_and_explicit(&tty->t_flags, ~TF_IOFF, memory_order_relaxed); | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling fifo_freespace(tty->rx_fifo) without holding tty->rx_mutex is a data race because the RX FIFO can be concurrently modified by the interrupt worker thread or other contexts. To avoid this race and prevent potential lock inversion (since libtty_write locks tx_mutex), we should lock and unlock rx_mutex strictly around the fifo_freespace check.
/* IXOFF: send CSTART if we stopped the remote and the queue has space again */
if (CMP_FLAG(i, IXOFF)) {
mutexLock(tty->rx_mutex);
int space = fifo_freespace(tty->rx_fifo);
mutexUnlock(tty->rx_mutex);
if (space >= LIBTTYDISC_INPUT_ON_THRESHOLD &&
(atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_IOFF) != 0) {
const char cstart = CSTART;
/* non-blocking write so that we don't deadlock */
libtty_write(tty, &cstart, 1, O_NONBLOCK);
atomic_fetch_and_explicit(&tty->t_flags, ~TF_IOFF, memory_order_relaxed);
}
}| case TCSETSF: | ||
| libtty_flush(tty, TCIFLUSH); | ||
| /* fallthrough */ | ||
| case TCSETSW: | ||
| case TCSETSF: { | ||
| libtty_drain(tty); | ||
| /* fallthrough */ | ||
| case TCSETS: { |
There was a problem hiding this comment.
In TCSETSF, the input queue is flushed before draining the output queue. Since draining the output queue (libtty_drain) blocks and can take time, new input characters could arrive during this period and would not be flushed. To adhere to the standard behavior where all input received up to the point of the termios change is discarded, the output queue should be drained first, followed by flushing the input queue.
| case TCSETSF: | |
| libtty_flush(tty, TCIFLUSH); | |
| /* fallthrough */ | |
| case TCSETSW: | |
| case TCSETSF: { | |
| libtty_drain(tty); | |
| /* fallthrough */ | |
| case TCSETS: { | |
| case TCSETSF: | |
| libtty_drain(tty); | |
| libtty_flush(tty, TCIFLUSH); | |
| goto set_termios; | |
| case TCSETSW: | |
| libtty_drain(tty); | |
| /* fallthrough */ | |
| case TCSETS: | |
| set_termios: { |
| if ((lsr & LSR_PE) != 0 && ((iflags & INPCK) != 0)) { /* parity error */ | ||
| if ((iflags & IGNPAR) != 0) { | ||
| /* ignore characters with parity errors */ | ||
| } | ||
| else { | ||
| if ((iflags & PARMRK) != 0) { | ||
| if (i + 2 < SW_BUF_SIZE) { | ||
| uart->buf[i++] = '\377'; | ||
| uart->buf[i++] = '\0'; | ||
| uart->buf[i++] = c; | ||
| } | ||
| /* TODO: else maybe signal SW overrun */ | ||
| } | ||
| else { | ||
| if (i < SW_BUF_SIZE) { | ||
| uart->buf[i++] = '\0'; | ||
| } | ||
| /* TODO: else maybe signal SW overrun */ | ||
| } | ||
| } | ||
| } | ||
| else if ((lsr & LSR_BI) != 0) { /* break condition */ | ||
| if ((iflags & IGNBRK) != 0) { | ||
| /* ignore break condition */ | ||
| } | ||
| else { | ||
| if (i < SW_BUF_SIZE) { | ||
| uart->buf[i++] = '\0'; | ||
| } | ||
| /* TODO: else maybe signal SW overrun */ | ||
| } | ||
| } |
There was a problem hiding this comment.
A break condition (LSR_BI) typically also triggers a parity error (LSR_PE) because the line is held low. If LSR_PE is checked first and INPCK is enabled, the break condition will be incorrectly handled as a parity error. Reordering the checks to handle the break condition first ensures correct behavior.
if ((lsr & LSR_BI) != 0) { /* break condition */
if ((iflags & IGNBRK) != 0) {
/* ignore break condition */
}
else {
if (i < SW_BUF_SIZE) {
uart->buf[i++] = '\\0';
}
/* TODO: else maybe signal SW overrun */
}
}
else if ((lsr & LSR_PE) != 0 && ((iflags & INPCK) != 0)) { /* parity error */
if ((iflags & IGNPAR) != 0) {
/* ignore characters with parity errors */
}
else {
if ((iflags & PARMRK) != 0) {
if (i + 2 < SW_BUF_SIZE) {
uart->buf[i++] = '\\377';
uart->buf[i++] = '\\0';
uart->buf[i++] = c;
}
/* TODO: else maybe signal SW overrun */
}
else {
if (i < SW_BUF_SIZE) {
uart->buf[i++] = '\\0';
}
/* TODO: else maybe signal SW overrun */
}
}
}1 participant
Description
Motivation and Context
Types of changes
How Has This Been Tested?
ia32-generic-qemuChecklist:
Special treatment