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

[Jonas Jelonek <jelonek.jonas@gmail.com>]

Hi,

On 15.06.26 11:07, Simon Horman 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.
> 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?

This has been the hardest part so far but the solution I have right now
seems the "best" one so far. But this part needs some guidance from
DT maintainers.

I would hold against the AIs suggestions due to particular reasons. Using
specific compatibles for the PSE chips would be very wrong IMO because:

(1) It would claim those PSE chips are what is interfaced on the bus. But
     this is not true, there's only the MCU which mostly hides the PSE chips.
(2) The PSE chips could be used without such an MCU. While this is rather
     theoretical for Realtek PSE chips, it is definitely the case for the
     Broadcom PSE chips. And in that case, they even use a different
     for communication, called Broadcom Serial Control.

On the other side, using specific MCU compatibles would be wrong too:

(1) The MCU silicon is irrelevant to the PSE/PoE system. Those are just
     general purpose MCUs where the supplied firmware on that MCU,
     not the silicon, defines the interface to the host.
(2) The MCUs are from various vendors (here STMicro, GigaDevice,
     Nuvoton), since they are general purpose they're likely used in various
     other applications. Claiming compatibles here for a specific
     application is IMO wrong.

Finally, why using the Realtek prefix/scope: The system that is described
by the bindings and supported by the driver has so far been seen solely
on Realtek-based switches. Some SDK sources already include a driver
implementation for that. Though there is no 100% evidence of that, it
seems quite reasonable that Realtek provides this MCU+firmware+PSE
silicon as kind of a package. Thus, Realtek defines and provides the
interface and behavior. The suffix then just denotes the protocol dialect
used, correlating with the PSE silicon behind the MCU.

In older switch generations, Realtek just decided to use BCM59xxx PSE
chips until they developed and shipped their owns. Likely because
the capabilities of BCM and Realtek PSE silicon slightly differs, they
decided to alter the protocol with different opcodes and slightly
different command set.

I've seen a similar review from Sashiko on the bindings patch, my
explanation above applies to it in the same way.

I'll try to improve the rationale a bit in the descriptions for the next
version.

>>  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?
>
> [ ... ]
>

Valid, I will add a check for the seq_num field 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?

This would be speculative IMO, because 'no answer' was the only behavior
I could observe so far. It always came up as a timeout when the MCU
wasn't ready yet.

> [ ... ]
>
>> +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?

No, this is an inconsistency. I'll fix this in the next version.

> [ ... ]
>
>> 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?
>

Valid, will fix it as suggested in the next version.

>> +
>> +	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;
>> +}
> [ ... ]

Best regards,
Jonas