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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] 32-bit arithmetic overflow during baud rate calculation before assignment to a 64-bit variable.
- [High] Calculated baud rate divisor `xyd_x` exceeds its 16-bit register field for high baud rates, causing silent truncation.
- [High] Failure to enforce 32-bit I/O access causes the 32-bit custom baud register write to be silently truncated to 8 bits.
- [High] Incomplete initialization of the `rxtrig_bytes` array breaks sysfs configuration and writes invalid trigger states to the hardware.
- [Medium] Premature integer division in the baud rate calculation amplifies fractional truncation errors.
- [Medium] The custom baud rate calculation hardcodes a 20MHz crystal clock, bypassing the framework's clock frequency synchronization.
--

--- Patch [2]: [PATCH v2 2/2] serial: 8250: Add Airoha SoC UART and HSUART support ---
commit 19616a2ff2ea1d768e60b410919225285f078ea9
Author: Christian Marangi <ansuelsmth@gmail.com>

serial: 8250: Add Airoha SoC UART and HSUART support

Add support for Airoha AN7523 UART and AN7581 HSUART. These implement a
standard 16550 UART with only some 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
> --- /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)
> +{
> +	const struct airoha_8250_clk_div_info *clk_div_info;
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	u32 xindiv_clk;
> +	u64 xyd_x, nom;
> +	int i;
[ ... ]
> +	/*
> +	 * Calculate XYD_x and XINCLKDR register by searching
> +	 * through a table of crystal_clock divisors.
> +	 */
> +	nom = baud * XYD_Y;

[Severity: High]
Since baud and XYD_Y are both 32-bit integers, will this multiplication
overflow for standard baud rates (like 115200) before being assigned to the
64-bit nom variable? Should one of the operands be cast to u64?

> +	for (i = 0 ; i < ARRAY_SIZE(airoha_clk_div_info) ; i++) {
> +		clk_div_info = &airoha_clk_div_info[i];
> +
> +		xindiv_clk = XINDIV_CLOCK / clk_div_info->div;

[Severity: Medium]
This logic relies on a hardcoded 20MHz XINDIV_CLOCK macro. Does this bypass
the framework's clock frequency synchronization, and should it use
port->uartclk instead to respect the clock-frequency Device Tree property?

> +		xyd_x = div_u64(nom, xindiv_clk) * 16;

[Severity: Medium]
Does performing the integer division before multiplying by 16 discard
fractional data too early and amplify truncation errors? Would
div_u64(nom * 16, xindiv_clk) preserve better precision?

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

[Severity: High]
For high baud rates, can xyd_x exceed 65535 when it exits the loop? If so,
does FIELD_PREP() silently mask the oversized value to fit into the 16-bit
UART_AIROHA_XYD_X mask, writing a truncated and incorrect divisor to the
hardware?

> +			FIELD_PREP(UART_AIROHA_XYD_Y, XYD_Y));
> +
> +	/* Restore normal register access. */
> +	serial_port_out(port, UART_LCR, up->lcr);
> +}
> +
> +static int airoha_8250_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	uart.port.dev = dev;
> +	if (device_is_compatible(dev, "airoha,an7581-hsuart"))
> +		uart.port.type = UART_PORT_AIROHA_HS;
> +	else
> +		uart.port.type = UART_PORT_AIROHA;
> +	uart.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
> +			  UPF_FIXED_TYPE | UPF_IOREMAP;

[Severity: High]
Is uart.port.iotype = UPIO_MEM32 missing here?

Without enforcing 32-bit I/O access, if the Device Tree omits 
reg-io-width = <4>, the port defaults to UPIO_MEM. This causes the 32-bit
register writes in airoha_set_divisor() to use 8-bit writeb() accessors,
silently truncating the configuration.

> +	uart.port.set_divisor = airoha_set_divisor;
> +	uart.port.get_divisor = airoha_get_divisor;
[ ... ]
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e94a0802cbddc..13abfb471d185 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},

[Severity: High]
Does leaving the trailing indices of rxtrig_bytes uninitialized (which
implicitly become 0) break sysfs configuration?

When the user requests 4 bytes, bytes_to_fcr_rxtrig() checks
bytes < conf_type->rxtrig_bytes[i]. 4 < 4 is false, and it will fall
through to evaluate 4 < 0 for the uninitialized indices, eventually returning
an undefined hardware state.

Should this array be fully populated with monotonically increasing values,
like {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: High]
The same uninitialized rxtrig_bytes issue appears to be present here as well.

> +		.flags		= UART_CAP_FIFO,
> +	},
>  };

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