Re: [PATCH v7 07/10] tty: serial: Add Cortina UART driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] tty: serial: cortina-uart: missing uart_handle_break() breaks
  SysRq
- [Medium] tty: serial: cortina-uart: unconditional RX/TX handling in
  cortina_uart_isr()
--

commit b5b5d422d0304117c1e8a78b6129aa1a61ffa2ba
Author: Fil Dunsky <filipp.dunsky@gmail.com>

tty: serial: Add Cortina UART driver

This commit adds a serial driver for the UART found in Cortina Systems
CS75xx (Goldengate G2) SoCs, with system console and earlycon support.

> diff --git a/drivers/tty/serial/cortina-uart.c b/drivers/tty/serial/cortina-uart.c
> new file mode 100644
> index 0000000000000..72ac035eb8e91
> --- /dev/null
> +++ b/drivers/tty/serial/cortina-uart.c

[ ... ]

> +static void cortina_uart_tx_chars(struct uart_port *port)
> +{
> +	u8 ch;
> +
> +	uart_port_tx(port, ch,
> +		     !(readl(port->membase + CORTINA_UART_INFO) &
> +		       CORTINA_UART_INFO_TX_FULL),
> +		     writel(ch, port->membase + CORTINA_UART_TXDAT));
> +}
> +
> +static void cortina_uart_start_tx(struct uart_port *port)
> +{
> +	cortina_uart_ie_update(port, 0, CORTINA_UART_IRQ_TX_EMPTY);
> +
> +	/*
> +	 * The TX empty interrupt only fires when the FIFO drains, so prime
> +	 * the FIFO here.
> +	 */
> +	cortina_uart_tx_chars(port);
> +}
> +
> +static void cortina_uart_stop_rx(struct uart_port *port)
> +{
> +	cortina_uart_ie_update(port, CORTINA_UART_IRQ_RX_NONEMPTY, 0);
> +}
> +
> +static void cortina_uart_rx_chars(struct uart_port *port, u32 status)
> +{
> +	struct tty_port *tport = &port->state->port;
> +	unsigned int budget = CORTINA_UART_RX_BUDGET;
> +	u32 data, err;
> +
> +	if (status & CORTINA_UART_IRQ_RX_OVERRUN)
> +		port->icount.overrun++;
> +	if (status & CORTINA_UART_IRQ_RX_PARITY)
> +		port->icount.parity++;
> +	if (status & CORTINA_UART_IRQ_RX_STOP)
> +		port->icount.frame++;
> +
> +	/*
> +	 * The parity and framing error status is only latched in the interrupt
> +	 * register, not per character, so charge it to the first character
> +	 * read in this pass.
> +	 */
> +	err = status & port->read_status_mask & CORTINA_UART_IRQ_RX_ERR;
> +
> +	while (budget-- && !(readl(port->membase + CORTINA_UART_INFO) &
> +			     CORTINA_UART_INFO_RX_EMPTY)) {
> +		u8 ch, flag = TTY_NORMAL;
> +		u32 ch_err = err;
> +
> +		data = readl(port->membase + CORTINA_UART_RXDAT);
> +		if (!(data & CORTINA_UART_RXDAT_VALID))
> +			continue;
> +
> +		port->icount.rx++;
> +		ch = FIELD_GET(CORTINA_UART_RXDAT_DATA, data);
> +		err = 0;
> +
> +		if (port->ignore_status_mask & CORTINA_UART_RXDAT_VALID)
> +			continue;
> +		if (ch_err & port->ignore_status_mask)
> +			continue;
> +
> +		if (ch_err & CORTINA_UART_IRQ_RX_PARITY)
> +			flag = TTY_PARITY;
> +		else if (ch_err & CORTINA_UART_IRQ_RX_STOP)
> +			flag = TTY_FRAME;
> +		else if (uart_prepare_sysrq_char(port, ch))
> +			continue;

[Severity: Medium]
Can uart_prepare_sysrq_char() ever succeed without a call to
uart_handle_break()?

uart_prepare_sysrq_char() checks port->sysrq, which is only armed by
uart_handle_break().  Because nothing in the driver calls
uart_handle_break() when a break is received (a NUL character with
CORTINA_UART_IRQ_RX_STOP), port->sysrq stays 0 and serial SysRq and break
handling never trigger.  In addition, cortina_uart_set_termios() only adds
CORTINA_UART_IRQ_RX_STOP to port->read_status_mask when INPCK is set, which
masks stop-bit errors and breaks when INPCK is clear.

> +
> +		tty_insert_flip_char(tport, ch, flag);
> +	}
> +
> +	/* The characters left in the FIFO were received before the lost ones */
> +	if (status & CORTINA_UART_IRQ_RX_OVERRUN &
> +	    ~port->ignore_status_mask)
> +		tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> +
> +	tty_flip_buffer_push(tport);
> +}
> +
> +static irqreturn_t cortina_uart_isr(int irq, void *dev_id)
> +{
> +	struct uart_port *port = dev_id;
> +	u32 status;
> +
> +	guard(uart_port_lock_check_sysrq_irqsave)(port);
> +
> +	status = readl(port->membase + CORTINA_UART_INT);
> +	writel(status, port->membase + CORTINA_UART_INT);
> +
> +	cortina_uart_rx_chars(port, status);
> +	cortina_uart_tx_chars(port);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Should cortina_uart_isr() check which interrupt causes are active before
running the RX and TX handlers?

Calling cortina_uart_tx_chars() -> uart_port_tx() on every RX interrupt
when TX is idle or stopped sees pending < WAKEUP_CHARS and triggers a
spurious uart_write_wakeup() and ->stop_tx() MMIO write.  Likewise, calling
cortina_uart_rx_chars() on a TX-only interrupt schedules flip-buffer work
via tty_flip_buffer_push() even when the RX FIFO is empty, and
unconditionally returning IRQ_HANDLED when status is 0 prevents the
spurious IRQ detector from catching a stuck interrupt.

Also, cortina_uart_start_tx() enables CORTINA_UART_IRQ_TX_EMPTY in
CORTINA_UART_IE before priming the FIFO and without clearing any stale
CORTINA_UART_IRQ_TX_EMPTY bit in CORTINA_UART_INT, which immediately fires
a spurious TX interrupt while the newly filled FIFO is still full.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-b4-cs75xx-v7-0-8f17f38821b4@gmail.com?part=7