Re: [CI v1 1/1] Revert "serial: 8250: Switch to nbcon console, take 2"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] braille: NULL pointer dereference in braille_write() on NBCON
  consoles
- [Medium] serial: 8250_mid: TX corruption and unlocked writes in
  mid8250_set_termios()
- [Low] serial: 8250: false lockdep warning in
  serial8250_em485_stop_tx() during oops
- [Low] serial: 8250: commit message lacks revert rationale and has
  inaccurate notes
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] serial: 8250: integer underflow in wait_for_lsr() [1]
--

commit 2fbf0749c6335f59fdac63a45592d21cc2a1cdd0
Author: Michał Grzelak <michal.grzelak@linux.intel.com>

> Revert "serial: 8250: Switch to nbcon console, take 2"
> 
> This reverts commit d3539347022ad4eeb9dbd29c50bfca17b9d8a146.
> 
> This revert consists of 6 additional reverts; all of them in order of
> applying:
> - Revert "serial: 8250: export and rename wait_for_xmitr()" [2c5942fd0300cb966339dd0366890be626b405e8]
> - Revert "printk: fix kernel-doc name for nbcon_write_context_is_braille" [19ed235c1adcaf40f05997dfe8b96eb209effd35]
> - Revert "braille: nbcon: Allow to use a serial console with NBCON API as Braille console" [c83e3d2912432107148cc5e288ddd153a84c0f43]
> - Revert "serial: 8250: Change console_msr_work to IRQ_WORK_LAZY" [37aba16e7943634eb3e5e2e7e850ebb38500d935]
> - Reapply "serial: 8250: drop lockdep annotation from serial8250_clear_IER()" [bbbb07f9fad6518090b7171dd8f5f737d8ddb63b]
> - Revert "serial: 8250: Switch to nbcon console, take 2" [d3539347022ad4eeb9dbd29c50bfca17b9d8a146]
> - Revert "serial: 8250_mid: wait for LSR tx empty before setting termios" [1c305d5f1daabd317a60b6a2e2ac6b1aff7e05df]
> 
> removed 2 conflicting #include's on top.
> 
> Signed-off-by: Michał Grzelak <michal.grzelak@linux.intel.com>

[Severity: Low]
Could the commit message explain why d3539347022a ("serial: 8250: Switch to
nbcon console, take 2") is being reverted?  Also, the note about removing 2
conflicting #include lines does not match the diff, which does not remove any
#include directives.

> diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
> index 5a3e375f4800f..06b43b678d6ea 100644
> --- a/drivers/accessibility/braille/braille_console.c
> +++ b/drivers/accessibility/braille/braille_console.c
> @@ -62,50 +62,14 @@ static void braille_write(u16 *buf)
>  {
>  	static u16 lastwrite[WIDTH];
>  	unsigned char data[1 + 1 + 2*WIDTH + 2 + 1], csum = 0, *c;
> -	struct nbcon_write_context wctxt = { };
> -	unsigned long flags;
>  	u16 out;
>  	int i;
>  
>  	if (!braille_co)
>  		return;
>  
> -	/*
> -	 * Braille console is not registered in console_list. Instead, it
> -	 * is integrated with VT and shows what appears on the graphical
> -	 * console under console_lock(). From this POV it is a legacy
> -	 * console. But it calls serial console driver which might be
> -	 * converted to the NBCON API. It is similar to
> -	 * nbcon_legacy_emit_next_record() except that we should try
> -	 * harder to get the lock. Otherwise, the Braille device won't show
> -	 * everything what is displayed on the terminal.
> -	 *
> -	 * In short, simulate the original locking using NBCON API.
> -	 */
> -	if (braille_co->flags & CON_NBCON) {
> -		if (panic_on_this_cpu()) {
> -			/*
> -			 * This should be good enough in practice. Most/all
> -			 * serial console drivers have the atomic callback.
> -			 */
> -			if (!braille_co->write_atomic)
> -				return;
> -
> -			local_irq_save(flags);
> -			/* NBCON API strictly requires the ownership. */
> -			if (!nbcon_braille_try_acquire(braille_co, &wctxt)) {
> -				local_irq_restore(flags);
> -				return;
> -			}
> -		} else {
> -			braille_co->device_lock(braille_co, &flags);
> -			while (!nbcon_braille_try_acquire(braille_co, &wctxt))
> -				cpu_relax();
> -		}
> -	}
> -
>  	if (!memcmp(lastwrite, buf, WIDTH * sizeof(*buf)))
> -		goto unlock_nbcon;
> +		return;
>  	memcpy(lastwrite, buf, WIDTH * sizeof(*buf));
>  
>  #define SOH 1
> @@ -138,24 +102,7 @@ static void braille_write(u16 *buf)
>  	*c++ = csum;
>  	*c++ = ETX;
>  
> -	if (braille_co->flags & CON_NBCON) {
> -		nbcon_write_context_set_buf(&wctxt, (char *)data, c - data);
> -		if (panic_on_this_cpu())
> -			braille_co->write_atomic(braille_co, &wctxt);
> -		else
> -			braille_co->write_thread(braille_co, &wctxt);
> -	} else {
> -		braille_co->write(braille_co, data, c - data);
> -	}
> -
> -unlock_nbcon:
> -	if (braille_co->flags & CON_NBCON) {
> -		nbcon_braille_release(&wctxt);
> -		if (panic_on_this_cpu())
> -			local_irq_restore(flags);
> -		else
> -			braille_co->device_unlock(braille_co, flags);
> -	}
> +	braille_co->write(braille_co, data, c - data);
>  }

[Severity: High]
Does reverting c83e3d291243 ("braille: nbcon: Allow to use a serial console
with NBCON API as Braille console") cause a NULL pointer dereference in
braille_write() when a remaining CON_NBCON serial driver is used as a Braille
console?

Other serial console drivers such as amba-pl011, imx, and sifive remain
CON_NBCON consoles with .write set to NULL.  Booting with console=brl,ttyAMA0
(or ttymxc0 / ttySIF0) will crash on the first braille_co->write() call, and
those drivers also lose the !nbcon_is_braille(co) checks in their
write_atomic() callbacks.  Only the 8250_port.c hunk of c83e3d291243 should
be dropped when reverting d3539347022a ("serial: 8250: Switch to nbcon
console, take 2").

[ ... ]

> diff --git a/drivers/tty/serial/8250/8250_mid.c b/drivers/tty/serial/8250/8250_mid.c
> index df721418e97ef..0f62961ca9d09 100644
> --- a/drivers/tty/serial/8250/8250_mid.c
> +++ b/drivers/tty/serial/8250/8250_mid.c
> @@ -211,7 +211,6 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  				const struct ktermios *old)
>  {
>  	unsigned int baud = tty_termios_baud_rate(termios);
> -	struct uart_8250_port *up = up_to_u8250p(p);
>  	struct mid8250 *mid = p->private_data;
>  	unsigned short ps = 16;
>  	unsigned long fuart = baud * ps;
> @@ -234,16 +233,11 @@ static void mid8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  	}
>  
>  	rational_best_approximation(fuart, mid->board->freq, w, w, &mul, &div);
> +	p->uartclk = fuart * 16 / ps;		/* core uses ps = 16 always */
>  
> -	scoped_guard(spinlock_irq, &p->lock) {
> -		p->uartclk = fuart * 16 / ps;	/* core uses ps = 16 always */
> -
> -		serial8250_wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
> -
> -		writel(ps, p->membase + INTEL_MID_UART_PS);	/* set PS */
> -		writel(mul, p->membase + INTEL_MID_UART_MUL);	/* set MUL */
> -		writel(div, p->membase + INTEL_MID_UART_DIV);
> -	}
> +	writel(ps, p->membase + INTEL_MID_UART_PS);		/* set PS */
> +	writel(mul, p->membase + INTEL_MID_UART_MUL);		/* set MUL */
> +	writel(div, p->membase + INTEL_MID_UART_DIV);
>  
>  	serial8250_do_set_termios(p, termios, old);
>  }

[Severity: Medium]
Does reverting 1c305d5f1daa ("serial: 8250_mid: wait for LSR tx empty before
setting termios") and 2c5942fd0300 ("serial: 8250: export and rename
wait_for_xmitr()") reintroduce TX data corruption and unlocked register
writes in mid8250_set_termios()?

Those commits fixed an independent bug in 8250_mid where changing termios
while characters are in flight reprograms INTEL_MID_UART_PS,
INTEL_MID_UART_MUL, and INTEL_MID_UART_DIV and updates p->uartclk without
holding p->lock or waiting for UART_LSR_BOTH_EMPTY.  Reverting them also
leaves behind an unused #include <linux/cleanup.h> and
MODULE_IMPORT_NS("SERIAL_8250") in drivers/tty/serial/8250/8250_mid.c.

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 36b2b0a2c05cd..5777db9c6d89e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c

[ ... ]

> @@ -1319,6 +1306,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
>  {
>  	unsigned char mcr = serial8250_in_MCR(p);
>  
> +	/* Port locked to synchronize UART_IER access against the console. */
> +	lockdep_assert_held_once(&p->port.lock);
> +
>  	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
>  		mcr |= UART_MCR_RTS;
>  	else
> @@ -1334,16 +1324,6 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
>  		serial8250_clear_and_reinit_fifos(p);
>  
>  		if (toggle_ier) {
> -			/*
> -			 * Port locked to synchronize UART_IER access against
> -			 * the console. The lockdep_assert must be restricted
> -			 * to this condition because only here is it
> -			 * guaranteed that the port lock is held. The other
> -			 * hardware access in this function is synchronized
> -			 * by console ownership.
> -			 */
> -			lockdep_assert_held_once(&p->port.lock);
> -
>  			p->ier |= UART_IER_RLSI | UART_IER_RDI;
>  			serial_port_out(&p->port, UART_IER, p->ier);
>  		}

[Severity: Low]
Can moving lockdep_assert_held_once(&p->port.lock) out of the if (toggle_ier)
block trigger a false lockdep warning during an oops?

When oops_in_progress is set, serial8250_console_write() uses
uart_port_trylock_irqsave() and continues unlocked if the trylock fails,
calling up->rs485_stop_tx(up, false) at the end of the transmission.  Since
UART_IER is only touched when toggle_ier is true, asserting p->port.lock
unconditionally at function entry will warn when locked == 0.

> @@ -2047,7 +2027,7 @@ static bool wait_for_lsr(struct uart_8250_port *up, int bits)
>  }
>  
>  /* Wait for transmitter and holding register to empty with timeout */
> -void serial8250_wait_for_xmitr(struct uart_8250_port *up, int bits)
> +static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>  {
>  	unsigned int tmout;
>  	bool tx_ready;

[Severity: High]
This problem wasn't introduced by this patch, but can tmout underflow to
UINT_MAX in wait_for_lsr() [1] at very high baud rates?

drivers/tty/serial/8250/8250_port.c:wait_for_lsr() {
	if (up->port.frame_time)
		tmout = up->port.frame_time * 2 / NSEC_PER_USEC;
	else
		tmout = 10000;

	for (;;) {
		status = serial_lsr_in(up);

		if ((status & bits) == bits)
			break;
		if (--tmout == 0)
			break;
		udelay(1);
		touch_nmi_watchdog();
	}
}

When a fast UART runs at a baud rate with a frame_time below 500 ns (such as
24 Mbps), up->port.frame_time * 2 / NSEC_PER_USEC truncates tmout to 0.  If
the LSR bits are not immediately set, --tmout wraps around to UINT_MAX and
spins with udelay(1) for up to 4.2 billion iterations (~71 minutes), locking
up the CPU.

[1] https://sashiko.dev/#/bug/linux-7e6ea87e-575e-4a64-826f-d64665ba4b15

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115122441.3141883-1-michal.grzelak@linux.intel.com?part=1