Re: [PATCH v4 2/2] serial: wk2xxx: Add WK2xxx SPI UART driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] UART transmit FIFO data corruption and stats race due to missing port lock in wk2xxx_handle_tx().
- [High] TTY buffer flood due to persistent overrun flag in batch RX loop.
- [High] The tx_empty callback performs a synchronous SPI read which sleeps while the serial core holds the port spinlock.
- [High] Early return on SPI error in wk2xxx_shutdown skips essential software state cleanup, causing open_ports to permanently leak.
- [Medium] OF node reference leak when parsing DT nodes for non-RS485 variants.
- [Medium] Commit message claims to apply serial properties from DT subnodes, but they are not applied.
- [Low] The driver intentionally breaks the build on architectures with L1_CACHE_BYTES < 31 (e.g., m68k).
--

commit 8ec8660308853d2cff61b90024208e8b3e1bafdb
Author: Zi Jie Zhao <zjzhao@edatec.cn>
Subject: serial: wk2xxx: Add WK2xxx SPI UART driver

This commit introduces a new driver for the WK2xxx SPI to UART bridge ICs.
It models the driver after the NXP sc16is7xx driver, utilizing a kthread
worker for SPI register accesses and supporting polled or interrupt-driven
operation.

> diff --git a/drivers/tty/serial/wk2xxx.c b/drivers/tty/serial/wk2xxx.c
> index 0000000000000..15869fbdeab07
> --- /dev/null
> +++ b/drivers/tty/serial/wk2xxx.c
[ ... ]
> +	/*
> +	 * Shared SPI transfer buffers. All SPI accesses are serialized by
> +	 * s->reg_lock, so these are never used concurrently. Each buffer is
> +	 * cache-line aligned and sized to a full cache line, so DMA cache
> +	 * maintenance on one of them can never invalidate a line shared
> +	 * with any other field (notably the p[] array below).
> +	 */
> +	u8				spi_tx[L1_CACHE_BYTES] ____cacheline_aligned;
> +	u8				spi_rx[L1_CACHE_BYTES] ____cacheline_aligned;
> +	struct wk2xxx_one		p[];
> +};
> +
> +static_assert(WK2XXX_MAX_SPI_LEN + 1 <= L1_CACHE_BYTES);

[Severity: Low]
Does this intentionally break the build for architectures with a cache line 
size smaller than 31 bytes (such as m68k where L1_CACHE_BYTES is 16)? 

Might it be better to size the array to something like 
max(L1_CACHE_BYTES, 32) instead of failing the compilation?

[ ... ]
> +static void wk2xxx_handle_tx(struct uart_port *port)
> +{
[ ... ]
> +	to_send = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail, txlen);
> +	sent = 0;
> +	while (to_send) {
> +		unsigned int chunk = min_t(unsigned int, to_send,
> +					   WK2XXX_MAX_SPI_LEN);
> +
> +		if (wk2xxx_fifo_write(s, portno, tail, chunk))
> +			break;
> +		tail += chunk;
> +		to_send -= chunk;
> +		sent += chunk;
> +	}
> +	uart_xmit_advance(port, sent);

[Severity: High]
Is it safe to manipulate the xmit_fifo pointers and call uart_xmit_advance() 
here without holding the port lock?

If a concurrent operation like a tcflush() ioctl resets the FIFO via 
uart_flush_buffer() (which zeroes the in and out indices), could this 
lockless uart_xmit_advance() call advance the out index beyond the in index, 
causing a substantial underflow and subsequent data corruption?

[ ... ]
> +static void wk2xxx_handle_rx(struct uart_port *port)
> +{
[ ... ]
> +	/* Map the FIFO status register error flags to line status. */
> +	if (fsr & WK2XXX_FSR_ERR_MASK) {
> +		if (fsr & WK2XXX_FSR_RFPE_BIT) {
> +			port->icount.parity++;
> +			lsr |= WK2XXX_LSR_PE_BIT;
> +			flag = TTY_PARITY;
> +		}
> +		if (fsr & WK2XXX_FSR_RFFE_BIT) {
> +			port->icount.frame++;
> +			lsr |= WK2XXX_LSR_FE_BIT;
> +			flag = TTY_FRAME;
> +		}
> +		if (fsr & WK2XXX_FSR_RFOE_BIT) {
> +			port->icount.overrun++;
> +			lsr |= WK2XXX_LSR_OE_BIT;
> +			flag = TTY_OVERRUN;
> +		}
[ ... ]
> +	for (i = 0; i < rxlen; ++i) {
> +		u8 ch = one->buf[i];
> +
> +		if (uart_handle_sysrq_char(port, ch))
> +			continue;
> +
> +		if (lsr & port->ignore_status_mask)
> +			continue;
> +
> +		uart_insert_char(port, lsr, WK2XXX_LSR_OE_BIT, ch, flag);
> +	}

[Severity: High]
Could this loop insert an excessive number of dummy overrun characters into 
the TTY buffer?

Since lsr is modified once outside the loop when the WK2XXX_FSR_RFOE_BIT is 
set, the WK2XXX_LSR_OE_BIT remains active for the entire iteration. When 
uart_insert_char() evaluates this bit, it will insert a dummy overrun byte 
for every single character in this chunk (up to 256 times) for a single 
overrun event.

[ ... ]
> +static unsigned int wk2xxx_tx_empty(struct uart_port *port)
> +{
> +	struct wk2xxx_port *s = dev_get_drvdata(port->dev);
> +	u8 fsr = 0;
> +
> +	if (wk2xxx_port_reg_read(s, port->iobase, WK2XXX_FSR_REG, &fsr))
> +		return TIOCSER_TEMT;

[Severity: High]
Can this sequence sleep while executing in an atomic context?

The serial core routinely invokes the ops->tx_empty() callback while holding 
the port spinlock with interrupts disabled (for instance, during the 
TIOCSERGETLSR ioctl). However, wk2xxx_port_reg_read() performs a synchronous 
spi_sync() transfer which can sleep.

[ ... ]
> +static void wk2xxx_shutdown(struct uart_port *port)
> +{
> +	struct wk2xxx_port *s = dev_get_drvdata(port->dev);
> +	unsigned int portno = port->iobase;
> +	u8 reg;
> +
> +	scoped_guard(mutex, &s->reg_lock) {
> +		/* Disable the sub-UART interrupt in the global mask. */
> +		if (wk2xxx_raw_read(s, WK2XXX_GIER_REG, &reg))
> +			return;

[Severity: High]
If this SPI read fails, the function returns early. Does this leave the 
software state permanently out of sync?

By returning here, the function skips the atomic_dec_return(&s->open_ports) 
and kthread_flush_worker() cleanup steps at the end of wk2xxx_shutdown(). 
This leaves the open_ports counter permanently elevated and strands the 
polling kthread so it can never be stopped.

[ ... ]
> +static int wk2xxx_probe(struct spi_device *spi)
> +{
[ ... ]
> +		/* Locate the matching "serial@i" DT subnode, if any. */
> +		for_each_available_child_of_node(dev->of_node, child) {
> +			u32 reg;
> +
> +			if (!of_node_name_eq(child, "serial"))
> +				continue;
> +			if (of_property_read_u32(child, "reg", &reg))
> +				continue;
> +			if (reg == i) {
> +				port_np = child;
> +				break;
> +			}
> +		}
[ ... ]
> +		/*
> +		 * Temporarily retarget dev's fwnode to the per-port subnode
> +		 * so uart_get_rs485_mode() picks up the per-port properties.
> +		 */
> +		if (port_np && devtype->has_rs485) {
> +			device_set_node(dev, of_fwnode_handle(port_np));
> +			ret = uart_get_rs485_mode(&s->p[i].port);
> +			device_set_node(dev, saved_fwnode);
> +			of_node_put(port_np);
> +			if (ret)
> +				goto out_ports;
> +		}

[Severity: Medium]
Is there a device tree node reference leak here for non-RS485 variants?

The port_np reference is acquired via the for_each_available_child_of_node() 
loop earlier. However, if devtype->has_rs485 evaluates to false (such as for 
the WK2132 or WK2202 variants), this block is entirely bypassed and 
of_node_put(port_np) is never called.

[Severity: Medium]
The commit message states the driver will "apply serial and RS-485 properties 
from each serial@N DT subnode to the corresponding channel."

While uart_get_rs485_mode() is called in this block, should there also be a 
call to uart_read_port_properties() to ensure the generic serial properties 
(like tx-trigger or rx-trigger) are actually applied as described?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908103129.58085-1-zjzhao@edatec.cn?part=2