| Message ID | 20250513024212.74658-1-cuiyunhui@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/4] serial: 8250: fix panic due to PSLVERR | expand |
Hi All, Gentle ping. Any comments on this patchset? On Tue, May 13, 2025 at 10:42 AM Yunhui Cui <cuiyunhui@bytedance.com> 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 serial_port_in(port, UART_RX). > This satisfies the PSLVERR trigger condition. > > When 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) in dw8250_check_lcr(), causing it to enter > dw8250_force_idle(). > > Put serial_port_out(port, UART_LCR, UART_LCR_WLEN8) under the port->lock > to fix this issue. > > 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 <cuiyunhui@bytedance.com> > --- > drivers/tty/serial/8250/8250_port.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 6d7b8c4667c9c..07fe818dffa34 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2376,9 +2376,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; > -- > 2.39.2 > Thanks, Yunhui
On 2025-05-13, Yunhui Cui <cuiyunhui@bytedance.com> wrote: > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 07fe818dffa34..9a04f24b0c762 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2133,25 +2132,22 @@ 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; > + unsigned long flags; > > serial8250_rpm_get(up); > > + uart_port_lock_irqsave(port, &flags); > lsr = serial_port_in(port, UART_LSR); > + if (lsr & UART_LSR_DR) > + status = serial_port_in(port, UART_RX); > + uart_port_unlock_irqrestore(port, flags); I realize I previously made a comment saying it was OK to add the spin locking here. But I have changed my mind. Please remove this spin locking. It is not necessary because with kgdb all the other CPUs are quiesced, so there is no need to synchronize with the console. Also, it will deadlock if kgdb took over while the port was locked. > @@ -2513,7 +2514,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 > @@ -2521,6 +2521,14 @@ 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); > + /* > + * Read data port to reset things, and then unlink from > + * the IRQ chain. > + * Since reading UART_RX clears interrupts, doing so with > + * FIFO disabled won't trigger PSLVERR. > + */ > + serial_port_in(port, UART_RX); > + uart_port_unlock_irqrestore(port, flags); > > #ifdef CONFIG_SERIAL_8250_RSA > /* > @@ -2529,11 +2537,6 @@ void serial8250_do_shutdown(struct uart_port *port) > disable_rsa(up); > #endif > > - /* > - * Read data port to reset things, and then unlink from > - * the IRQ chain. > - */ > - serial_port_in(port, UART_RX); I am thinking you should keep the read here and instead move the unlock below the read. This would mean the lock/unlock in disable_rsa() need to be removed. (The function comments for disable_rsa() aready say that the caller needs to hold the port lock.) I am thinking something like the below (untested) diff instead of the above 2 hunks. John Ogness diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 22b3f7a193070..51467383aaf5a 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -781,8 +781,6 @@ static void disable_rsa(struct uart_8250_port *up) if (up->port.type == PORT_RSA && up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) { - uart_port_lock_irq(&up->port); - mode = serial_in(up, UART_RSA_MSR); result = !(mode & UART_RSA_MSR_FIFO); @@ -794,7 +792,6 @@ static void disable_rsa(struct uart_8250_port *up) if (result) up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16; - uart_port_unlock_irq(&up->port); } } #endif /* CONFIG_SERIAL_8250_RSA */ @@ -2536,7 +2533,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 @@ -2555,8 +2551,12 @@ void serial8250_do_shutdown(struct uart_port *port) /* * Read data port to reset things, and then unlink from * the IRQ chain. + * + * Since reading UART_RX clears interrupts, doing so with + * FIFO disabled won't trigger PSLVERR. */ serial_port_in(port, UART_RX); + uart_port_unlock_irqrestore(port, flags); serial8250_rpm_put(up); up->ops->release_irq(up);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 6d7b8c4667c9c..07fe818dffa34 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2376,9 +2376,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;
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 serial_port_in(port, UART_RX). This satisfies the PSLVERR trigger condition. When 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) in dw8250_check_lcr(), causing it to enter dw8250_force_idle(). Put serial_port_out(port, UART_LCR, UART_LCR_WLEN8) under the port->lock to fix this issue. 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 <cuiyunhui@bytedance.com> --- drivers/tty/serial/8250/8250_port.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)