Re: [PATCH 4/4] serial: 8250: Add Airoha SoC UART and HSUART support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardware registers are accessed without holding the port lock, causing a critical race condition with interrupt handlers.
- [High] Integer overflow and out-of-bounds truncation in custom baud rate calculation.
- [High] Missing NULL pointer check for device_get_match_data() result.
- [Low] Unused static const arrays (dead code).
--

--- Patch [4]: [PATCH 4/4] serial: 8250: Add Airoha SoC UART and HSUART support ---
commit 787398b7c15179f8d67914a09402fbd411a20806
Author: Christian Marangi <ansuelsmth@gmail.com>

serial: 8250: Add Airoha SoC UART and HSUART support

This commit adds support for Airoha AN7523 UART and AN7581 HSUART.
These implement a standard 16550 UART with custom logic for baud rate handling.

> diff --git a/drivers/tty/serial/8250/8250_airoha.c b/drivers/tty/serial/8250/8250_airoha.c
> new file mode 100644
> index 0000000000000..9ec98cd8de438
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_airoha.c
[ ... ]
> +static const struct airoha_8250_clk_div_info airoha_clk_div_info[] = {
> +	{ .div = 10, .mask = BIT(2) },
> +	{ .div = 4, .mask = BIT(1) },
> +	{ .div = 2, .mask = BIT(0) },
> +};
> +
> +static const int clock_div_tab[] = { 10, 4, 2};
> +static const int clock_div_reg[] = {  4, 2, 1};

[Severity: Low]
Are these arrays used anywhere in the driver? It looks like the implementation
relies on airoha_clk_div_info instead, making these dead code.

> +
> +/*
> + * Airoha UART baud rate calculation logic
[ ... ]
> +static void airoha_set_termios(struct uart_port *port, struct ktermios *termios,
> +			       const struct ktermios *old)
> +{
> +	const struct airoha_8250_clk_div_info *clk_div_info;
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	unsigned int xyd_x, nom, denom;
> +	unsigned int baud;
> +	int i;
> +
> +	serial8250_do_set_termios(port, termios, old);
> +
> +	baud = serial8250_get_baud_rate(port, termios, old);
> +
> +	/* Set DLAB to access the baud rate divider registers (BRDH, BRDL) */
> +	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);

[Severity: High]
Does this sequence leave the port registers unprotected?

Looking at serial8250_do_set_termios(), it acquires and then releases the
port lock. Modifying UART_LCR here to set the DLAB bit without holding the
port lock leaves a race window.

If a UART RX or TX interrupt fires concurrently, the interrupt handler
(serial8250_handle_irq) will acquire the port lock and attempt to read UART_RX
or write UART_TX at offset 0. Since DLAB=1, offset 0 becomes the UART_DLL
register, which could lead to silently overwriting the baud rate divisor or
reading corrupted data.

Additionally, does this access registers without an active runtime PM reference?

> +
> +	/* Set baud rate calculation defaults (BRDIV ([BRDH,BRDL]) to 1) */
> +	serial_port_out(port, UART_AIROHA_BRDL, UART_BRDL_20M);
> +	serial_port_out(port, UART_AIROHA_BRDH, UART_BRDH_20M);
> +
> +	/*
> +	 * Calculate XYD_x and XINCLKDR register by searching
> +	 * through a table of crystal_clock divisors.
> +	 */
> +	for (i = 0 ; i < ARRAY_SIZE(airoha_clk_div_info) ; i++) {
> +		clk_div_info = &airoha_clk_div_info[i];
> +
> +		denom = (XINDIV_CLOCK / 40) / clk_div_info->div;
> +		nom = baud * (XYD_Y / 40);
> +		xyd_x = ((nom / denom) << 4);

[Severity: High]
Could the calculation for nom silently overflow on high baud rates?

If baud is very high (e.g., 3,000,000 or 4,000,000 for HSUARTs), multiplying
it by 1625 (XYD_Y / 40) exceeds the maximum capacity of a 32-bit unsigned int,
resulting in an incorrectly calculated baud rate.

> +		/* For the HSUART xyd_x needs to be scaled by a factor of 2 */
> +		if (port->type == UART_PORT_AIROHA_HS)
> +			xyd_x = xyd_x >> 1;
> +		if (xyd_x < XYD_Y)
> +			break;
> +	}
> +
> +	serial_port_out(port, UART_AIROHA_XINCLKDR, clk_div_info->mask);
> +	serial_port_out(port, UART_AIROHA_XYD, (xyd_x << 16) | XYD_Y);

[Severity: High]
If the loop terminates without finding a valid divider and xyd_x remains
large, will the left shift of xyd_x by 16 silently overflow a 32-bit integer
here before the write to UART_AIROHA_XYD?

> +
> +	/* unset DLAB */
> +	serial_port_out(port, UART_LCR, up->lcr);
> +}
> +
> +static int airoha_8250_probe(struct platform_device *pdev)
> +{
> +	const struct airoha_8250_data *data;
> +	struct uart_8250_port uart = { };
> +	struct device *dev = &pdev->dev;
> +	struct airoha_8250_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return dev_err_probe(dev, -EINVAL, "invalid address\n");
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	data = device_get_match_data(dev);
> +
> +	uart.port.dev = dev;
> +	uart.port.type = data->type;

[Severity: High]
Is it possible for device_get_match_data() to return NULL here?

If a platform device is manually bound via sysfs without a Device Tree node,
it would lack OF match data, causing device_get_match_data() to return NULL.
This would lead to a NULL pointer dereference when accessing data->type.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709205656.319531-1-ansuelsmth@gmail.com?part=4