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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unhandled high baud rates leave UART hardware in an inconsistent, desynchronized state due to mismatched clock limits.
- [Medium] Silent truncation of 32-bit custom register writes if `reg-io-width` is omitted from the device tree.
- [Medium] Incorrect initialization of `rxtrig_bytes` array causes FCR misconfiguration and breaks sysfs reads.
--

--- Patch [2]: [PATCH v3 2/2] serial: 8250: Add Airoha SoC UART and HSUART support ---
commit af8c5302b76a32f7b1c0a8787d38cbf61806b1bc
Author: Christian Marangi <[email protected]>

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
> index 0000000000000..5cc9b8f8a6181
> --- /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)
> +{

[ ... ]

> +	/* 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]
If userspace requests a high baud rate that cannot find a valid xyd_x, the
loop completes with div_bit < 0 and returns early. Could this abandon the
divisor setup and leave the hardware completely desynchronized with DLAB set
in the LCR register, since the LCR restore step at the end of the function is
skipped?

> +
> +	serial_port_out(port, UART_AIROHA_XINCLKDR, BIT(div_bit));
> +	serial_port_out(port, UART_AIROHA_XYD,
> +			FIELD_PREP(UART_AIROHA_XYD_X, xyd_x) |
> +			FIELD_PREP(UART_AIROHA_XYD_Y, XYD_Y));

[Severity: Medium]
Since this constructs a 32-bit payload, could the upper 24 bits be silently
truncated by an 8-bit writeb() accessor here?

If the optional reg-io-width property is omitted from the device tree,
uart_read_and_validate_port_properties() called during airoha_8250_probe()
defaults uart.port.iotype to UPIO_MEM (8-bit) because it wasn't explicitly set
to UPIO_MEM32 beforehand.

> +
> +	/* Restore normal register access. */
> +	serial_port_out(port, UART_LCR, up->lcr);
> +}

[ ... ]

> 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},

[Severity: Medium]
The rxtrig_bytes arrays here don't appear to be strictly monotonically
increasing.

If userspace writes '4' to the rx_trig_bytes sysfs file, is it possible this
fails the bytes_to_fcr_rxtrig() loop checks (since 4 < 4 and 4 < 0 are false),
returning index 3 and misconfiguring the FCR trigger bits?

> +		.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},
> +		.flags		= UART_CAP_FIFO,
> +	},
>  };

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