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 [off-list ref]

serial: 8250: add Ambarella UART driver

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

[ ... ]

+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/20260818-cv75-v5-v5-0-7bbb12f0360f@ambarella.com?part=10