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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: unconditional x_char write overflows full TX FIFO in uart_port_tx
- [Medium] tty: missing break detection makes sysrq unreachable
- [Medium] tty: 10ms timeout in putchar causes console data loss at low baud rates
--

commit cd8f7c82aa6edac81dac7b24361325875ebfc80f
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
and Cortina Access SoCs. It implements the current serial core API, managing
devm resources, transmit and receive FIFOs, and supports system console and
earlycon.

> diff --git a/drivers/tty/serial/cortina-uart.c b/drivers/tty/serial/cortina-uart.c
> index 0000000000000..899fc20978ca0
> --- /dev/null
> +++ b/drivers/tty/serial/cortina-uart.c
> @@ -0,0 +1,595 @@

[ ... ]

> +static void __maybe_unused cortina_uart_putchar(struct uart_port *port,
> +						unsigned char ch)
> +{
> +	u32 info;
> +
> +	readl_poll_timeout_atomic(port->membase + CORTINA_UART_INFO, info,
> +				  !(info & CORTINA_UART_INFO_TX_FULL), 1,
> +				  CORTINA_UART_POLL_TIMEOUT_US);
> +	writel(ch, port->membase + CORTINA_UART_TXDAT);

[Severity: Medium]
Will this hardcoded 10ms timeout cause console data loss at low baud rates?

The timeout is set to CORTINA_UART_POLL_TIMEOUT_US (10,000 microseconds) to wait
for space in the TX FIFO. If the FIFO remains full and the timeout expires, the
driver ignores the timeout error and executes writel() anyway, which could
overwrite the FIFO and drop characters.

At low baud rates, such as 300 baud, a single 10-bit character takes about
33.3ms to transmit. When the FIFO is full at this speed, the putchar function
would consistently time out because 10ms is not enough time for even a single
byte to free up, resulting in guaranteed data corruption during earlycon and
console output.

Should the timeout scale dynamically with port->frame_time, or perhaps spin
indefinitely?

> +}

[ ... ]

> +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));

[Severity: High]
Does calling uart_port_tx() without first verifying available space risk
overflowing the hardware TX FIFO?

The driver calls cortina_uart_tx_chars() unconditionally from the ISR, even
on purely RX interrupts. During an active transmit workload, the TX FIFO
could be completely full.

The uart_port_tx() macro writes a pending port->x_char (e.g. for software
flow control) using the provided put_char expression immediately, without
evaluating the tx_ready expression first. It assumes the caller has already
ensured at least one byte of space is available.

If an x_char is pending and the TX FIFO is full, the put_char expression
will execute regardless, overflowing the FIFO and corrupting the hardware
state. Should we explicitly check for TX FIFO space and return early before
calling uart_port_tx()?

> +}

[ ... ]

> +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 (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]
Is the sysrq character handling reachable here without break detection?

The driver attempts to intercept sysrq characters with
uart_prepare_sysrq_char() and claims to support sysrq by setting
port->has_sysrq. However, uart_prepare_sysrq_char() only intercepts
characters if the port is already in the sysrq state (port->sysrq is
non-zero).

The sysrq state is traditionally entered when a BREAK condition is received,
requiring the driver to call uart_handle_break(port) to set port->sysrq
with a timestamp.

Since this driver does not appear to detect a break condition (e.g. checking
for a framing error with a null byte) and never calls uart_handle_break(),
is there another mechanism intended to place the port into the sysrq state,
or does this leave the sysrq feature non-functional?

> +
> +		tty_insert_flip_char(tport, ch, flag);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-b4-cs75xx-v4-0-df250d903682@gmail.com?part=7