Re: [PATCH v5 10/12] serial: 8250: add Ambarella UART driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Data race on up->ier and premature hardware interrupt enablement in ambarella_startup().
- [Medium] Silent drop of accumulated UART error and break status in ambarella_idle_enter().
--

--- Patch [10]: [PATCH v5 10/12] serial: 8250: add Ambarella UART driver ---
commit 766436c7b1daf21bb460e72b46363bc9a4c0f145
Author: Long Zhao <[email protected]>

serial: 8250: add Ambarella UART driver

This patch adds an 8250 platform driver for Ambarella CV75 instead of claiming
snps,dw-apb-uart compatibility.

> diff --git a/drivers/tty/serial/8250/8250_ambarella.c b/drivers/tty/serial/8250/8250_ambarella.c
> new file mode 100644
> index 0000000000000..951355e19fa25
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_ambarella.c
[ ... ]
> +static int ambarella_idle_enter(struct uart_port *p)
> +{
[ ... ]
> +	status = serial_lsr_in(up);
> +	if (status & UART_LSR_DR) {
> +		readl(p->membase + (UART_RX << p->regshift));
> +		up->lsr_saved_flags = 0;

[Severity: Medium]
Will this unconditionally clear any pending parity, framing, overrun errors,
or break conditions accumulated in lsr_saved_flags by the preceding call
to serial_lsr_in()? If so, this could prevent the interrupt handler from
reporting these conditions to the upper layers, resulting in lost break
signals.

> +	}
> +
> +	if (readl(p->membase + (AMBARELLA_UART_USR << p->regshift)) &
> +	    AMBARELLA_UART_USR_BUSY) {
> +		ambarella_idle_exit(p);
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int ambarella_startup(struct uart_port *port)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	int ret;
> +
> +	ret = serial8250_do_startup(port);
> +	if (ret)
> +		return ret;
> +
> +	up->ier |= AMBARELLA_UART_IER_ETOI;
> +	serial_port_out(port, UART_IER, up->ier);

[Severity: High]
Does modifying up->ier and writing it to UART_IER here without holding
port->lock cause a data race with concurrent console writes
(e.g., serial8250_console_write()), which also manipulates interrupts under
the lock? 

Additionally, doesn't writing the IER register here defeat the 8250 core's
intentional deferral of interrupt enablement? The core typically defers
writing to UART_IER until set_termios to avoid swamping the interrupt handler
before the FIFOs are configured.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10