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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The PoE power supply regulator is enabled after the PSE system is globally enabled, which can trigger hardware faults.
- [Medium] A race condition in the UART transport between `rtpse_uart_send()` and `rtpse_uart_receive()` can cause spurious transaction failures (-EIO) for new commands.
--

commit ee055c1d6f6679a77708ffd42882e725da96d11d
Author: Jonas Jelonek <jelonek.jonas@gmail.com>

net: pse-pd: add Realtek/Broadcom PSE MCU driver

A range of PoE switches use a small microcontroller on the PCB to front
the actual PSE silicon. The host CPU talks to that MCU over I2C/SMBus or
UART using a fixed 12-byte request/response protocol.

> diff --git a/drivers/net/pse-pd/realtek-pse-core.c b/drivers/net/pse-pd/realtek-pse-core.c
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-core.c

[ ... ]

> +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);

[Severity: High]
Does this enable sequence trigger hardware faults? If rtpse_set_global_state()
is called before regulator_enable(), the PSE chips might actively sense and
deliver power before their main power rail is stable. This could cause the
hardware to detect an Under-Voltage Lockout (UVLO) fault. 

Should the poe_supply be enabled and allowed to stabilize before globally
enabling the system?

> diff --git a/drivers/net/pse-pd/realtek-pse-uart.c b/drivers/net/pse-pd/realtek-pse-uart.c
> --- /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)
> +{

[ ... ]

> +	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;

[Severity: Medium]
Can this sequence cause spurious transaction failures for new commands? 
In rtpse_uart_send(), reinit_completion() is called outside the rx_lock
before clearing rx_len. Concurrently, in rtpse_uart_receive(), complete()
is also called outside the spinlock.

If a delayed UART RX interrupt from a previous timed-out transaction fires
just as a new transaction starts, could the interrupt handler fill rx_buf
and call complete() after the new send reinitializes the completion, but
before it clears rx_len? 

This might cause the new transaction to return immediately from
wait_for_completion_timeout() and falsely fail with -EIO due to rx_len
being evaluated as incomplete.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612132944.460646-1-jelonek.jonas@gmail.com?part=2