Re: [PATCH v3 2/3] i3c: master: dw: Map CCC hardware errors to I3C M0/M2

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Premature transfer completion and queue desynchronization due to missing completion checks in the ISR.
- [High] When a transfer times out, the dequeue function does not start the next pending transfer, causing all queued transfers to stall and timeout.
- [High] The `dw_i3c_master_daa` function ignores the result of the ENTDAA transfer and `xfer->ret`. If the transfer times out, it misinterprets this as a successful assignment and instantiates phantom I3C devices.
- [High] A timed-out transfer leaves uncleared error interrupt status in the hardware, which can cause a subsequent transfer to be falsely completed with success.
--

commit 5f1a48847f4d3d2352ca97a610c727bc9bbd9942
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

i3c: master: dw: Map CCC hardware errors to I3C M0/M2

This patch maps DesignWare response-queue status errors to standard I3C
error codes in ccc->err. It also ensures ccc->err is reset to
I3C_ERROR_UNKNOWN before each transfer.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index e42cf3ddcec8f..c4004f87c6857 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -493,6 +493,7 @@ static void dw_i3c_master_end_xfer_locked(struct dw_i3c_master *master, u32 isr)

[Severity: Critical]
This is a pre-existing issue, but I have a question about how this function
interacts with the interrupt handler.

If dw_i3c_master_irq_handler() unconditionally calls
dw_i3c_master_end_xfer_locked() for any unmasked interrupt, including
INTR_IBI_THLD_STAT (In-Band Interrupt), could this lead to an issue when
a transfer is still executing?

When nresp is 0, the response loop finishes without errors, ret remains 0,
and it calls complete(&xfer->comp).

Could this prematurely abort the current transfer on the software side,
reporting it as completely successful while the hardware is still performing
DMA, potentially leading to data corruption or use-after-free on transfer
buffers?

>  			break;
>  		case RESPONSE_ERROR_PARITY:
>  		case RESPONSE_ERROR_IBA_NACK:
> +		case RESPONSE_ERROR_ADDRESS_NACK:
>  		case RESPONSE_ERROR_TRANSF_ABORT:
>  		case RESPONSE_ERROR_CRC:
>  		case RESPONSE_ERROR_FRAME:

[ ... ]

> @@ -742,8 +762,8 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
>  		dw_i3c_master_dequeue_xfer(master, xfer);
>  

[Severity: High]
This is a pre-existing issue, but I am curious about what happens when this
timeout occurs and dw_i3c_master_dequeue_xfer() is called.

In dw_i3c_master_dequeue_xfer_locked(), when a timed-out transfer is
cancelled, it nullifies master->xferqueue.cur and resets the hardware
queues. However, it does not pop the next transfer from
master->xferqueue.list or issue a DEV_CTRL_RESUME command.

Does this strand all previously queued transfers so they eventually time
out? And if a new transfer is enqueued, will it see cur == NULL, bypass
the stranded transfers, and cause out-of-order execution?

[Severity: High]
Additionally, this is another pre-existing issue on the same timeout path:
when the hardware queues are reset, it appears INTR_TRANSFER_ERR_STAT is
not cleared from the hardware's interrupt status register.

When a new transfer is enqueued and started, could the pending error
interrupt immediately trigger dw_i3c_master_irq_handler()?

Since the queue was reset, nresp would be 0. The loop over responses would
execute 0 times, leaving ret = 0. This seems like it would falsely signal
that the new transfer completed successfully. Is this sequence possible?

>  	ret = xfer->ret;
> -	if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
> -		ccc->err = I3C_ERROR_M2;
> +	cmd = &xfer->cmds[0];
> +	dw_i3c_ccc_map_err(ccc, cmd);
>  
>  	return ret;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but observing the timeout handling here brings
up a question about dw_i3c_master_daa() in the same driver.

If wait_for_completion_timeout() fails during an ENTDAA transfer in
dw_i3c_master_daa(), cmd->rx_len remains 0. The function appears to ignore
xfer->ret and computes:

newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);

With rx_len == 0, the mask covers all maxdevs slots. Intersecting this with
unassigned slots seems to register a phantom device for every empty slot on
the bus, and then returns 0 to falsely signal a successful DAA procedure.

Should dw_i3c_master_daa() check the result of the transfer to avoid
instantiating phantom I3C devices on a timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781142158.git.tze.yee.ng@altera.com?part=2

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c