Re: [PATCH net-next v11 3/4] net: pse-pd: realtek-pse-mcu: add I2C transport

[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: realtek-pse-mcu: add I2C transport

This adds the I2C/SMBus transport for the Realtek PSE MCU core, providing
the send/recv callbacks used to exchange the 12-byte MCU frames. Two
framings are supported and selected by the compatible: SMBus block
transfers by default, and raw I2C when the match data sets native_i2c.

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

[ ... ]

> +static int rtpse_mcu_i2c_smbus_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +
> +	/* Send opcode as SMBus command byte; remaining 11 bytes as block data */
> +	return i2c_smbus_write_i2c_block_data(client, req->opcode, RTPSE_MCU_MSG_SIZE - 1,
> +					      (u8 *)req + 1);
> +}
> +
> +static int rtpse_mcu_i2c_smbus_recv(struct rtpse_mcu_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_MCU_I2C_MAX_TRIES; tries++) {
> +		if (tries > 0)
> +			msleep(RTPSE_MCU_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);

[ ... ]

> +static int rtpse_mcu_i2c_native_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +	void *buf;
> +	int ret;
> +
> +	/* i2c_master_send() may DMA, so the buffer must not be on the stack. */
> +	buf = kmemdup(req, RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = i2c_master_send(client, buf, RTPSE_MCU_MSG_SIZE);
> +	kfree(buf);

[Severity: Low]
Is the premise in this comment correct?  i2c_master_send() and
i2c_master_recv() both go through i2c_transfer_buffer_flags(), which
builds the message as:

drivers/i2c/i2c-core-base.c:i2c_transfer_buffer_flags() {
	struct i2c_msg msg = {
		.addr = client->addr,
		.flags = flags | (client->flags & I2C_M_TEN),
		.len = count,
		.buf = buf,
	};
	...
}

I2C_M_DMA_SAFE is never set there, so an adapter that wants to DMA has to
obtain its buffer from i2c_get_dma_safe_msg_buf(), which bounces again
exactly because the flag is absent:

drivers/i2c/i2c-core-base.c:i2c_get_dma_safe_msg_buf() {
	...
	if (msg->flags & I2C_M_DMA_SAFE)
		return msg->buf;
	...
	if (msg->flags & I2C_M_RD)
		return kzalloc(msg->len, GFP_KERNEL);
	else
		return kmemdup(msg->buf, msg->len, GFP_KERNEL);
}

So does the kmemdup() here actually achieve anything?  On a DMA-capable
adapter the 12-byte frame is copied a second time by the core, and the
core's on-stack struct rtpse_mcu_msg was already legal for these APIs
(Documentation/i2c/dma-considerations.rst: "it is *not* mandatory that the
buffer of an I2C message is DMA safe").

The SMBus path in the same file already passes the caller-supplied stack
req/resp straight into i2c_smbus_write_i2c_block_data() and
i2c_smbus_read_i2c_block_data(), which seems to contradict the rationale
used for the raw-I2C path.

The same question applies to rtpse_mcu_i2c_native_recv() below, where the
kmalloc() plus the memcpy(resp, buf, RTPSE_MCU_MSG_SIZE) duplicate what
i2c_get_dma_safe_msg_buf()/i2c_put_dma_safe_msg_buf() already do for a
non-DMA-safe read buffer.  It also adds an -ENOMEM abort to a PSE control
transaction that the bus core could otherwise always complete.

If skipping the core's bounce is really the goal, should these buffers be
handed to i2c_master_send_dmasafe()/i2c_master_recv_dmasafe(), or to an
explicit i2c_msg with I2C_M_DMA_SAFE?  Otherwise, could the allocations
and both comments simply be dropped?

The commit message carries the same statement:

  "Because i2c_master_send()/i2c_master_recv() may DMA, the raw-I2C path
  bounces each frame through a heap buffer rather than the core's stack
  buffers; the SMBus path is unaffected."

Could this be reworded so it does not document a DMA rule that the I2C
core does not impose?

> +	if (ret < 0)
> +		return ret;
> +	return ret == RTPSE_MCU_MSG_SIZE ? 0 : -EIO;
> +}
> +
> +static int rtpse_mcu_i2c_native_recv(struct rtpse_mcu_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;
> +	u8 *buf;
> +
> +	/* i2c_master_recv() may DMA, so read into an off-stack buffer. */
> +	buf = kmalloc(RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
> +		if (tries > 0)
> +			msleep(RTPSE_MCU_I2C_RETRY_MS);
> +
> +		ret = i2c_master_recv(client, buf, RTPSE_MCU_MSG_SIZE);
> +		if (ret < 0)
> +			goto out;
> +		if (ret == RTPSE_MCU_MSG_SIZE) {
> +			memcpy(resp, buf, RTPSE_MCU_MSG_SIZE);

[ ... ]