On Mon, 14 Apr 2025, Yunhui Cui wrote: > When the PSLVERR_RESP_EN parameter is set to 1, the device generates > an error response if an attempt is made to read an empty RBR (Receive > Buffer Register) while the FIFO is enabled. > > In serial8250_do_startup(), calling serial_port_out(port, UART_LCR, > UART_LCR_WLEN8) triggers dw8250_check_lcr(), which invokes > dw8250_force_idle() and serial8250_clear_and_reinit_fifos(). The latter > function enables the FIFO via serial_out(p, UART_FCR, p->fcr). > Execution proceeds to the dont_test_tx_en label: > ... > serial_port_in(port, UART_RX); > This satisfies the PSLVERR trigger condition. > > Because another CPU(e.g., using printk()) is accessing the UART (UART > is busy), the current CPU fails the check (value & ~UART_LCR_SPAR) == > (lcr & ~UART_LCR_SPAR), causing it to enter dw8250_force_idle(). > > To fix this, all calls to serial_out(UART_LCR) and serial_in(UART_RX) > should be executed under port->lock. Additionally, checking the readiness > via UART_LSR should also be done under port->lock. > > Panic backtrace: > [ 0.442336] Oops - unknown exception [#1] > [ 0.442343] epc : dw8250_serial_in32+0x1e/0x4a > [ 0.442351] ra : serial8250_do_startup+0x2c8/0x88e > ... > [ 0.442416] console_on_rootfs+0x26/0x70 > > Fixes: c49436b657d0 ("serial: 8250_dw: Improve unwritable LCR workaround") > Link: https://lore.kernel.org/all/84cydt5peu.fsf@jogness.linutronix.de/T/ > Signed-off-by: Yunhui Cui As Andy mentioned, this change looks it would benefit from splitting to multiple parts. However, this change brings back some memories from a few years back. Back then, there was a reporter who had issues issues related to dw8250_force_idle() or writing some of the registers (IIRC). I ended up looking into finding a better solution to the write-while-BUSY problem which entirely replaced dw8250_force_idle() that is quite hacky and seems unreliable on fundamendal level. Sadly, once I had posted a patch for testing, the reporter went dead silent so the patch was left rotting as I had no time to try to reproduce. Perhaps the patch I created back then would be useful for addressing this problem you're facing (the patch is attached). I've rebased the patch on top of the tty-next now (but I did no testing beyond compiling). There are a few further thoughts / missing bits mentioned in the comments within the patch itself (I did not try to updated them now, so the comments may have rotten too). > --- > drivers/tty/serial/8250/8250_dw.c | 8 +++++ > drivers/tty/serial/8250/8250_port.c | 46 ++++++++++++++++++----------- > 2 files changed, 36 insertions(+), 18 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c > index af24ec25d976..e97200ff30e3 100644 > --- a/drivers/tty/serial/8250/8250_dw.c > +++ b/drivers/tty/serial/8250/8250_dw.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -112,6 +113,13 @@ static void dw8250_force_idle(struct uart_port *p) > struct uart_8250_port *up = up_to_u8250p(p); > unsigned int lsr; > > + /* > + * Serial_in(p, UART_RX) should be under port->lock, but we can't add > + * it to avoid AA deadlock as we're unsure if serial_out*(...UART_LCR) > + * is under port->lock. > + */ > + lockdep_assert_held_once(&p->lock); > + > serial8250_clear_and_reinit_fifos(up); > > /* > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 3f256e96c722..21bbd18195f5 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -1328,6 +1328,7 @@ static void autoconfig_irq(struct uart_8250_port *up) > unsigned int ICP = 0; > unsigned long irqs; > int irq; > + u16 lsr; > > if (port->flags & UPF_FOURPORT) { > ICP = (port->iobase & 0xfe0) | 0x1f; > @@ -1357,9 +1358,10 @@ static void autoconfig_irq(struct uart_8250_port *up) > /* Synchronize UART_IER access against the console. */ > uart_port_lock_irq(port); > serial_out(up, UART_IER, UART_IER_ALL_INTR); > + lsr = serial_in(up, UART_LSR); > + if (lsr & UART_LSR_DR) > + serial_port_in(port, UART_RX); > uart_port_unlock_irq(port); > - serial_in(up, UART_LSR); > - serial_in(up, UART_RX); > serial_in(up, UART_IIR); > serial_in(up, UART_MSR); > serial_out(up, UART_TX, 0xFF); > @@ -2137,19 +2139,16 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits) > static int serial8250_get_poll_char(struct uart_port *port) > { > struct uart_8250_port *up = up_to_u8250p(port); > - int status; > + int status = NO_POLL_CHAR; > u16 lsr; > > serial8250_rpm_get(up); > > + uart_port_lock_irqsave(port, &flags); > lsr = serial_port_in(port, UART_LSR); > - > - if (!(lsr & UART_LSR_DR)) { > - status = NO_POLL_CHAR; > - goto out; > - } > - > - status = serial_port_in(port, UART_RX); > + if ((lsr & UART_LSR_DR)) > + status = serial_port_in(port, UART_RX); > + uart_port_unlock_irqrestore(port, flags); > out: > serial8250_rpm_put(up); > return status; > @@ -2264,13 +2263,16 @@ int serial8250_do_startup(struct uart_port *port) > * Clear the FIFO buffers and disable them. > * (they will be reenabled in set_termios()) > */ > + uart_port_lock_irqsave(port, &flags); > serial8250_clear_fifos(up); > > /* > * Clear the interrupt registers. > */ > - serial_port_in(port, UART_LSR); > - serial_port_in(port, UART_RX); > + lsr = serial_port_in(port, UART_LSR); > + if (lsr & UART_LSR_DR) > + serial_port_in(port, UART_RX); > + uart_port_unlock_irqrestore(port, flags); > serial_port_in(port, UART_IIR); > serial_port_in(port, UART_MSR); > > @@ -2380,9 +2382,10 @@ int serial8250_do_startup(struct uart_port *port) > /* > * Now, initialize the UART > */ > - serial_port_out(port, UART_LCR, UART_LCR_WLEN8); > > uart_port_lock_irqsave(port, &flags); > + serial_port_out(port, UART_LCR, UART_LCR_WLEN8); > + > if (up->port.flags & UPF_FOURPORT) { > if (!up->port.irq) > up->port.mctrl |= TIOCM_OUT1; > @@ -2428,15 +2431,16 @@ int serial8250_do_startup(struct uart_port *port) > } > > dont_test_tx_en: I don't see this in the tty-next branch? ~/linux/tty-next$ git grep dont_test_tx_en | cat - ~/linux/tty-next$ -- i. > - uart_port_unlock_irqrestore(port, flags); > > /* > * Clear the interrupt registers again for luck, and clear the > * saved flags to avoid getting false values from polling > * routines or the previous session. > */ > - serial_port_in(port, UART_LSR); > - serial_port_in(port, UART_RX); > + lsr = serial_port_in(port, UART_LSR); > + if (lsr & UART_LSR_DR) > + serial_port_in(port, UART_RX); > + uart_port_unlock_irqrestore(port, flags); > serial_port_in(port, UART_IIR); > serial_port_in(port, UART_MSR); > up->lsr_saved_flags = 0; > @@ -2492,6 +2496,7 @@ void serial8250_do_shutdown(struct uart_port *port) > { > struct uart_8250_port *up = up_to_u8250p(port); > unsigned long flags; > + u16 lsr; > > serial8250_rpm_get(up); > /* > @@ -2518,7 +2523,6 @@ void serial8250_do_shutdown(struct uart_port *port) > port->mctrl &= ~TIOCM_OUT2; > > serial8250_set_mctrl(port, port->mctrl); > - uart_port_unlock_irqrestore(port, flags); > > /* > * Disable break condition and FIFOs > @@ -2526,6 +2530,7 @@ void serial8250_do_shutdown(struct uart_port *port) > serial_port_out(port, UART_LCR, > serial_port_in(port, UART_LCR) & ~UART_LCR_SBC); > serial8250_clear_fifos(up); > + uart_port_unlock_irqrestore(port, flags); > > #ifdef CONFIG_SERIAL_8250_RSA > /* > @@ -2538,7 +2543,12 @@ void serial8250_do_shutdown(struct uart_port *port) > * Read data port to reset things, and then unlink from > * the IRQ chain. > */ > - serial_port_in(port, UART_RX); > + uart_port_lock_irqsave(port, &flags); > + lsr = serial_port_in(port, UART_LSR); > + if (lsr & UART_LSR_DR) > + serial_port_in(port, UART_RX); > + uart_port_unlock_irqrestore(port, flags); > + > serial8250_rpm_put(up); > > up->ops->release_irq(up); >