Re: [PATCH] i3c: dw: reduce do_daa time if there's no client

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A late hardware response from a timed-out transfer can erroneously complete a newly enqueued transfer due to insufficient hardware abortion.
- [High] Ignoring `xfer->ret` in `dw_i3c_master_daa` leads to Undefined Behavior in `GENMASK` and phantom device registration if the hardware completes with an error.
- [High] Queue stall in `dw_i3c_master_dequeue_xfer_locked` due to failure to start the next transfer after a timeout.
--

--- Patch [1]: [PATCH] i3c: dw: reduce do_daa time if there's no client ---
commit 68e06554d2c19f165bbc91e6eef6e5857d1b346a
Author: Jisheng Zhang <jszhang@kernel.org>

i3c: dw: reduce do_daa time if there's no client

If there's no client, wait for completion will timeout, then there's no
need to add i3c dev at all. This can save boot time by ~6s if there's
no client connected to the i3c interface.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 2f8c0c4683e06..ae4ac37cc08c3 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -885,8 +885,10 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
>  		      COMMAND_PORT_ROC;
>  
>  	dw_i3c_master_enqueue_xfer(master, xfer);
> -	if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
> +	if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT)) {
>  		dw_i3c_master_dequeue_xfer(master, xfer);

[Severity: High]
This is a pre-existing issue, but does this dequeue logic leave the hardware
susceptible to late responses?

Looking at dw_i3c_master_dequeue_xfer_locked(), it resets the software queues:

dw_i3c_master_dequeue_xfer_locked() {
    ...
    writel(RESET_CTRL_RX_FIFO | RESET_CTRL_TX_FIFO |
           RESET_CTRL_RESP_QUEUE | RESET_CTRL_CMD_QUEUE,
           master->regs + RESET_CTRL);
    ...
}

However, this does not halt or soft-reset the underlying hardware state
machine (using RESET_CTRL_SOFT). If the transaction is merely delayed on the
bus and finishes later, won't a response be pushed to the newly cleared
RESP_QUEUE? Since a subsequent single-command transfer (like another DAA)
will use a Transaction ID (TID) of 0, could the IRQ handler mistakenly apply
the late response to the new transfer, leading to state mismatch and data
corruption?

[Severity: High]
This is also a pre-existing issue, but does dw_i3c_master_dequeue_xfer_locked()
strand other pending transfers when a timeout occurs?

When setting the current transfer to NULL on timeout:

dw_i3c_master_dequeue_xfer_locked() {
    ...
    master->xferqueue.cur = NULL;
    ...
}

The driver fails to pull and start the next transfer from master->xferqueue.list.

Because cur is NULL, the next time dw_i3c_master_enqueue_xfer() is called,
it will bypass the queue list entirely, set the new transfer as cur, and
start it immediately:

dw_i3c_master_enqueue_xfer() {
    ...
    if (master->xferqueue.cur) {
        list_add_tail(&xfer->node, &master->xferqueue.list);
    } else {
        master->xferqueue.cur = xfer;
        dw_i3c_master_start_xfer_locked(master);
    }
}

Will this cause older queued transfers to languish in the list forever and
inevitably fail via timeout?

> +		goto rpm_out;
> +	}
>  
>  	newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);

[Severity: High]
This isn't a bug introduced by this patch, but if the transfer completes
with a hardware error (like a parity error or NACK), is xfer->ret ignored?

If the completion succeeds but xfer->ret indicates an error occurred before
any devices were assigned, cmd->rx_len will still equal the requested count
(master->maxdevs). This makes the calculation:

master->maxdevs - cmd->rx_len - 1

evaluate to -1.

Doesn't GENMASK(-1, 0) trigger C undefined behavior by shifting by the
type's width, resulting in ~0UL? This looks like it would cause the driver
to erroneously register all free slots as phantom devices, which could lead
to a system panic when communicating with them later.

>  	newdevs &= ~olddevs;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803140307.12665-1-jszhang@kernel.org?part=1

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