Re: [PATCH 3/4] serial: sh-sci: Skip TDxE status clear for PORT_SCI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: sh-sci: broken TX on SuperH PORT_SCI from skipping clear_SCxSR()
- [Medium] serial: sh-sci: unlocked control register RMW in sci_tx_end_interrupt()
--

commit c9eeab42d1c45b13adb330fd8baab03f6ddbfeb6
Author: Au Dang <au.dang.jx@renesas.com>

serial: sh-sci: Skip TDxE status clear for PORT_SCI

This commit skips calling clear_SCxSR() to clear the TDxE status bit for
PORT_SCI in sci_poll_put_char() and sci_transmit_chars(), relying on hardware
to automatically clear the bit when data is written to TDR.

> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index d577f9de62595..e4c7a3e146884 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -913,7 +913,8 @@ static void sci_poll_put_char(struct uart_port *port, unsigned char c)
>  	} while (!(status & SCxSR_TDxE(port)));
>  
>  	sci_serial_out(port, SCxTDR, c);
> -	s->ops->clear_SCxSR(port, SCxSR_TDxE_CLEAR(port) & ~SCxSR_TEND(port));
> +	if (s->type != PORT_SCI)
> +		s->ops->clear_SCxSR(port, SCxSR_TDxE_CLEAR(port) & ~SCxSR_TEND(port));
>  }
>  #endif /* CONFIG_CONSOLE_POLL || CONFIG_SERIAL_SH_SCI_CONSOLE ||
>  	  CONFIG_SERIAL_SH_SCI_EARLYCON */

[Severity: High]
Will skipping clear_SCxSR() for all PORT_SCI ports break transmission on
classic SuperH SCI hardware?

In drivers/tty/serial/sh-sci.c, PORT_SCI is shared between DT-based Renesas
RZ/G2L SCIg ports (such as renesas,r9a07g044-sci matching renesas,sci) and
classic non-DT SuperH SCI ports (SH770x, SH7750, and SH7760 registered via
plat_sci_port with .type = PORT_SCI).

While RZ/G2L SCIg hardware automatically clears SSR.TDRE when transmit data is
written to TDR, classic SuperH SCI hardware does not clear SCSSR.TDRE (or
TEND) on CPU writes to SCTDR. On classic SuperH SCI, software must read SCSSR
with TDRE == 1, write the byte to SCTDR, and then explicitly clear TDRE to 0
via clear_SCxSR() before the hardware transfers SCTDR to the transmit shift
register (SCTSR) and begins shifting.

With this check in both sci_poll_put_char() and sci_transmit_chars(),
SCSSR.TDRE is never cleared on classic SuperH PORT_SCI systems, preventing
SCTDR from transferring to SCTSR in interrupt-driven transmission and causing
sci_poll_put_char() to see TDRE remain 1 and overwrite SCTDR without shifting
characters out.

Notice also that sci_poll_put_char() and sci_transmit_chars() are the only
callers of SCxSR_TDxE_CLEAR(port):

#define SCxSR_TDxE_CLEAR(port) \
	(((port)->type == PORT_SCI) ? SCI_TDxE_CLEAR : SCIF_TDxE_CLEAR)

Skipping both calls when s->type == PORT_SCI leaves SCI_TDxE_CLEAR and the
PORT_SCI branch in SCxSR_TDxE_CLEAR() completely unreachable.

> @@ -1049,7 +1050,8 @@ static void sci_transmit_chars(struct uart_port *port)
>  		port->icount.tx++;
>  	} while (--count > 0);
>  
> -	s->ops->clear_SCxSR(port, SCxSR_TDxE_CLEAR(port));
> +	if (s->type != PORT_SCI)
> +		s->ops->clear_SCxSR(port, SCxSR_TDxE_CLEAR(port));
>  
>  	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
>  		uart_write_wakeup(port);

[Severity: Medium]
Now that PORT_SCI avoids the extra TXI interrupt and transitions directly to
TEIE-driven completion when xmit_fifo becomes empty in sci_transmit_chars():

	if (kfifo_is_empty(&tport->xmit_fifo)) {
		if (s->type == PORT_SCI) {
			ctrl = sci_serial_in(port, SCSCR);
			ctrl &= ~SCSCR_TIE;
			ctrl |= SCSCR_TEIE;
			sci_serial_out(port, SCSCR, ctrl);
		}

		sci_stop_tx(port);
	}

can the unlocked control register read in sci_tx_end_interrupt() race with a
concurrent sci_start_tx() and stall transmission?

In drivers/tty/serial/sh-sci.c:sci_tx_end_interrupt(), the control register
and xmit_fifo are read before acquiring port->lock:

	ctrl = s->ops->read_reg(port, regs->control);
	if (!kfifo_is_empty(&tport->xmit_fifo)) {
		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->params->param_bits->te_clear);
	s->ops->write_reg(port, regs->control, ctrl);
	uart_port_unlock_irqrestore(port, flags);

If CPU A enters sci_tx_end_interrupt(), reads regs->control (with TIE == 0),
and sees kfifo_is_empty(&tport->xmit_fifo) == true before acquiring
port->lock, CPU B can concurrently run uart_write() -> __uart_start() ->
sci_start_tx() under port->lock, queueing new bytes into xmit_fifo and setting
SCSCR_TE | SCSCR_TIE in regs->control.

When CPU A then acquires port->lock and writes back its stale ctrl value with
te_clear masked off, it overwrites the TIE and TE bits just enabled by
sci_start_tx(), leaving the newly queued bytes stuck in xmit_fifo. Similarly,
if kfifo_is_empty() is false, reading ctrl outside port->lock can clobber
concurrent control register updates made under port->lock by sci_start_rx() or
sci_stop_rx().

Should sci_tx_end_interrupt() acquire port->lock before reading regs->control
and checking kfifo_is_empty(&tport->xmit_fifo)?

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