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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] tty: serial: cortina: corrupted RX data passed as TTY_NORMAL on parity/framing errors
- [Low] serial: incorrect ordering of overrun error reporting in TTY buffer
--

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

tty: serial: Add Cortina UART driver

This commit adds a driver for the UART found in Cortina Systems CS75xx
(Goldengate G2) SoCs. It implements the current serial core API and devm
resource management, and supports the system console and earlycon.

> diff --git a/drivers/tty/serial/cortina-uart.c b/drivers/tty/serial/cortina-uart.c
> new file mode 100644
> index 0000000000000..0c2733a7c6b1d
> --- /dev/null
> +++ b/drivers/tty/serial/cortina-uart.c
[ ... ]
> +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;
> +
> +	if (status & CORTINA_UART_IRQ_RX_OVERRUN) {
> +		port->icount.overrun++;
> +		tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> +	}

[Severity: Low]
Could this result in the TTY_OVERRUN marker being inserted before the older,
valid characters are extracted from the FIFO?

When a hardware overrun occurs, the RX FIFO is full of older, valid
characters, while the newly arriving characters that did not fit were lost.
By inserting the TTY_OVERRUN marker before the while loop that drains the
FIFO, it appears the driver places the error marker before the older
characters in the TTY stream.

Would this cause the TTY layer to report the data loss in the wrong
chronological position relative to the valid data? The overrun marker should
likely be inserted after the valid characters in the FIFO have been drained.

> +	if (status & CORTINA_UART_IRQ_RX_PARITY)
> +		port->icount.parity++;
> +	if (status & CORTINA_UART_IRQ_RX_STOP)
> +		port->icount.frame++;
> +
> +	while (budget-- && !(readl(port->membase + CORTINA_UART_INFO) &
> +			     CORTINA_UART_INFO_RX_EMPTY)) {
> +		u8 ch;
> +
> +		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);
> +
> +		if (port->ignore_status_mask & CORTINA_UART_RXDAT_VALID)
> +			continue;
> +		if (uart_prepare_sysrq_char(port, ch))
> +			continue;
> +
> +		tty_insert_flip_char(tport, ch, TTY_NORMAL);
> +	}

[Severity: Medium]
Is it intentional that received characters are unconditionally flagged as
TTY_NORMAL and pushed to the TTY buffer, even if the interrupt indicated a
parity or framing error?

When the hardware detects a parity or framing error, it increments the
corresponding statistics but then unconditionally inserts all received
characters into the TTY buffer with the TTY_NORMAL flag.

[ ... ]
> +static void cortina_uart_set_termios(struct uart_port *port,
> +				     struct ktermios *termios,
> +				     const struct ktermios *old)
> +{
[ ... ]
> +	uart_port_lock_irqsave(port, &flags);
> +
> +	port->ignore_status_mask = 0;
> +	if (!(termios->c_cflag & CREAD))
> +		port->ignore_status_mask |= CORTINA_UART_RXDAT_VALID;
> +
> +	uart_update_timeout(port, termios->c_cflag, baud);

[Severity: Medium]
Should port->read_status_mask or port->ignore_status_mask be configured here
for parity and framing errors?

By not populating these masks based on termios->c_iflag (e.g., INPCK, IGNPAR,
PARMRK), it appears the driver completely bypasses termios error-handling
policies and silently passes corrupted data to userspace as perfectly valid
characters.

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