Re: [PATCH v5 3/3] 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] Deadlock (if console) and Log Flooding (if non-console) via unratelimited dev_err() under port lock
- [High] Silent probe failure or mismatched baud rate validation due to uninitialized port->uartclk
- [Medium] Driver squats on global UAPI port type namespace
- [Medium] Incomplete rxtrig_bytes initialization sets undefined hardware state and breaks sysfs reads
--

--- Patch [3]: [PATCH v5 3/3] serial: 8250: Add Airoha SoC UART and HSUART support ---
commit 5a2afaf1ec03e067e37973b1f2ee37f044a204f9
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. It provides
a standard 16550 UART driver with custom logic for baud rate handling.

> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 77fe0588fd6bd..e6b2b94312292 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -73,6 +73,12 @@ struct serial8250_config {
>  	unsigned int	flags;
>  };
>  
> +enum uart_port_type {
> +	/* Internal 8250 only */
> +	UART_PORT_AIROHA		= 124,
> +	UART_PORT_AIROHA_HS		= 125,
> +};

[Severity: Medium]
Can this driver safely use these values without them being allocated in the
global UAPI header?

The port->type values are part of the global userspace UAPI, defined in
include/uapi/linux/serial_core.h, and are exposed to userspace via the
TIOCGSERIAL ioctl. Since the highest officially allocated port type is 123,
assigning 124 and 125 internally rather than in the UAPI header risks a future
collision. 

If a new generic serial driver is legitimately assigned 124 in the UAPI
header, it will silently collide with UART_PORT_AIROHA, causing uart_config
array index collisions and userspace misidentification of the port type.

[ ... ]
> diff --git a/drivers/tty/serial/8250/8250_airoha.c b/drivers/tty/serial/8250/8250_airoha.c
> new file mode 100644
> index 0000000000000..fe72a39561c28
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_airoha.c
[ ... ]
> +static void airoha_set_divisor(struct uart_port *port, unsigned int baud,
> +			       unsigned int quot, unsigned int quot_frac)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	u32 xindiv_clk;
> +	u64 xyd_x, nom;
> +	int div_bit;
[ ... ]
> +	/* Couldn't find a valid xyd_x */
> +	if (div_bit < 0) {
> +		dev_err(port->dev, "failed to find suitable clock divisor for baud %u\n",
> +			baud);
> +		return;
> +	}

[Severity: High]
Is it safe to call dev_err() here?

This function is invoked via serial8250_do_set_termios() with the raw
spinlock port->lock held. If this port is the active serial console,
dev_err() invokes printk(), which synchronously calls
serial8250_console_write(). The console write function will attempt to
acquire the exact same port->lock, resulting in an instant self-deadlock.

If the port is not the console, the error is unratelimited. This allows a
local user with write access to the TTY device to spam the kernel log by
requesting unsupported high baud rates via the TCSETS2 ioctl in a tight loop.

[ ... ]
> +static int airoha_8250_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port uart = { };
> +	struct device *dev = &pdev->dev;
> +	struct airoha_8250_priv *priv;
> +	struct resource *res;
> +	int ret;
[ ... ]
> +	uart.port.mapbase = res->start;
> +	uart.port.mapsize = resource_size(res);
> +
> +	ret = uart_read_and_validate_port_properties(&uart.port);

[Severity: High]
Does this code need to initialize uart.port.uartclk before parsing properties?

The hardcoded divisor logic in airoha_set_divisor() relies on a 20 MHz
crystal (XINDIV_CLOCK = 20 * HZ_PER_MHZ). However, uartclk isn't explicitly
set before or after calling uart_read_and_validate_port_properties().

If the device tree omits the legacy clock-frequency property (e.g., using the
modern clocks property instead), uartclk remains 0. This causes
serial8250_register_8250_port() to reject the port with -EINVAL, resulting in
a silent probe failure.

If the device tree specifies a different frequency (like a 100MHz APB bus
clock), the serial core will bound baud rates based on that incorrect
frequency. This would allow unsupported high baud rates to reach the hardcoded
20MHz divisor logic, repeatedly triggering the deadlock or log-flooding issue
above.

[ ... ]
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e94a0802cbddc..11728a789922d 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -311,6 +311,22 @@ static const struct serial8250_config uart_config[] = {
>  		.rxtrig_bytes	= {1, 8, 16, 30},
>  		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
>  	},
> +	[UART_PORT_AIROHA] = {
> +		.name		= "Airoha UART",
> +		.fifo_size	= 8,
> +		.tx_loadsz	= 1,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 | UART_FCR_CLEAR_RCVR,
> +		.rxtrig_bytes	= {1, 4, 4, 4},
> +		.flags		= UART_CAP_FIFO,
> +	},
> +	[UART_PORT_AIROHA_HS] = {
> +		.name		= "Airoha HSUART",
> +		.fifo_size	= 128,
> +		.tx_loadsz	= 128,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 | UART_FCR_CLEAR_RCVR,
> +		.rxtrig_bytes	= {1, 4},

[Severity: Medium]
Will this partially initialized rxtrig_bytes array break sysfs reads?

The rxtrig_bytes array is 4 elements long, but only two are initialized here,
leaving the remaining elements as implicitly 0. 

When a root user configures a trigger level > 4 via
/sys/class/tty/ttyS*/rx_trig_bytes, bytes_to_fcr_rxtrig() iterates over this
array checking bytes < conf_type->rxtrig_bytes[i]. Because the uninitialized
elements are 0, the condition evaluates to false, and the function returns an
undefined trigger state (UART_FCR_R_TRIG_11).

When sysfs later tries to read the value via fcr_get_rxtrig_bytes(), it will
fetch 0 from the array and return -EOPNOTSUPP. Padding the array with
identical maximum values (e.g., {1, 4, 4, 4}) like the standard Airoha UART
would avoid this.

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