On Sat, Oct 24, 2026 at 11:10 PM Maciej W. Rozycki <[email protected]> wrote: > > On Sat, 24 Oct 2026, James Hilliard wrote: > > > uart_write_wakeup() expects the UART port lock to be held. Several worker, > > DMA completion and interrupt paths call it without that lock, including > > indirect calls from uart_handle_cts_change() and the transmit helpers. > > Fair enough, and thank you for the cleanup and spotting the issue in the > first place, however... > > > diff --git a/drivers/tty/serial/sb1250-duart.c > > b/drivers/tty/serial/sb1250-duart.c > > index b4e1b90e5960..b15f76acd06b 100644 > > --- a/drivers/tty/serial/sb1250-duart.c > > +++ b/drivers/tty/serial/sb1250-duart.c > > @@ -454,10 +454,12 @@ static irqreturn_t sbd_interrupt(int irq, void > > *dev_id) > > > > if (intstat & M_DUART_ISR_RX) > > sbd_receive_chars(sport); > > - if (intstat & M_DUART_ISR_IN) > > - sbd_status_handle(sport); > > - if (intstat & M_DUART_ISR_TX) > > - sbd_transmit_chars(sport); > > + scoped_guard(uart_port_lock_irqsave, uport) { > > + if (intstat & M_DUART_ISR_IN) > > + sbd_status_handle(sport); > > + if (intstat & M_DUART_ISR_TX) > > + sbd_transmit_chars(sport); > > + } > > ... why do you think the whole functions need to be called with the lock > held while they may or may not call the handlers in question even?
For sbd_transmit_chars(), locking only uart_write_wakeup() would leave the empty-FIFO decision and TX interrupt masking racing with start_tx(). A writer could queue data and enable TX after the empty check, then the interrupt handler disables TX interrupts and leaves that output stalled. The same lock protects x_char handling and FIFO consumption against concurrent updates and flushing. These races predate this series, but wakeup-only locking would leave them unresolved. I'd keep the complete TX operation locked; each invocation sends at most one character without a FIFO-drain wait. > I > think we should avoid excessive locking where possible to avoid holding > execution elsewhere unnecessarily, shouldn't we? For sbd_status_handle(), I'd read and filter the change status first, take the lock around the CTS/DSR updates, and release it before waking modem-status waiters. I'd also move the TX lock into sbd_transmit_chars(), so RX-only interrupts don't acquire it. That means two acquisitions when both status and TX need servicing, in exchange for narrower critical sections. > > Maciej >
