Re: [PATCH v6 4/5] i3c: master: Validate GET CCC payload length and retry Direct GET once

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Indiscriminate retries of internal kernel software errors during I3C CCC GET commands.

Pre-existing issues:
- [High] renesas_i3c_rx_isr overwrites rather than appends to cmd->rx_buf, corrupting long read transfers and causing FIFO underflows.
--

--- Patch [4]: [PATCH v6 4/5] i3c: master: Validate GET CCC payload length and retry Direct GET once ---
commit 0493e80008c19d117849be20fe713d3e08ab598e
Author: Adrian Ng Ho Yin <[email protected]>

i3c: master: Validate GET CCC payload length and retry Direct GET once

This commit adds retry logic for Direct GET CCC commands and validates
the payload length of GET CCCs after a successful transfer. Failed Direct
GET CCCs are retried up to cmd->retries times when the driver reports
failure or an I3C error.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3666d011f10ea..965d74e6bf166 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1005,7 +1037,25 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
>  	    !master->ops->supports_ccc_cmd(master, cmd))
>  		return -EOPNOTSUPP;
>  
> -	return master->ops->send_ccc_cmd(master, cmd);
> +	max_attempts = cmd->retries + 1;
> +	ret = -EIO;
> +	for (attempt = 0; attempt < max_attempts; attempt++) {
> +		unsigned int i;
> +
> +		if (cmd->rnw)
> +			for (i = 0; i < cmd->ndests; i++)
> +				cmd->dests[i].payload.actual_len = 0;
> +
> +		cmd->err = I3C_ERROR_UNKNOWN;
> +		ret = master->ops->send_ccc_cmd(master, cmd);
> +		if (!ret && cmd->err == I3C_ERROR_UNKNOWN)
> +			break;

[Severity: Medium]
Does this loop also retry internal kernel software errors?

If a host software error occurs (for example, -ENOMEM during DMA descriptor
allocation, or -EOPNOTSUPP), master->ops->send_ccc_cmd() will return the
error code. This bypasses the !ret break condition, causing the loop to
retry indiscriminately.

Should the loop explicitly distinguish between bus-level I3C errors that
need retries and local software errors that should immediately abort?

> +	}
> +
> +	if (!ret)
> +		ret = i3c_ccc_validate_payload_len(cmd);
> +
> +	return ret;
>  }

[ ... ]

While reviewing the retries for GET CCC commands, I noticed a separate issue
in the Renesas I3C driver's receive interrupt handler:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_rx_isr() {
    ...
    } else {
        read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
        i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
        cmd->rx_count = read_bytes;
    }
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this overwrite the
receive buffer rather than appending to it?

When an I3C read transfer exceeds the FIFO threshold, renesas_i3c_rx_isr()
fires multiple times. Instead of appending received bytes to
cmd->rx_buf + cmd->rx_count and accumulating cmd->rx_count += read_bytes, it
writes to the base address of cmd->rx_buf and resets cmd->rx_count.

Later, renesas_i3c_resp_isr() calculates bytes_remaining = data_len -
cmd->rx_count. Because rx_count only reflects the size of the last chunk,
bytes_remaining exceeds the actual data left in the FIFO, which could cause
an underflow and read garbage data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c