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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] The IDA line number is freed before the UART port is unregistered, allowing an ID reuse race condition.
- [High] Closing a single UART port unconditionally stops the shared polling loop, breaking all other active ports on the same SPI chip.
- [High] Ignored SPI read errors leave stack variables uninitialized, leading to undefined control flow and leakage of stale buffer data to user-space.
- [High] Ports are registered and exposed to user-space before the IRQ handler and polling infrastructure are initialized.
- [High] Unprotected access to serial core data structures (xmit_fifo, x_char, icount) without holding port->lock causes data races and severe kfifo corruption.
- [High] SPI transfer buffers are allocated on the stack, violating DMA-safety requirements and potentially causing memory corruption or panics.
- [High] Late devm IRQ teardown causes Use-After-Free of port->state and kworker task_struct if an interrupt fires during manual port removal.
- [High] Polling mode deadlocks the kworker thread permanently upon the first TX FIFO drain.
- [High] 64-bit division using the `/` operator will break kernel builds on 32-bit architectures.
- [Medium] The shared IRQ handler unconditionally returns IRQ_HANDLED, breaking shared interrupt semantics.
- [Medium] Standard termios CREAD (Receiver Enable) flag is incorrectly evaluated, failing to drop normal characters.
- [Medium] RX error flags from FSR are incorrectly applied to every character in the burst read.
- [Medium] wk2xxx_throttle() fails to disable the RX timeout interrupt, defeating flow control.
- [Medium] Incorrect implementation of unsupported RS485 delays.
--

commit fa4922eb42a208a55c46770ba645376285db8163
Author: zjzhao <zjzhao@edatec.cn>
Subject: serial: wk2xxx: Add WK2xxx SPI UART driver

This commit adds a new serial driver for the WK2xxx SPI to UART bridge ICs.
It supports up to four full-duplex UART channels over a single SPI slave
interface, with features like hardware flow control and RS485, using a
kthread worker for register accesses.

> diff --git a/drivers/tty/serial/wk2xxx.c b/drivers/tty/serial/wk2xxx.c
> new file mode 100644
> index 0000000000000..184bef9b0fa81
> --- /dev/null
> +++ b/drivers/tty/serial/wk2xxx.c
[ ... ]
> +static int wk2xxx_raw_read(struct wk2xxx_port *s, u8 addr, u8 *val)
> +{
> +	u8 tx[2] = { WK2XXX_SPI_READ | addr, 0 };
> +	u8 rx[2] = { 0, 0 };
> +	int ret;
> +
> +	ret = wk2xxx_spi_transfer(s, tx, rx, sizeof(tx));

[Severity: High]
Is it safe to pass stack-allocated buffers to spi_sync() here and in the
fifo access functions?

Stack memory is typically not DMA-safe and can lack cacheline alignment,
potentially causing memory corruption or panics on cache-incoherent
architectures.

[ ... ]
> +static void wk2xxx_throttle(struct uart_port *port)
> +{
> +	unsigned long flags;
> +
> +	/* Stop draining the RX FIFO to apply back-pressure. */
> +	uart_port_lock_irqsave(port, &flags);
> +	wk2xxx_ier_clear(port, WK2XXX_SIER_RFTRIG_IEN_BIT);
> +	uart_port_unlock_irqrestore(port, flags);
> +}

[Severity: Medium]
Does this sequence fully apply back-pressure?

The RX timeout interrupt (WK2XXX_SIER_RXOUT_IEN_BIT) remains enabled. If any
bytes remain in the hardware FIFO, the timeout interrupt will eventually fire
and push more characters, defeating the flow control request.

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

[Severity: High]
Does this lockless access to tport->xmit_fifo expose the buffer to races?

Because wk2xxx_fifo_write() can sleep, a concurrent flush operation could
acquire port->lock and reset the kfifo. If that happens, the subsequent
lockless uart_xmit_advance() could increment the fifo out index beyond the
in index, leading to underflows and heap corruption.

[ ... ]
> +static void wk2xxx_handle_rx(struct uart_port *port)
> +{
[ ... ]
> +	u8 fsr, rfcnt, lsr = 0, flag = TTY_NORMAL;
> +
> +	wk2xxx_port_reg_read(s, portno, WK2XXX_FSR_REG, &fsr);
[ ... ]
> +	wk2xxx_port_reg_read(s, portno, WK2XXX_RFCNT_REG, &rfcnt);

[Severity: High]
If the SPI transaction fails here, the return value is ignored.

Could this leave rfcnt uninitialized, using its stale value to dictate how
much memory is read? Since wk2xxx_fifo_read() can also fail silently, this
might push unmodified, stale heap data directly to user-space.

> +	/* Map the FIFO status register error flags to line status. */
> +	if (fsr & WK2XXX_FSR_ERR_MASK) {
[ ... ]
> +	}
> +
> +	port->icount.rx += rxlen;
> +
> +	for (i = 0; i < rxlen; ++i) {
> +		u8 ch = one->buf[i];
[ ... ]
> +		uart_insert_char(port, lsr, WK2XXX_LSR_OE_BIT, ch, flag);
> +	}

[Severity: Medium]
Does reading fsr once and applying the exact same lsr error flags to every
character in the loop incorrectly flag valid characters as corrupted?

A single corrupted byte in the hardware FIFO could cause the entire batch
to be marked with framing, parity, or overrun errors.

[ ... ]
> +static bool wk2xxx_port_irq(struct wk2xxx_port *s, unsigned int portno)
> +{
> +	struct uart_port *port = &s->p[portno].port;
> +	u8 sifr, sier;
> +	bool rc = false;
> +
> +	wk2xxx_port_reg_read(s, portno, WK2XXX_SIFR_REG, &sifr);
> +	wk2xxx_port_reg_read(s, portno, WK2XXX_SIER_REG, &sier);

[Severity: High]
If the SPI transaction fails here, the return value is ignored.

Could this leave sifr and sier uninitialized, causing undefined behavior
and spurious interrupt handling based on stack garbage?

[ ... ]
> +static irqreturn_t wk2xxx_irq(int irq, void *dev_id)
> +{
[ ... ]
> +		for (i = 0; i < s->devtype->nr_uart; ++i)
> +			if (gifr & BIT(i))
> +				keep_polling |= wk2xxx_port_irq(s, i);
> +	} while (keep_polling);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
If keep_polling evaluates to false on the first iteration, this function
unconditionally returns IRQ_HANDLED even though no device interrupt was
pending.

Will this mask unhandled interrupts and break semantics for other
devices sharing the IRQ line?

[ ... ]
> +static void wk2xxx_poll_proc(struct kthread_work *ws)
> +{
> +	struct wk2xxx_port *s = container_of(ws, struct wk2xxx_port,
> +					     poll_work.work);
> +
> +	/* Reuse the IRQ handler; the interrupt ID is unused here. */
> +	wk2xxx_irq(0, s);
> +
> +	kthread_queue_delayed_work(&s->kworker, &s->poll_work,
> +				   msecs_to_jiffies(WK2XXX_POLL_PERIOD_MS));
> +}

[Severity: High]
Can this lead to a permanent deadlock when the TX FIFO drains?

The wk2xxx_poll_proc runs on s->kworker and processes the loop until the
device is idle. When TX empties, wk2xxx_handle_tx attempts to disable the
interrupt by queuing reg_work via wk2xxx_ier_clear to the very same
s->kworker. Since the worker is already blocked waiting for the interrupt
to clear in the keep_polling loop, reg_work never runs, creating an
infinite loop.

[ ... ]
> +static int wk2xxx_config_rs485(struct uart_port *port, struct ktermios *termios,
> +			       struct serial_rs485 *rs485)
> +{
[ ... ]
> +		if (rs485->delay_rts_after_send)
> +			return -EINVAL;

[ ... ]
> +static const struct serial_rs485 wk2xxx_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
> +};

[Severity: Medium]
Is it intentional to advertise up to 1ms of delay_rts_after_send support to
the serial core, but then explicitly return -EINVAL in wk2xxx_config_rs485
when that delay is requested?

This mismatch causes valid user-space ioctls to fail unexpectedly.

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

[Severity: High]
Like the previous read functions, SPI failures here are ignored.

Will this leave fsr uninitialized and evaluate arbitrary stack garbage in
the ternary logic?

[ ... ]
> +static void wk2xxx_calc_divisor(unsigned long clk, unsigned int baud,
> +				u8 *baud0, u8 *baud1, u8 *pres)
> +{
[ ... ]
> +	*pres = ((unsigned long long)(clk % (baud * 16)) * 100 / baud + 50) / 100;
> +}

[Severity: High]
Can this division cause link failures on 32-bit architectures?

The code casts clk to a 64-bit unsigned long long and then divides by the
32-bit baud using the native / operator, which breaks 32-bit builds lacking
libgcc.

[ ... ]
> +static void wk2xxx_set_termios(struct uart_port *port, struct ktermios *termios,
> +			       const struct ktermios *old)
> +{
[ ... ]
> +	/* Set status ignore mask. */
> +	port->ignore_status_mask = 0;
> +	if (termios->c_iflag & IGNBRK)
> +		port->ignore_status_mask |= WK2XXX_LSR_BI_BIT;
> +	if (!(termios->c_cflag & CREAD))
> +		port->ignore_status_mask |= WK2XXX_LSR_BRK_ERROR_MASK;

[Severity: Medium]
Does this correctly filter out characters when the receiver is disabled via
CREAD?

By setting only error bits in the ignore mask, standard characters with a
successful line status (lsr == 0) will bypass the filter in
wk2xxx_handle_rx and continue being pushed to the TTY layer.

[ ... ]
> +static void wk2xxx_shutdown(struct uart_port *port)
> +{
[ ... ]
> +	if (s->polling)
> +		kthread_cancel_delayed_work_sync(&s->poll_work);

[Severity: High]
Since s->poll_work is shared across all UART ports on the SPI chip, won't
shutting down a single port permanently cancel the shared polling loop?

This appears to starve all other concurrent open ports on the same device
of TX and RX events.

[ ... ]
> +static int wk2xxx_probe(struct spi_device *spi)
> +{
[ ... ]
> +		/* Register port. */
> +		ret = uart_add_one_port(&wk2xxx_uart, &s->p[i].port);
> +		if (ret)
> +			goto out_ports;
> +
> +		port_registered[i] = true;
> +	}
> +
> +	if (spi->irq <= 0) {
[ ... ]
> +	ret = devm_request_threaded_irq(dev, spi->irq, NULL, wk2xxx_irq,

[Severity: High]
Does exposing the port to user-space before its interrupt handler is
requested create a race condition?

If the port is opened immediately after uart_add_one_port, wk2xxx_startup
will unmask IC hardware interrupts before the handler is actually registered,
potentially causing unhandled IRQ storms that disable the shared line.

[ ... ]
> +out_ports:
> +	for (i = 0; i < devtype->nr_uart; i++) {
> +		if (s->p[i].port.line < WK2XXX_MAX_DEVS)
> +			ida_free(&wk2xxx_lines, s->p[i].port.line);
> +		if (port_registered[i])
> +			uart_remove_one_port(&wk2xxx_uart, &s->p[i].port);
> +	}

[Severity: High]
Does freeing the IDA line before unregistering the port expose a race
condition?

Releasing the ID early could allow a concurrent probe to claim the same
line ID before uart_remove_one_port is fully processed, leading to
subsystem corruption.

[ ... ]
> +static void wk2xxx_remove(struct spi_device *spi)
> +{
> +	struct wk2xxx_port *s = dev_get_drvdata(&spi->dev);
> +	int i;
> +
> +	for (i = 0; i < s->devtype->nr_uart; i++) {
> +		ida_free(&wk2xxx_lines, s->p[i].port.line);
> +		uart_remove_one_port(&wk2xxx_uart, &s->p[i].port);
> +	}

[Severity: High]
Along with the IDA freeing issue mentioned above, is there a use-after-free
risk here if a hardware interrupt fires?

The IRQ was requested via the managed devm_request_threaded_irq API, so it
stays active until after this function returns and devres cleanup runs.
However, uart_remove_one_port frees the port state manually, meaning a late
interrupt could deference freed memory.

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