Re: [PATCH v3 2/4] serial: sh-sci: Keep TX enabled on TEI while data is pending

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: sh-sci: race in sci_tx_end_interrupt() disables TE during active TX
--

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

serial: sh-sci: Keep TX enabled on TEI while data is pending

This commit keeps the transmitter enabled in sci_tx_end_interrupt() when
pending data exists in the xmit kfifo or x_char, handing over to the port's
transmit_chars() callback instead of unconditionally clearing TE and TEIE. It
also swaps TIE for TEIE in sci_transmit_chars() and rsci_transmit_chars() when
the port is stopped.

> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 9eb297dbc9010..6df25fef5a5f5 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
[ ... ]
> @@ -2012,6 +2026,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;
> @@ -2020,8 +2035,18 @@ static irqreturn_t sci_tx_end_interrupt(int irq, void *ptr)
>  		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->ops->read_reg(port, regs->control);
> +	if (port->x_char || (!kfifo_is_empty(&tport->xmit_fifo) &&
> +			     !uart_tx_stopped(port))) {
> +		ctrl &= ~(s->params->param_bits->teie);
> +		ctrl |= s->params->param_bits->tie;
> +		s->ops->write_reg(port, regs->control, ctrl);
> +		s->ops->transmit_chars(port);
> +		uart_port_unlock_irqrestore(port, flags);
> +		return IRQ_HANDLED;
> +	}
> +
> +	ctrl &= ~(s->params->param_bits->te_clear);
>  	s->ops->write_reg(port, regs->control, ctrl);
>  	uart_port_unlock_irqrestore(port, flags);

[Severity: High]
Can sci_tx_end_interrupt() clear TE while newly written bytes are still
being transmitted by the hardware?

When the last byte in tport->xmit_fifo is written to the transmit data
register, sci_transmit_chars() and rsci_transmit_chars() clear TIE and set
TEIE. If new data is queued before TEI is serviced, sci_start_tx() in
drivers/tty/serial/sh-sci.c and rsci_start_tx() in drivers/tty/serial/rsci.c
enable TIE without clearing TEIE:

drivers/tty/serial/sh-sci.c:sci_start_tx() {
	...
		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);
}

In addition, when sci_tx_end_interrupt() takes the pending-data branch, it
sets s->params->param_bits->tie in regs->control while TDRE is asserted before
calling s->ops->transmit_chars(port). Even if transmit_chars() consumes all
pending bytes and clears TIE while setting TEIE, the rising edge on
SCIx_TXI_IRQ is already latched in the interrupt controller.

Because SCIx_TXI_IRQ (sci_tx_interrupt) and SCIx_TEI_IRQ
(sci_tx_end_interrupt) are separate interrupts, sci_tx_interrupt() can acquire
port->lock before a concurrent or pending sci_tx_end_interrupt() does. If
transmit_chars() drains all newly queued bytes from tport->xmit_fifo or
port->x_char into the hardware transmit data register or FIFO, the software
queues become empty while the hardware is still actively shifting out those
bytes (TEND is 0).

When sci_tx_end_interrupt() subsequently acquires port->lock, it only checks
port->x_char and kfifo_is_empty(&tport->xmit_fifo) without checking
regs->status (such as s->params->param_bits->poll_sent_bits for TEND and TDRE)
to verify that hardware transmission has actually finished. Wouldn't it then
clear s->params->param_bits->te_clear, shutting down the transmitter mid-frame
and corrupting or dropping the in-flight serial data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-g2ul_sci-v3-0-b3097163edd5@bp.renesas.com?part=2