Re: [PATCH net-next v11 2/4] net: pse-pd: add Realtek PSE MCU core

[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: add Realtek PSE MCU core

This adds the shared core for the Realtek PSE MCU driver: the 12-byte
request/response protocol with sum-mod-256 checksum, the per-dialect opcode
tables and response parser hooks for the two protocol generations, and the
pse_controller_ops glue plus a registration helper for the transport modules.

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-core.c b/drivers/net/pse-pd/realtek-pse-mcu-core.c
> new file mode 100644
> index 0000000000000..ed3b743470dbf
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu-core.c
> @@ -0,0 +1,988 @@

[ ... ]

> +	/*
> +	 * Explicit MCU error opcodes (observed on the Gen1 dialect; harmless
> +	 * to check for Gen2 too). Catch these before the generic opcode/CRC
> +	 * mismatch path so callers see a meaningful errno.
> +	 */
> +	switch (resp->opcode) {
> +	case RTPSE_MCU_OPCODE_INCOMPLETE:
> +		return -EBADE;
> +	case RTPSE_MCU_OPCODE_BAD_CSUM:
> +		return -EBADMSG;
> +	case RTPSE_MCU_OPCODE_NOT_READY:
> +		return -EAGAIN;
> +	}
> +
> +	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;
> +
> +	return 0;
> +}

[Severity: Low]
Does this validation in rtpse_mcu_do_xfer() accept an all-zero frame as a
valid reply?

For a request with opcode 0x00 and seq_num 0x00, an all-zero 12-byte
response satisfies every term:

  resp->opcode  == 0 == req->opcode
  resp->seq_num == 0 == req->seq_num
  rtpse_mcu_checksum() over eleven 0x00 bytes == 0 == resp->checksum

Both dialects use 0x00 for a real command:

  rtpse_mcu_dialect_gen1: [RTPSE_MCU_CMD_PORT_ENABLE]      = RTPSE_MCU_OP(0x00)
  rtpse_mcu_dialect_gen2: [RTPSE_MCU_CMD_SET_GLOBAL_STATE] = RTPSE_MCU_OP(0x00)

and seq_num wraps back to 0x00 every 256 transactions since pse->seq is a u8
incremented unconditionally in rtpse_mcu_do_xfer():

  req->seq_num = pse->seq++;

Would it be worth adding a discriminator here, for instance rejecting a frame
whose bytes are all zero, or reserving seq_num 0?

> +static int rtpse_mcu_port_cmd(struct rtpse_mcu_ctrl *pse, unsigned int port, u8 opcode, u8 arg)
> +{
> +	struct rtpse_mcu_msg req, resp;
> +	int ret;
> +
> +	rtpse_mcu_msg_init(&req, opcode);
> +	req.payload[0] = port;
> +	req.payload[1] = arg;
> +
> +	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
> +	if (ret)
> +		return ret;
> +
> +	if (resp.payload[0] != port || resp.payload[1] != 0)
> +		return -EIO;
> +
> +	return 0;
> +}

[Severity: Low]
Related to the above: does this echo check catch the all-zero frame for port
0? An all-zero payload gives payload[0] == 0 == port and payload[1] == 0, so
a Gen1 port-0 enable/disable would be reported as successful even though the
frame carried no MCU data. rtpse_mcu_set_global_state() has the same shape,
it only requires resp.payload[0] == 0x0.

The same hole also shortens the poll in the I2C transport, since
rtpse_mcu_resp_is_final() declares an all-zero read final for opcode 0x00 /
seq_num 0x00 and the loop stops instead of waiting for the genuine reply.

[ ... ]

> +	/*
> +	 * Release the MCU from reset before the first transaction; the
> +	 * boot-retry loop in discover() waits for it to answer.
> +	 */
> +	gpiod = devm_gpiod_get_optional(pse->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod))
> +		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
> +				     "failed to get reset gpio\n");
> +
> +	ret = rtpse_mcu_discover(pse, &info);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Some boards gate all ports through a hardware line; deassert it only
> +	 * once the MCU is confirmed present, so a failed probe leaves the ports
> +	 * gated. From here the MCU protocol governs per-port admin state.
> +	 */
> +	gpiod = devm_gpiod_get_optional(pse->dev, "disable-ports", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod))
> +		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
> +				     "failed to get disable-ports gpio\n");

[Severity: Medium]
Does the comment's promise hold for failures after this point?

rtpse_mcu_register() drives disable-ports low here, but nothing re-asserts it
on the error paths that follow. The devres unwind only releases the
descriptor; gpiod_free_commit() in drivers/gpio/gpiolib.c clears descriptor
flags, drops the label and optionally calls gc->free(), it does not change the
driven output level:

  if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
          if (guard.gc->free)
                  guard.gc->free(guard.gc, gpiod_hwgpio(desc));
          clear_bit(GPIOD_FLAG_ACTIVE_LOW, &flags);

So if devm_add_action_or_reset() or devm_pse_controller_register() below fails
(the latter propagates -EINVAL/-ENODEV from of_load_pse_pis() for a
missing or mismatched pse-pis node, and -ENOMEM from the kfifo/devres
allocations), the board is left with all ports ungated and no driver bound.
The same applies on unbind or module unload.

Since the level also persists past the descriptor's lifetime, doesn't a
subsequent probe attempt start with the gate already open? That is exactly the
case the comment guards against: if rtpse_mcu_discover() then fails on a
re-probe, the ports are ungated while the MCU is unresponsive and unmanaged.

Would a devm action that re-asserts the line (and the reset line) fit here?

> +	if (!info.system_enable) {
> +		ret = rtpse_mcu_set_global_state(pse, true);
> +		/* Dialects without a global-state concept (e.g. Gen1) return
> +		 * -EOPNOTSUPP; treat that as "no separate enable required".
> +		 */
> +		if (ret && ret != -EOPNOTSUPP)
> +			return dev_err_probe(pse->dev, ret,
> +					     "failed to enable PSE system\n");
> +		if (!ret) {
> +			ret = devm_add_action_or_reset(pse->dev,
> +						       rtpse_mcu_global_disable, pse);
> +			if (ret)
> +				return ret;
> +		}
> +	}

[Severity: Medium]
Is there any compensating teardown for the Gen1 case? rtpse_mcu_dialect_gen1
has no RTPSE_MCU_CMD_SET_GLOBAL_STATE entry, so
rtpse_mcu_set_global_state() returns -EOPNOTSUPP and no devm action is
registered at all. The same happens on boards that already report
info.system_enable, where the whole block is skipped. For those boards
nothing disables power or re-gates the ports on a later probe failure or on
unbind.

[ ... ]

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu.h b/drivers/net/pse-pd/realtek-pse-mcu.h
> new file mode 100644
> index 0000000000000..52f9442d7117c
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu.h
> @@ -0,0 +1,93 @@

[ ... ]

> +/*
> + * A polling transport can stop here: the reply to this request (opcode and
> + * seq_num), or a terminal error. The seq_num rejects a stale reply left by an
> + * abandoned transaction, instead of desyncing by one frame.
> + */
> +static inline bool rtpse_mcu_resp_is_final(const struct rtpse_mcu_msg *req,
> +					   const struct rtpse_mcu_msg *resp)
> +{
> +	return (resp->opcode == req->opcode && resp->seq_num == req->seq_num) ||
> +	       resp->opcode == RTPSE_MCU_OPCODE_INCOMPLETE ||
> +	       resp->opcode == RTPSE_MCU_OPCODE_BAD_CSUM;
> +}

[Severity: Low]
This is the transport-side half of the same question: for a request with
opcode 0x00 and seq_num 0x00, an all-zero read is declared final here, so
rtpse_mcu_i2c_smbus_recv() / rtpse_mcu_i2c_native_recv() stop polling rather
than retrying for the real frame. Should this helper also reject a frame that
is entirely zero?