Re: [PATCH v4 2/7] 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 2 potential
issues to consider:
- [High] serial: sh-sci: missing sci_stop_tx() when stopped in
  sci_transmit_chars()
- [Medium] serial: sh-sci: premature TE disable in
  sci_tx_end_interrupt() during active TX
--

commit 2144acd3ea7e3d82d9a7516af61337ce1de099e8
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
x_char or unstopped xmit_fifo data is pending, and updates
sci_transmit_chars() and rsci_transmit_chars() to switch from TIE to TEIE
when transmission 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
[ ... ]
> @@ -1007,6 +1011,20 @@ static int sci_rxfill(struct uart_port *port)
>   *                   the interrupt related routines                       *
>   * ********************************************************************** */
>  
> +static void sci_clr_tie_set_teie(struct uart_port *port)
> +{
> +	struct sci_port *s = to_sci_port(port);
> +	unsigned short ctrl;
> +
> +	if (s->type != PORT_SCI)
> +		return;
> +
> +	ctrl = sci_serial_in(port, SCSCR);
> +	ctrl &= ~SCSCR_TIE;
> +	ctrl |= SCSCR_TEIE;
> +	sci_serial_out(port, SCSCR, ctrl);
> +}
> +
>  static void sci_transmit_chars(struct uart_port *port)
>  {
>  	struct tty_port *tport = &port->state->port;
> @@ -1050,14 +1068,10 @@ static void sci_transmit_chars(struct uart_port *port)
>  	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
>  		uart_write_wakeup(port);
>  	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_clr_tie_set_teie(port);
>  		sci_stop_tx(port);
> +	} else if (stopped) {
> +		sci_clr_tie_set_teie(port);
>  	}
>  }

[Severity: High]
Does the "else if (stopped)" branch leave SCSCR_TIE enabled on
non-PORT_SCI ports?

sci_clr_tie_set_teie() returns immediately when s->type != PORT_SCI, and
this branch does not call sci_stop_tx().  If sci_transmit_chars() runs on
a PORT_SCIF, PORT_SCIFA, PORT_SCIFB, PORT_HSCIF, or PORT_IRDA port while
stopped with data still in xmit_fifo (for example, after uart_send_xchar()
calls sci_start_tx()), SCSCR_TIE remains set and triggers a continuous TXI
interrupt storm until transmission is unstopped.

> @@ -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: Medium]
Can sci_tx_end_interrupt() still clear TE while newly written bytes are
being transmitted?

When transmit_chars() drains xmit_fifo, it clears TIE and sets TEIE while
the last byte is still shifting out.  If uart_write() runs before that
byte finishes, sci_start_tx() or rsci_start_tx() sets TIE without clearing
TEIE, leaving both interrupts enabled:

CPU0                              CPU1
transmit_chars()
  /* drains xmit_fifo */
  ctrl &= ~TIE;
  ctrl |= TEIE;
uart_write()
  start_tx()
    ctrl |= TIE | TE;
    /* TIE and TEIE both set */
                                  /* TDR transfers to shift reg: TXI */
                                  /* shift reg finishes: TEI */
                                  sci_tx_interrupt()
                                    lock(port)
                                    transmit_chars()
                                      /* writes new byte to TDR,
                                         drains xmit_fifo */
                                    unlock(port)
                                  sci_tx_end_interrupt()
                                    lock(port)
                                    /* xmit_fifo is now empty */
                                    ctrl &= ~te_clear;
                                      <-- clears TE during active TX
                                    unlock(port)

Because sci_tx_end_interrupt() checks only x_char and xmit_fifo without
verifying in regs->status that poll_sent_bits (TDRE and TEND) are still
set in hardware, it disables the transmitter before the newly written byte
has been sent.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-g2ul_sci-v4-0-6815e41d90c5@renesas.com?part=2