Re: [PATCH v4 2/2] 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:
- [Critical] Calling dev_err() in airoha_set_divisor() causes a spinlock deadlock if the port is the kernel console.
- [High] Writing to the 32-bit `UART_AIROHA_XYD` register using `serial_port_out()` will silently truncate data to 8 bits if the device tree does not explicitly specify a 32-bit I/O width.
- [High] The custom `rxtrig_bytes` configurations for Airoha UARTs are incompletely populated, breaking the generic 8250 trigger level logic and causing incorrect FIFO trigger values to be programmed.
- [Medium] Defining `UART_PORT_AIROHA` and `UART_PORT_AIROHA_HS` in a private header improperly extends the global userspace-visible `PORT_*` numbering sequence.
--

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

serial: 8250: Add Airoha SoC UART and HSUART support
    
This patch adds support for Airoha AN7523 UART and AN7581 HSUART. It
implements a standard 16550 UART 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]
Is it safe to define these port type numbers in a private header instead of
include/uapi/linux/serial_core.h?

Since userspace can read this value via the TIOCGSERIAL ioctl, could this lead
to a collision if another port type is assigned the same ID in the global UAPI
header?

> 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)
> +{
[ ... ]
> +	/* 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: Critical]
Can this cause a spinlock deadlock if the port is the kernel console?

The caller serial8250_do_set_termios() holds the port->lock spinlock when
invoking the set_divisor callback. If dev_err() is called and this UART is the
active console, printk will synchronously try to acquire the same port->lock in
serial8250_console_write(), leading to a deadlock.

> +
> +	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: High]
Is there a risk that writing to the 32-bit UART_AIROHA_XYD register using
serial_port_out() will silently truncate the data to 8 bits?

If the device tree is missing the reg-io-width property, the 8250 core will
default to 8-bit I/O. Since the hardware requires 32-bit writes here, should
the driver explicitly set uart.port.iotype to UPIO_MEM32 in the probe function
to ensure the generic accessor doesn't invoke writeb() and discard the upper
24 bits?

> 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
[ ... ]
> +	[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,
> +	},

[Severity: High]
Does the incomplete initialization of rxtrig_bytes for UART_PORT_AIROHA_HS
break the generic trigger level logic?

The array defines only the first two elements, leaving indices 2 and 3 as 0.
When bytes_to_fcr_rxtrig() iterates over all states:

        for (i = 1; i < UART_FCR_R_TRIG_MAX_STATE; i++) {
                if (bytes < conf_type->rxtrig_bytes[i])

Since bytes is unsigned, bytes < 0 is false, so it will fall through and return
UART_FCR_R_TRIG_11 for any requested trigger size of 4 or more, which would
program 0 bytes into the hardware's FCR. Should the rest of the elements be
populated?

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