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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unprotected data race on UART receive buffer state between asynchronous RX callback and process-context TX function.
- [High] I2C transport handlers swallow explicit MCU error opcodes, causing 1-second stalls and masking failures.
- [High] UART receive handler returns partially consumed bytes, leaving overflow bytes in the serdev tty buffer which corrupts subsequent frames.
- [High] The advertised 60W maximum power limit for the BCM59121 chip is physically unreachable and silently clamped to 51W.
- [Low] Use of non-existent macro `kzalloc_obj()` causes a build failure.
--

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

net: pse-pd: add Realtek/Broadcom PSE MCU driver
    
This commit introduces a new driver for the microcontroller fronting Realtek
and Broadcom PSE chips on PoE switches. It includes both I2C and UART
transport modules alongside a shared core to handle protocol dialects.

> diff --git a/drivers/net/pse-pd/realtek-pse-core.c b/drivers/net/pse-pd/realtek-pse-core.c
> index 0000000000000..711f85aebf5e8
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-core.c
[ ... ]
> +static const struct rtpse_chip_info bcm59121_info = {
> +	.device_id = RTPSE_DEVICE_ID_BCM59121,
> +	.max_mW_per_port = 60000,	/* 802.3bt Type 3 */
> +	.name = "BCM59121",
> +	.pw_read_lsb_mW = 200,
> +	.pw_set_cmd = RTPSE_CMD_PORT_SET_POWER_LIMIT,
> +	.pw_set_lsb_mW = 200,
> +};
[ ... ]
> +static int rtpse_port_set_pw_limit(struct pse_controller_dev *pcdev, int id, int max_mW)
> +{
> +	const struct rtpse_opcode *type_opc, *val_opc;
> +	struct rtpse_ctrl *pse = to_rtpse_ctrl(pcdev);
> +	const struct rtpse_chip_info *chip = pse->chip;
> +	unsigned int prg_val;
> +	int ret;
> +
> +	if (max_mW < 0 || max_mW > chip->max_mW_per_port)
> +		return -ERANGE;
[ ... ]
> +	prg_val = min_t(unsigned int, max_mW / chip->pw_set_lsb_mW, 0xff);
> +
> +	return rtpse_port_cmd(pse, id, val_opc->op, prg_val);
> +}

[Severity: High]
Is the 60W maximum power limit for the BCM59121 chip physically unreachable
here? 

With max_mW_per_port set to 60000 and pw_set_lsb_mW set to 200, programming
60W results in a target of 300. Since this is packed into an 8-bit command
payload, the min_t calculation clamps the value to 0xff (255). 

This seems to silently restrict the maximum output to 51W, which could break
functionality for devices expecting the advertised power budget.

[ ... ]
> +static int rtpse_port_get_pw_limit_ranges(struct pse_controller_dev *pcdev, int id,
> +					  struct pse_pw_limit_ranges *out)
> +{
> +	struct ethtool_c33_pse_pw_limit_range *range;
> +	struct rtpse_ctrl *pse = to_rtpse_ctrl(pcdev);
> +
> +	range = kzalloc_obj(*range, GFP_KERNEL);
> +	if (!range)
> +		return -ENOMEM;

[Severity: Low]
Will this code compile successfully? There does not appear to be a
kzalloc_obj() macro in the standard kernel API, which might lead to an
immediate build failure. Should this just be kzalloc() instead?

> diff --git a/drivers/net/pse-pd/realtek-pse-i2c.c b/drivers/net/pse-pd/realtek-pse-i2c.c
> index 0000000000000..8b9c31cbdfe99
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-i2c.c
[ ... ]
> +static int rtpse_i2c_smbus_recv(struct rtpse_ctrl *pse,
> +				const struct rtpse_mcu_msg *req,
> +				struct rtpse_mcu_msg *resp)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +	int tries, ret;
> +
> +	for (tries = 0; tries < RTPSE_I2C_MAX_TRIES; tries++) {
> +		if (tries > 0)
> +			msleep(RTPSE_I2C_RETRY_MS);
> +
> +		/* MCU needs 0x00 as command byte for read */
> +		ret = i2c_smbus_read_i2c_block_data(client, 0x00,
> +						    RTPSE_MCU_MSG_SIZE,
> +						    (u8 *)resp);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == RTPSE_MCU_MSG_SIZE && resp->opcode == req->opcode)
> +			return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}

[Severity: High]
Do these transport loops inadvertently swallow explicit MCU error opcodes?

If the MCU returns a valid error opcode like 0xfd (request incomplete) or
0xfe (checksum error), resp->opcode won't match req->opcode. 

Instead of returning the error so the core can translate and handle it,
the loop appears to ignore the response and continues to poll until hitting
the timeout.

Could this needlessly stall the bus for a full second on every invalid
command in both the SMBus and native I2C receive paths?

> diff --git a/drivers/net/pse-pd/realtek-pse-uart.c b/drivers/net/pse-pd/realtek-pse-uart.c
> index 0000000000000..27fa7e0c3f95e
> --- /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);
> +	size_t take;
> +
> +	take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
> +	if (take == 0)
> +		return count;  /* drop overflow bytes */
> +
> +	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);
> +
> +	return take;
> +}

[Severity: High]
Can returning take instead of count here cause framing desynchronization?

If the handler returns a value less than count, the serdev core keeps the
unconsumed bytes buffered.

When the next transaction begins, these leftover overflow bytes might
immediately push into the new buffer, permanently misaligning future
responses. Does it make sense to unconditionally return count here to drop
any overflow bytes and allow resynchronization?

[ ... ]
> +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);
> +	ctx->rx_len = 0;

[Severity: High]
Is there a data race on ctx->rx_len between the receive callback and this
send function?

The serdev callback rtpse_uart_receive() modifies ctx->rx_len asynchronously
from a softirq or workqueue context. If a delayed byte arrives just as
rtpse_uart_send() locklessly resets ctx->rx_len to 0, it could corrupt the
buffer state.

The pse->mutex appears to only serialize host-initiated requests, but not
the asynchronous UART receive callback. Could this lead to dropped frames or
out-of-bounds writes?

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