Re: [PATCH v3 2/2] serial: 8250_dw: add Ambarella CV75 quirks

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

New issues:
- [High] Premature and lockless write to UART_IER in dw8250_ier_mask_startup defeats interrupt deferral and introduces race conditions.

Pre-existing issues:
- [High] Runtime PM is enabled after port registration, violating subsystem PM initialization rules and causing circular wait conditions.
--

--- Patch [2]: [PATCH v3 2/2] serial: 8250_dw: add Ambarella CV75 quirks ---
commit 6e65f0f86d7deb4c1cfc42c64f909eb5435425fe
Author: Long Zhao <[email protected]>

serial: 8250_dw: add Ambarella CV75 quirks

Add CV75 platform data with an ETOI IER mask and skip-set-rate so the
DesignWare 8250 driver can drive the Ambarella UART for console use.

> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 5fba913f33010..e8aa592924d4d 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
[ ... ]
> @@ -572,6 +576,23 @@ 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;
> +
> +	/* Keep variant-specific bits in the shadow across subsequent updates. */
> +	up->ier |= d->pdata->ier_mask;
> +	serial_port_out(p, UART_IER, up->ier);

[Severity: High]
Does this lockless write to UART_IER introduce a race condition against the
serial console? 

The 8250 core explicitly defers enabling hardware RX interrupts until
set_termios() configures the FIFOs to prevent swamping the interrupt handler.
Writing to UART_IER here defeats that safety mechanism.

Furthermore, if a printk occurs concurrently, serial8250_console_write()
executes on another CPU and performs a read-modify-restore on UART_IER. This
lockless write races with the console driver, which can permanently drop the
ier_mask bits or cause unpredictable hardware behavior.

Should this write be protected by p->lock, and potentially deferred until
the FIFOs are configured?

> +
> +	return 0;
> +}
> +

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing
issue in dw8250_probe() regarding runtime PM initialization:

drivers/tty/serial/8250/8250_dw.c:dw8250_probe() {
    ...
	data->data.line = serial8250_register_8250_port(up);
	if (data->data.line < 0)
		return data->data.line;

	platform_set_drvdata(pdev, data);

	pm_runtime_enable(dev);
    ...
}

Does registering the port before enabling runtime PM cause circular wait
conditions during device probe?

Registration synchronously invokes the driver's pm callback (dw8250_do_pm)
via uart_add_one_port(). The dw8250_do_pm() callback unconditionally calls
pm_runtime_get_sync(port->dev). 

As documented in the TTY subsystem guidelines, invoking runtime PM APIs from
callbacks before pm_runtime_enable() is called leads to circular dependencies,
hanging the probe function indefinitely because the runtime PM infrastructure
is not yet initialized for the device.

Should pm_runtime_enable(dev) be called before serial8250_register_8250_port()?

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