Re: [PATCH net-next v2 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver

[Simon Horman <horms@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.
Full review at: https://sashiko.dev
---
net: pse-pd: add Realtek/Broadcom PSE MCU driver

This patch adds a driver for a microcontroller that fronts Realtek and
Broadcom PSE silicon on PoE switches, exposing the MCU over I2C/SMBus or
UART using a 12-byte fixed-frame protocol. The driver is split into a
shared core plus two transport modules, with a per-dialect opcode table
and parser hooks for the Realtek vs Broadcom variants.

> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
> index 7ef29657ee5d..b065b19db126 100644
> --- a/drivers/net/pse-pd/Kconfig
> +++ b/drivers/net/pse-pd/Kconfig
> @@ -13,6 +13,34 @@ menuconfig PSE_CONTROLLER
>  
>  if PSE_CONTROLLER
>  
> +config PSE_REALTEK
> +	tristate
> +	help
> +	  Shared core for the Realtek/Broadcom PSE MCU driver. This is
> +	  selected automatically by the transport options below.
> +
> +config PSE_REALTEK_I2C
> +	tristate "Realtek/Broadcom PSE MCU driver (I2C transport)"
> +	depends on I2C
> +	select PSE_REALTEK
> +	help
> +	  Driver for the microcontroller (MCU) that fronts the PSE
> +	  hardware on switches with Realtek or Broadcom PSE chips, attached
> +	  via I2C/SMBus. The MCU exposes a message-based protocol; the actual
> +	  PSE silicon is not accessed directly. To compile this driver as a
> +	  module, choose M here: the module will be called realtek-pse-i2c.
> +
> +config PSE_REALTEK_UART
> +	tristate "Realtek/Broadcom PSE MCU driver (UART transport)"
> +	depends on SERIAL_DEV_BUS
> +	select PSE_REALTEK
> +	help
> +	  Driver for the microcontroller (MCU) that fronts the PSE
> +	  hardware on switches with Realtek or Broadcom PSE chips, attached
> +	  via UART. The MCU exposes a message-based protocol; the actual PSE
> +	  silicon is not accessed directly. To compile this driver as a
> +	  module, choose M here: the module will be called realtek-pse-uart.
> +

[Medium]
The realtek, vendor prefix is used for both the Realtek dialect and the
Broadcom dialect compatibles (realtek,pse-mcu-rtk and
realtek,pse-mcu-bcm), and the binding documentation explicitly notes
that the prefix "reflects the platform" rather than the silicon vendor.
DT compatibles are stable ABI once accepted.

Would a brcm, prefix (or per-MCU-part compatibles) for the Broadcom
dialect be more appropriate, given that the BCM variant uses Broadcom
PSE silicon driven by an MCU built by Nuvoton or STMicro and has no
Realtek involvement?

Similarly, would describing the I2C wire framing (native vs SMBus) via
separate compatibles be preferable to encoding it in the
realtek,i2c-protocol DT property, and would describing the actual PSE
chip in DT (rather than relying on runtime device-id detection) match
the usual binding patterns?

>  config PSE_REGULATOR
>  	tristate "Regulator based PSE controller"
>  	help

[ ... ]

> diff --git a/drivers/net/pse-pd/realtek-pse-core.c b/drivers/net/pse-pd/realtek-pse-core.c
> new file mode 100644
> index 000000000000..3a601cfb6280
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-core.c

[ ... ]

> +static int rtpse_port_cmd(struct rtpse_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_do_xfer(pse, &req, &resp);
> +	if (ret)
> +		return ret;
> +
> +	if (resp.payload[0] != port || resp.payload[1] != 0)
> +		return -EIO;
> +
> +	return 0;
> +}

[Medium]
Can an all-zero 12-byte buffer be silently accepted as a successful
PORT_ENABLE/PORT_DISABLE on the BCM dialect for port 0?

For the BCM dialect, RTPSE_CMD_PORT_ENABLE maps to opcode 0x00, and the
checksum of eleven zero bytes is 0, so an all-zero response passes the
opcode and checksum checks in rtpse_do_xfer():

	if (resp->opcode != req->opcode ||
	    resp->checksum != rtpse_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
		return -EBADMSG;

rtpse_port_cmd() then verifies payload[0] == port and payload[1] == 0,
both of which pass when port == 0 and arg is 0 (disable) or when arg is
1 but the MCU never observed the command and the buffer is all zero
anyway since payload[1] is checked against 0.

The seq_num field exists in struct rtpse_mcu_msg and is filled with 0xff
by rtpse_mcu_msg_init() but is never checked on the response side. Could
a seq_num round-trip check (or a non-zero opcode sentinel for the BCM
dialect) reject stale or zeroed-out frames here?

[ ... ]

> +static int rtpse_discover(struct rtpse_ctrl *pse, struct rtpse_mcu_info *info)
> +{
> +	struct rtpse_mcu_ext_config ext_config;
> +	unsigned long deadline;
> +	int ret;
> +
> +	/*
> +	 * The MCU may not answer on the bus yet right after power-up or
> +	 * enable-gpios assertion: depending on the transport it either stays
> +	 * silent (-ETIMEDOUT) or does not ACK its address at all (-ENXIO /
> +	 * -EREMOTEIO). Retry within a bounded wall-time window so a slow boot
> +	 * still probes, while a genuinely unresponsive MCU fails with its real
> +	 * error instead of deferring forever and masking it.
> +	 */
> +	deadline = jiffies + msecs_to_jiffies(RTPSE_MCU_BOOT_TIMEOUT_MS);
> +	do {
> +		ret = rtpse_mcu_get_info(pse, info);
> +		if (ret != -ETIMEDOUT && ret != -ENXIO &&
> +		    ret != -EREMOTEIO && ret != -EAGAIN)
> +			break;
> +		msleep(RTPSE_MCU_BOOT_RETRY_MS);
> +	} while (time_before(jiffies, deadline));

[Low]
The comment says the loop is meant to tolerate a slow-boot MCU, but the
retry whitelist excludes -EBADMSG and -EIO.

rtpse_do_xfer() returns -EBADMSG when the opcode or checksum mismatch
fires, and rtpse_port_query()/rtpse_port_cmd() return -EIO when the
echoed port byte does not match the request. On a UART-attached MCU
that emits a single garbled frame while its serial peripheral
stabilises, or on an I2C-attached MCU that returns stale boot-ROM bytes
right after enable-gpios assertion, the first attempt would surface as
one of those errors rather than -ETIMEDOUT or -ENXIO, and the loop
would break out immediately.

Should -EBADMSG (and arguably -EIO) be added to the retry set so the
documented intent matches the actual behaviour?

[ ... ]

> +int rtpse_register(struct rtpse_ctrl *pse)
> +{

[ ... ]

> +	if (!info.system_enable) {
> +		ret = rtpse_set_global_state(pse, true);
> +		/* Dialects without a global-state concept (e.g. BCM) 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");
> +	}
> +
> +	ret = regulator_enable(pse->poe_supply);
> +	if (ret)
> +		return dev_err_probe(pse->dev, ret, "failed to enable PoE supply\n");
> +
> +	ret = devm_add_action_or_reset(pse->dev, rtpse_regulator_disable, pse->poe_supply);
> +	if (ret)
> +		return ret;

[Low]
If rtpse_set_global_state(pse, true) succeeds but a later step in
rtpse_register() fails (regulator_enable, devm_add_action_or_reset, or
devm_pse_controller_register), is the MCU left with the PSE subsystem
enabled while the kernel detaches?

The regulator is rolled back via rtpse_regulator_disable through
devm_add_action_or_reset, but there does not seem to be a matching
devm_add_action_or_reset to call rtpse_set_global_state(false) on
probe-failure or driver-removal. Is that asymmetry intentional?

[ ... ]

> diff --git a/drivers/net/pse-pd/realtek-pse-uart.c b/drivers/net/pse-pd/realtek-pse-uart.c
> new file mode 100644
> index 000000000000..785407b379a4
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-uart.c

[ ... ]

> +static size_t rtpse_uart_receive(struct serdev_device *serdev,
> +				 const u8 *buf, size_t count)
> +{
> +	struct rtpse_uart *ctx = serdev_device_get_drvdata(serdev);
> +	bool done = false;
> +	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;
> +			done = (ctx->rx_len == sizeof(ctx->rx_buf));
> +		}
> +	}
> +	if (done)
> +		complete(&ctx->rx_done);
> +
> +	/* consume all to avoid desync/misalignment */
> +	return count;
> +}

[ ... ]

> +static int rtpse_uart_send(struct rtpse_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> +	struct rtpse_uart *ctx = to_rtpse_uart(pse);
> +	int written;
> +
> +	/* clear any leftover rx state before transmitting */
> +	reinit_completion(&ctx->rx_done);
> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock)
> +		ctx->rx_len = 0;

[High]
Is there a race between reinit_completion() here and the asynchronous
serdev receive_buf callback when a previous transaction has timed out
with bytes still in flight?

reinit_completion() is an unlocked x->done = 0 write, and the matching
complete() in rtpse_uart_receive() is called outside rx_lock. The
following interleave seems possible:

  rtpse_uart_send() runs reinit_completion()       /* done = 0 */
  rtpse_uart_receive() takes rx_lock, fills rx_buf
    to 12 bytes, computes done=true, releases lock
  rtpse_uart_send() takes rx_lock, sets rx_len = 0,
    releases lock
  rtpse_uart_receive() calls complete(&ctx->rx_done) /* done = 1 */
  rtpse_uart_send() writes the new request
  rtpse_uart_recv() wait_for_completion_timeout()
    returns immediately (done was 1), then sees
    rx_len == 0 and returns -EIO

rtpse_discover()'s retry whitelist covers -ETIMEDOUT, -ENXIO,
-EREMOTEIO, and -EAGAIN but not -EIO, so a single occurrence of this
race during the boot retry window would break out of the loop and turn
into a permanent dev_err_probe() failure.

Would moving complete() inside rx_lock in rtpse_uart_receive(), or
performing reinit_completion() and rx_len = 0 together inside rx_lock
while excluding the completer, close this window?

> +
> +	written = serdev_device_write(ctx->serdev, (const u8 *)req, sizeof(*req),
> +				      RTPSE_UART_TX_TIMEOUT);
> +	if (written < 0)
> +		return written;
> +	if (written != sizeof(*req))
> +		return -EIO;
> +
> +	return 0;
> +}

[ ... ]