Re: [PATCH] serial: 8250: fix panic due to PSLVERR

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Apr 03, 2025 at 02:36:16PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 03, 2025 at 05:03:36PM +0800, Yunhui Cui wrote:

A couple of more questions here:
1) what is the DW IP version and where did you get the PSLVERR_RESP_EN
parameter from?
2) what is the setting of the UART_16550_COMPATIBLE parameter?

> > 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,
> 
> serial8250_do_startup()
> 
> > 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
> 
> printk()
> 
> > is busy), the current CPU fails the check (value & ~UART_LCR_SPAR) ==
> > (lcr & ~UART_LCR_SPAR), causing it to enter dw8250_force_idle().
> > 
> > To resolve this issue, relevant serial_port_out operations should be
> 
> serial_port_out()
> 
> > placed in a critical section, and UART_RX data should only be read
> > when the UART_LSR DR bit is set.
> 
> The last one is made in the common code, are you sure that all supported UARTs
> will be okay with such a change?
> 
> > Panic message:
> 
> Please, read this
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
> and act accordingly.
> 
> > [    0.442336] Oops - unknown exception [#1]
> > [    0.442337] Modules linked in:
> > [    0.442339] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.13-00102-gf1f43e345877 #1
> 
> Is it still reproducible on v6.14 (and soon v6.15-rc1)?
> 
> > [    0.442342] Tainted: [W]=WARN
> > [    0.442343] epc : dw8250_serial_in32+0x1e/0x4a
> > [    0.442351]  ra : serial8250_do_startup+0x2c8/0x88e
> > [    0.442354] epc : ffffffff8064efca ra : ffffffff8064af28 sp : ffff8f8000103990
> > [    0.442355]  gp : ffffffff815bad28 tp : ffffaf807e36d400 t0 : ffffaf80804cf080
> > [    0.442356]  t1 : 0000000000000001 t2 : 0000000000000000 s0 : ffff8f80001039a0
> > [    0.442358]  s1 : ffffffff81626fc0 a0 : ffffffff81626fc0 a1 : 0000000000000000
> > [    0.442359]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : ffffffff81626fc0
> > [    0.442360]  a5 : ffff8f800012d900 a6 : 000000000000000f a7 : 000000000fc648c1
> > [    0.442361]  s2 : 0000000000000000 s3 : 0000000200000022 s4 : 0000000000000000
> > [    0.442362]  s5 : ffffffff81626fc0 s6 : ffffaf8085227000 s7 : ffffffff81073c58
> > [    0.442363]  s8 : 0000000000500000 s9 : ffffaf80851a5a60 s10: ffffaf80851a5a60
> > [    0.442365]  s11: ffffffff80e85980 t3 : ffffaf807e324600 t4 : 0000000000000002
> > [    0.442365]  t5 : 0000000000000003 t6 : ffffaf80804cf072
> > [    0.442366] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000013
> > [    0.442368] [<ffffffff8064efca>] dw8250_serial_in32+0x1e/0x4a
> > [    0.442371] [<ffffffff8064af28>] serial8250_do_startup+0x2c8/0x88e
> > [    0.442373] [<ffffffff8064b514>] serial8250_startup+0x26/0x2e
> > [    0.442375] [<ffffffff806428a2>] uart_startup+0x13a/0x308
> > [    0.442377] [<ffffffff80642aa4>] uart_port_activate+0x34/0x50
> > [    0.442378] [<ffffffff8062ab6a>] tty_port_open+0xb4/0x110
> > [    0.442383] [<ffffffff8063f548>] uart_open+0x22/0x36
> > [    0.442389] [<ffffffff806234b4>] tty_open+0x1be/0x5e6
> > [    0.442396] [<ffffffff802f2d52>] chrdev_open+0x10a/0x2a8
> > [    0.442400] [<ffffffff802e7ab6>] do_dentry_open+0xf6/0x34e
> > [    0.442405] [<ffffffff802e9456>] vfs_open+0x2a/0xb4
> > [    0.442408] [<ffffffff80300124>] path_openat+0x676/0xf36
> > [    0.442410] [<ffffffff80300a58>] do_filp_open+0x74/0xfa
> > [    0.442412] [<ffffffff802e9900>] file_open_name+0x84/0x144
> > [    0.442414] [<ffffffff802e99f6>] filp_open+0x36/0x54
> > [    0.442416] [<ffffffff80a01232>] console_on_rootfs+0x26/0x70
> > [    0.442420] [<ffffffff80a0154e>] kernel_init_freeable+0x2d2/0x30e
> > [    0.442422] [<ffffffff8099c730>] kernel_init+0x2a/0x15e
> > [    0.442427] [<ffffffff809a7666>] ret_from_fork+0xe/0x1c
> > [    0.442430] Code: e022 e406 0800 4683 0c15 691c 872a 96bb 00d5 97b6 (439c) 851b
> > [    0.442432] ---[ end trace 0000000000000000 ]---
> > [    0.442434] Kernel panic - not syncing: Fatal exception in interrupt
> > [    0.442435] SMP: stopping secondary CPUs
> > [    0.451111] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
> 
> Fixes tag?
> Cc to stable@?
> 
> ...
> 
> >  	/*
> >  	 * Now, initialize the UART
> >  	 */
> 
> + Blank line.
> 
> > +	uart_port_lock_irqsave(port, &flags);
> >  	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
> >  
> > -	uart_port_lock_irqsave(port, &flags);
> >  	if (up->port.flags & UPF_FOURPORT) {
> >  		if (!up->port.irq)
> >  			up->port.mctrl |= TIOCM_OUT1;

-- 
With Best Regards,
Andy Shevchenko