Re: [PATCH net-next v12 4/4] net: pse-pd: realtek-pse-mcu: add UART transport

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The UART receive callback silently drops valid response bytes when multiple frames arrive back-to-back or are coalesced, causing transaction timeouts and framing desynchronization.
--

--- Patch [4]: [PATCH net-next v12 4/4] net: pse-pd: realtek-pse-mcu: add UART transport ---
commit 590478bd64a58c36fc17f754ed1f8e9aed82d8be
Author: Jonas Jelonek <jelonek.jonas@gmail.com>

net: pse-pd: realtek-pse-mcu: add UART transport
    
This commit adds the serdev (UART) transport for the Realtek PSE MCU core.
It registers the MCU as a serdev device and provides the send/recv callbacks
the core uses to exchange 12-byte frames asynchronously.

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-uart.c b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
> new file mode 100644
> index 0000000000000..af8bd8ace00e9
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu-uart.c

[ ... ]

> +static size_t rtpse_mcu_uart_receive(struct serdev_device *serdev,
> +				     const u8 *buf, size_t count)
> +{
> +	struct rtpse_mcu_uart *ctx = serdev_device_get_drvdata(serdev);
> +	size_t take;
> +
> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
> +		take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
> +		if (take) {
> +			memcpy(ctx->rx_buf + ctx->rx_len, buf, take);
> +			ctx->rx_len += take;
> +			if (ctx->rx_len == sizeof(ctx->rx_buf))
> +				complete(&ctx->rx_done);
> +		}
> +	}
> +
> +	/* consume all to avoid desync/misalignment */
> +	return count;
> +}

[Severity: High]
Will this silently drop bytes if the TTY layer delivers coalesced frames?

In rtpse_mcu_uart_receive(), take is capped by the remaining space in rx_buf
(up to 12 bytes). If the MCU responds with multiple frames quickly (for
example, an INCOMPLETE frame followed immediately by a final frame), the TTY
layer might coalesce these and pass a count > 12 to this callback.

Since this function copies at most 12 bytes but unconditionally returns count,
it tells the serdev core that all bytes were consumed. The remaining bytes
would be permanently lost.

Similarly, if a second frame arrives before the sleeping thread in
rtpse_mcu_uart_recv() wakes up to reset ctx->rx_len to 0, take will be 0,
the bytes will be ignored, but count is still returned.

Will this cause rtpse_mcu_uart_recv() to subsequently time out because the
final frame was discarded here?

[ ... ]

> +static int rtpse_mcu_uart_recv(struct rtpse_mcu_ctrl *pse,
> +			       const struct rtpse_mcu_msg *req,
> +			       struct rtpse_mcu_msg *resp)
> +{
> +	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
> +	unsigned long timeout = RTPSE_MCU_UART_RX_TIMEOUT;
> +	bool final;
> +
> +	/* Wait past a non-final frame (transient NOT_READY / stale) within budget. */
> +	do {
> +		timeout = wait_for_completion_timeout(&ctx->rx_done, timeout);
> +		if (!timeout)
> +			return -ETIMEDOUT;
> +
> +		scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
> +			if (ctx->rx_len != sizeof(*resp))
> +				return -EIO;
> +
> +			memcpy(resp, ctx->rx_buf, sizeof(*resp));
> +			final = rtpse_mcu_resp_is_final(req, resp);
> +			if (!final) {
> +				reinit_completion(&ctx->rx_done);
> +				ctx->rx_len = 0;
> +			}
> +		}
> +	} while (!final);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809112251.5797-1-jelonek.jonas@gmail.com?part=4