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

[Jonas Jelonek]

On 06.08.26 03:11, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: pse-pd: realtek-pse-mcu: add UART transport
>
> This adds a serdev (UART) transport for the Realtek PSE MCU core, with
> send/recv callbacks that exchange the 12-byte frames and an asynchronous
> receive_buf callback. The baud rate defaults to 19200 and can be overridden
> with the "current-speed" property.
>
>> 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..9baa17d8d31fa
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
>> @@ -0,0 +1,164 @@
> [ ... ]
>
>> +/*
>> + * No framing is done here: a glitched frame costs one transaction, then
>> + * the next _send re-frames from rx_len 0. Resync works by returning count
>> + * (not take), dropping any overflow so serdev keeps no leftover to bleed
>> + * into the next frame.
>> + */
>> +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;
>> +}
> [ ... ]
>
>> +static int rtpse_mcu_uart_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
>> +{
>> +	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
>> +	int written;
>> +
>> +	/* clear any leftover rx state before transmitting */
>> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
>> +		reinit_completion(&ctx->rx_done);
>> +		ctx->rx_len = 0;
>> +	}
> [ ... ]
>
>> +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);
>> +
>> +	if (!wait_for_completion_timeout(&ctx->rx_done, RTPSE_MCU_UART_RX_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));
>> +	}
>> +	return 0;
>> +}
> [Severity: Medium]
> Should rtpse_mcu_uart_recv() apply rtpse_mcu_resp_is_final() before
> accepting a frame?  The req argument is unused here, so the first complete
> 12-byte frame becomes the transaction result no matter what it contains.
>
> realtek-pse-mcu.h describes one of those frames as transient:
>
>   /*
>    * MCU status opcodes (seen on the Gen1 dialect; Gen2 never emits them).
>    * INCOMPLETE/BAD_CSUM are terminal; NOT_READY is transient.
>    */
>   #define RTPSE_MCU_OPCODE_NOT_READY		0xff	/* -EAGAIN  */
>
> and the I2C transport keeps polling until the acceptance predicate holds,
> so a NOT_READY frame never escapes the transaction:
>
>   drivers/net/pse-pd/realtek-pse-mcu-i2c.c:rtpse_mcu_i2c_smbus_recv() {
> 	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
> 		...
> 		if (ret == RTPSE_MCU_MSG_SIZE && rtpse_mcu_resp_is_final(req, resp))
> 			return 0;
> 	}
> 	return -ETIMEDOUT;
>   }
>
> With the UART transport, a NOT_READY frame is copied into resp and
> rtpse_mcu_do_xfer() turns it into an error for the caller:
>
>   drivers/net/pse-pd/realtek-pse-mcu-core.c:rtpse_mcu_do_xfer() {
> 	case RTPSE_MCU_OPCODE_NOT_READY:
> 		return -EAGAIN;
>   }
>
> Is any runtime caller prepared for that?  The only place -EAGAIN is retried
> is the probe-time boot loop in rtpse_mcu_discover(); the pse_controller_ops
> paths propagate it, so an ethtool PSE get/set would fail.
>
> Note that "realtek,pse-mcu-gen1" is in rtpse_mcu_uart_of_match[], which is
> the dialect documented as emitting these status opcodes.
>
> Can the real reply also be lost in that case?  Once rx_len is 12 the
> receive callback computes take == 0 and drops the following bytes; if they
> instead arrive after the next rtpse_mcu_uart_send() has reset rx_len to 0
> (the core only sleeps RTPSE_MCU_RESPONSE_MS = 25 ms between send and recv),
> they are consumed as the next transaction's response and rejected by:
>
>   if (resp->opcode != req->opcode ||
>       resp->seq_num != req->seq_num ||
>       resp->checksum != rtpse_mcu_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
> 	return -EBADMSG;
>
> failing a second consecutive operation.
>
> Would it make sense to loop on wait_for_completion_timeout() against a
> remaining-time budget until rtpse_mcu_resp_is_final(req, resp) holds?  As
> written only the first frame is waited for, leaving most of the
> RTPSE_MCU_RESPONSE_MAX_MS (1000 ms) receive budget unused.

Fixed in next version - uart_recv() now loops wait_for_completion_timeout()
against the remaining budget until resp_is_final(req, resp) holds, dropping
non-final frames (transient NOT_READY, or a stale frame from a prior
timeout), so it uses the full RESPONSE_MAX_MS and no longer surfaces a
transient frame as the transaction result.

Regards,
Jonas