Re: [PATCH v6 11/13] serial: 8250_dw: add Ambarella CV75 quirks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Eagerly writing to UART_IER in the startup callback prematurely enables RX interrupts and races with the console lock.

Pre-existing issues:
- [High] Calling pm_runtime_enable() after port registration creates a circular wait during device probe.
--

commit 75b385eb3bd4a7c612101e72e64ed9ec2244f1eb
Author: Long Zhao <longzhao@ambarella.com>

serial: 8250_dw: add Ambarella CV75 quirks

This patch adds quirk support for the Ambarella CV75 UART to the 8250_dw
driver, reusing existing APB UART functionality while skipping the baud rate
setup and keeping specific IER bits enabled.

> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c

[ ... ]

> @@ -572,6 +576,27 @@ static void dw8250_prepare_rx_dma(struct uart_8250_port *p)
>  	dw8250_writel_ext(up, RZN1_UART_RDMACR, val);
>  }
>  
> +static int dw8250_ier_mask_startup(struct uart_port *p)
> +{
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +	int ret;
> +
> +	ret = serial8250_do_startup(p);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Ambarella keeps variant IER bits (e.g. ETOI) set for correct RX
> +	 * timeout behaviour. Force them into the 8250 IER shadow so later
> +	 * generic IER updates do not clear them.
> +	 */
> +	up->ier |= d->pdata->ier_mask;
> +	serial_port_out(p, UART_IER, up->ier);

[Severity: High]
Does this eager write to UART_IER race with the console lock or risk causing
an interrupt storm?

When serial8250_do_startup() executes, it explicitly defers hardware interrupt
enablement by only updating the up->ier shadow. By immediately calling
serial_port_out() here without holding the port lock, this could prematurely
enable RX interrupts before FIFOs are fully configured, potentially swamping
the interrupt handler. It also risks data races with concurrent console writes
that read, clear, and restore UART_IER under the port lock.

Would it be safer to only update the shadow up->ier here, and allow
set_termios() to commit the final value to hardware later under the proper
lock?

> +
> +	return 0;
> +}

[ ... ]

> @@ -889,6 +922,7 @@ static const struct dw8250_platform_data dw8250_ultrarisc_dp1000_data = {
>  };
>  
>  static const struct of_device_id dw8250_of_match[] = {
> +	{ .compatible = "ambarella,cv75-uart", .data = &dw8250_ambarella_cv75_data },

[Severity: High]
This is a pre-existing issue, but does adding this new device expose it to a
deadlock during device probe?

Looking at dw8250_probe(), pm_runtime_enable() is called after the port
is registered:

dw8250_probe() {
    ...
    data->data.line = serial8250_register_8250_port(up);
    ...
    pm_runtime_enable(dev);
}

During port registration, uart_add_one_port() can synchronously invoke the pm
callback (dw8250_do_pm). Since dw8250_do_pm() uses pm_runtime_get_sync(),
calling it before runtime PM is enabled will hang the worker threads waiting
for operations on an uninitialized device.

Could pm_runtime_enable() be moved before serial8250_register_8250_port() in
dw8250_probe() to prevent this circular wait?

>  	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
>  	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-cv75-v5-v6-0-e918514cb3b1@ambarella.com?part=11