Re: [PATCH 2/4] serial: sh-sci: Don’t disable TX on TEI while the xmit kfifo still holds data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: sh-sci: race on control reg and xmit_fifo in sci_tx_end_interrupt()
- [Medium] serial: sh-sci: sci_tx_end_interrupt() ignores x_char and uart_tx_stopped()
--

commit c11148ecd71f7b39ea459c7c652a2f9e70cefdac
Author: Biju Das <biju.das.jz@bp.renesas.com>

serial: sh-sci: Don’t disable TX on TEI while the xmit kfifo still holds data

This commit checks whether the transmit kfifo is empty in
sci_tx_end_interrupt() before clearing TE and TEIE on SCI and RSCI ports. If
data remains queued, it clears TEIE, sets TIE, and hands over to
sci_tx_interrupt() so transmission can continue.

> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 9eb297dbc9010..d577f9de62595 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

[ ... ]

> @@ -2012,6 +2016,7 @@ static irqreturn_t sci_tx_end_interrupt(int irq, void *ptr)
>  {
>  	struct uart_port *port = ptr;
>  	struct sci_port *s = to_sci_port(port);
> +	struct tty_port *tport = &port->state->port;
>  	const struct sci_common_regs *regs = s->params->common_regs;
>  	unsigned long flags;
>  	u32 ctrl;
> @@ -2019,9 +2024,18 @@ static irqreturn_t sci_tx_end_interrupt(int irq, void *ptr)
>  	if (s->type != PORT_SCI && !sci_is_rsci_type(s->type))
>  		return sci_tx_interrupt(irq, ptr);
>  
> +	ctrl = s->ops->read_reg(port, regs->control);
> +	if (!kfifo_is_empty(&tport->xmit_fifo)) {

[Severity: Medium]
What happens if transmission is stopped via flow control or if a high-priority
XON/XOFF character is pending in port->x_char when sci_tx_end_interrupt()
runs?

In sci_transmit_chars() and rsci_transmit_chars(), the transmit loop checks
both port->x_char and uart_tx_stopped(port):

	if (port->x_char) {
		c = port->x_char;
		port->x_char = 0;
	} else if (stopped || !kfifo_get(&tport->xmit_fifo, &c)) {
		break;
	}

If uart_tx_stopped(port) is true while tport->xmit_fifo is non-empty,
sci_tx_end_interrupt() will clear TEIE, set TIE (leaving TE set), and call
sci_tx_interrupt() -> transmit_chars(). Because stopped is true,
transmit_chars() breaks out of the loop without writing to the transmit data
register and without clearing TE or re-enabling TEIE.

On SCI and RSCI, starting transmission from an idle state requires a 0-to-1
transition on TE alongside TIE. Leaving the transmitter idle with TE and TIE
already set means a later uart_start() -> sci_start_tx() or rsci_start_tx()
sees TE already set and cannot trigger a TXI interrupt, wedging transmission.

Conversely, if tport->xmit_fifo is empty but port->x_char is non-zero (queued
via uart_send_xchar()), checking only !kfifo_is_empty(&tport->xmit_fifo) will
shut down TE and TEIE without transmitting port->x_char.

Should sci_tx_end_interrupt() check port->x_char ||
(!kfifo_is_empty(&tport->xmit_fifo) && !uart_tx_stopped(port)) while holding
the port lock?

> +		uart_port_lock_irqsave(port, &flags);
> +		ctrl &= ~(s->params->param_bits->teie);
> +		ctrl |= s->params->param_bits->tie;
> +		s->ops->write_reg(port, regs->control, ctrl);
> +		uart_port_unlock_irqrestore(port, flags);
> +		return sci_tx_interrupt(irq, ptr);
> +	}
> +
>  	uart_port_lock_irqsave(port, &flags);
> -	ctrl = s->ops->read_reg(port, regs->control) &
> -		~(s->params->param_bits->te_clear);
> +	ctrl &= ~(s->params->param_bits->te_clear);
>  	s->ops->write_reg(port, regs->control, ctrl);
>  	uart_port_unlock_irqrestore(port, flags);

[Severity: High]
Can reading regs->control and checking kfifo_is_empty(&tport->xmit_fifo)
before acquiring port->lock race with concurrent updates to the control
register and xmit_fifo?

In the serial core, uart_write() holds port->lock while queuing data into
tport->xmit_fifo and calling __uart_start() -> sci_start_tx() or
rsci_start_tx():

drivers/tty/serial/sh-sci.c:sci_start_tx()
	ctrl = sci_serial_in(port, SCSCR);
	if (s->type == PORT_SCI)
		ctrl |= SCSCR_TE;
	sci_serial_out(port, SCSCR, ctrl | SCSCR_TIE);

drivers/tty/serial/rsci.c:rsci_start_tx()
	ctrl = rsci_serial_in(port, CCR0);
	ctrl |= CCR0_TIE | CCR0_TE;
	rsci_serial_out(port, CCR0, ctrl);

If sci_tx_end_interrupt() on CPU 0 reads regs->control (with TE=1, TEIE=1,
TIE=0) and sees kfifo_is_empty(&tport->xmit_fifo) as true before acquiring
port->lock, CPU 1 can concurrently run uart_write(), enqueue bytes into
tport->xmit_fifo, and call start_tx() to set TE and TIE in regs->control. When
CPU 0 then acquires port->lock and writes back its stale ctrl & ~te_clear, it
overwrites the TIE and TE bits set by CPU 1 and disables the transmitter while
newly queued data remains in tport->xmit_fifo.

Similarly, any other concurrent modification to regs->control under port->lock
(such as sci_stop_tx(), rsci_stop_tx(), sci_start_rx(), sci_stop_rx(),
rsci_stop_rx(), sci_break_ctl(), or rsci_break_ctl()) between the unlocked
read_reg() and write_reg() will be clobbered by the stale ctrl value.

In addition, in the !kfifo_is_empty() branch, port->lock is dropped before
calling sci_tx_interrupt(), which immediately re-acquires port->lock to call
s->ops->transmit_chars(port). Should port->lock be held across the entire
read_reg(), fifo check, write_reg(), and s->ops->transmit_chars(port) sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019170139.473808-1-biju.das.jz@bp.renesas.com?part=2