Re: [PATCH] i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message contradicts the implementation and inline comments regarding GET buffer updates on a failed SET command.
--

commit 00d2a5b4dfc14cff243cee94eac37b1733ad5a1d
Author: Shubham Patil <shubhamsanjay.patil@amd.com>

i3c: master: dw: Clamp GETMRL/GETMWL to controller FIFO limits
    
The DW master rejects private SDR transfers larger than caps.datafifodepth
with -EOPNOTSUPP. Targets often report MRL/MWL values larger than that
FIFO, so the core stores limits the controller cannot meet.

> After a successful GETMRL/GETMWL, issue Direct SETMRL/SETMWL to the
> same target with lengths capped to the data FIFO (in bytes), then
> rewrite the GET payload so the core keeps the same values. Only update
> the GET buffer once SET is acked, so a failed SET does not leave the
> core and the target disagreeing.

[Severity: Medium]
Is this commit message accurate? The implementation correctly updates the GET
buffer regardless of the SET outcome to protect the controller's FIFO, matching
the inline comments, but this directly violates the claim here that the buffer
is only updated if SET is acked.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 4563d8761ba0b..51defcb577619 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -844,6 +850,130 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
> +/*
> + * Cap the limits a target reported through GETMRL to what this controller can
> + * actually transfer, so the core never asks for a private read the data FIFO
> + * cannot hold. The optional IBI payload byte is capped to the IBI queue depth
> + * instead; since that byte is a u8, the IBI cap only ever applies to
> + * controllers whose IBI queue is smaller than 255 bytes.
> + *
> + * Direct SETMRL is optional, so a target may implement GETMRL and NACK the SET.
> + * Clamp the values handed back to the core either way: a failed SET only means
> + * the target keeps its own larger limit, which is harmless as long as the core
> + * stays within ours.
> + */
> +static int dw_i3c_master_clamp_mrl(struct dw_i3c_master *master,
> +				   struct i3c_ccc_cmd *ccc)
> +{
[ ... ]
> +	ret = dw_i3c_ccc_set(master, &set_cmd);
> +	if (ret)
> +		dev_dbg(&master->base.dev,
> +			"SETMRL not accepted by target: %d\n", ret);
> +
> +	if (clamp_read) {
> +		mrl->read_len = cpu_to_be16(max_fifo_bytes);
> +		dev_dbg(&master->base.dev,
> +			"clamped target MRL from %u to %u bytes (FIFO depth limit)\n",
> +			read_len, max_fifo_bytes);
> +	}

[Severity: Medium]
Could this update in dw_i3c_master_clamp_mrl() lead to developer confusion
regarding core state synchronization? It updates the MRL buffer unconditionally
after a potentially failed SET command, contrary to the commit message.

[ ... ]
> +static int dw_i3c_master_clamp_mwl(struct dw_i3c_master *master,
> +				   struct i3c_ccc_cmd *ccc)
> +{
[ ... ]
> +	ret = dw_i3c_ccc_set(master, &set_cmd);
> +	if (ret)
> +		dev_dbg(&master->base.dev,
> +			"SETMWL not accepted by target: %d\n", ret);
> +
> +	mwl->len = cpu_to_be16(max_fifo_bytes);

[Severity: Medium]
Similarly, does this unconditional update in dw_i3c_master_clamp_mwl() need
clarification in the commit message to avoid confusion about when the GET
buffer is updated?

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