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

[Jakub Kicinski <kuba@kernel.org>]

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.