Re: [PATCH v9 2/2] i3c: master: Add driver for AMD AXI I3C master controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver incorrectly calculates `actual_len` for early-terminated read transfers, over-reporting received bytes and exposing padding data.
- [Medium] Mixing goto-based error handling with scope-based cleanup (__free) in the same function.
- [Medium] Zero-length I2C/I3C transfers with NULL buffers are incorrectly rejected with -EINVAL.
- [Medium] Reaching DAA limits (maximum devices or exhausted dynamic addresses) causes the driver to discard all successfully enumerated devices.
--

commit 31faf134bb0fbc7c1ffe6801a15207207118c870
Author: Manikanta Guntupalli <manikanta.guntupalli@amd.com>

i3c: master: Add driver for AMD AXI I3C master controller

Add an I3C master driver and maintainers fragment for the AMD I3C bus
controller.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> new file mode 100644
> index 0000000000000..34ab1028c3ce8
> --- /dev/null
> +++ b/drivers/i3c/master/amd-i3c-master.c
[ ... ]
> +static void xi3c_master_rd_from_rx_fifo(struct xi3c_master *master,
> +					struct xi3c_cmd *cmd)
> +{
> +	u16 rx_data_available;
> +	u16 copy_len;
> +	u16 len;
> +
> +	rx_data_available = xi3c_rd_fifo_level(master);
> +	len = rx_data_available * XI3C_WORD_LEN;
> +
> +	if (!len)
> +		return;
> +
> +	copy_len = min_t(u16, len, cmd->rx_len);
> +	xi3c_readl_fifo(master->membase + XI3C_RD_FIFO_OFFSET,
> +			(u8 *)cmd->rx_buf, copy_len);
> +
> +	cmd->rx_buf = (u8 *)cmd->rx_buf + copy_len;
> +	cmd->rx_len -= copy_len;

[Severity: High]
Does this logic correctly account for early-terminated read transfers?

If an I3C target terminates a read early with a non-word-aligned byte count,
the hardware RX FIFO might indicate a word is available, but padding bytes
are included. This driver reads the full word and decrements rx_len. Later
in xi3c_master_i3c_xfers(), actual_len is calculated from this modified
rx_len, which could over-report the received bytes and expose padding data
to upper layers.

> +}
> +
> +static int xi3c_master_read(struct xi3c_master *master, struct xi3c_cmd *cmd)
> +{
> +	unsigned long timeout;
> +	u32 status_reg;
> +	int ret;
> +
> +	if (!cmd->rx_buf || cmd->rx_len > XI3C_MAXDATA_LENGTH)
> +		return -EINVAL;

[Severity: Medium]
Will this fail on valid zero-length transfers? Zero-length reads, such as
standard SMBus Quick commands or i2cdetect scans, typically have rx_len = 0
and a NULL rx_buf. The check here would incorrectly reject them.

[ ... ]
> +static int xi3c_master_write(struct xi3c_master *master, struct xi3c_cmd *cmd)
> +{
> +	unsigned long timeout;
> +	u16 cmd_len;
> +
> +	if (!cmd->tx_buf || cmd->tx_len > XI3C_MAXDATA_LENGTH)
> +		return -EINVAL;

[Severity: Medium]
Similarly, does this incorrectly reject valid zero-length writes with a NULL
tx_buf?

[ ... ]
> +static int xi3c_master_do_daa(struct i3c_master_controller *m)
> +{
> +	u8 pid_bufs[XI3C_MAX_DEVS][XI3C_DAA_SLAVEINFO_READ_BYTECOUNT] = {};
> +	struct xi3c_master *master = to_xi3c_master(m);
> +	struct xi3c_xfer *xfer __free(kfree) = NULL;

[Severity: Medium]
Is it safe to mix __free() scope-based cleanup with goto-based error handling
in this function?

There are explicit kernel guidelines against combining goto-based error
handling with __free() scope-based resource management in the same function,
as it can lead to fragile and confusing execution flow.

[ ... ]
> +	while (true) {
> +		struct xi3c_cmd *cmd = &xfer->cmds[0];
> +		u8 daa_byte;
> +
> +		if (master->daa.index >= XI3C_MAX_DEVS) {
> +			ret = -ENOSPC;
> +			goto err_daa;
> +		}
> +
> +		addr = i3c_master_get_free_addr(m, last_addr + 1);
> +		if (addr < 0) {
> +			ret = addr;
> +			goto err_daa;
> +		}

[Severity: Medium]
If the bus reaches the maximum device count or runs out of free dynamic
addresses, jumping to err_daa will abort the enumeration process without
registering any devices that were successfully enumerated prior to this.

Is it intended to discard the earlier successfully assigned devices instead
of keeping them?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623114417.2578189-1-shubhamsanjay.patil@amd.com?part=2