Re: [PATCH v2 1/3] i3c: master: dw: Report actual GET CCC payload length on success

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Timeout of an active transfer stalls the hardware queue indefinitely for pending transfers, causing out-of-order execution.
- [High] The Dynamic Address Assignment (DAA) function ignores transfer errors and registers bogus phantom devices on timeout or NACK.
- [High] Premature completion of a newly enqueued transfer after a timeout due to uncleared IRQ status.
--

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

i3c: master: dw: Report actual GET CCC payload length on success

On successful GET CCC transfers, set dests[0].payload.len from
RESPONSE_PORT_DATA_LEN so the I3C core receives the number of bytes
actually read. Core helpers such as i3c_master_getmrl_locked() use
dest.payload.len after the transfer to interpret the response.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index a7593d6efac5a..e42cf3ddcec8f 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -780,7 +783,10 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
>  		dw_i3c_master_dequeue_xfer(master, xfer);

[Severity: High]
This isn't a bug introduced by this patch, but does
dw_i3c_master_dequeue_xfer_locked() stall the hardware queue for pending
transfers?

When a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the
hardware queues and sets master->xferqueue.cur = NULL:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_dequeue_xfer_locked() {
    if (master->xferqueue.cur == xfer) {
        ...
        master->xferqueue.cur = NULL;

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

However, it doesn't appear to advance the queue by popping the next transfer
from master->xferqueue.list and calling dw_i3c_master_start_xfer_locked().
Could this leave any pending transfers stranded indefinitely? A subsequently
enqueued transfer might bypass the stranded items because cur == NULL triggers
the immediate start of the new transfer, leading to out-of-order execution
and eventual failure of the stranded transfers via their own timeouts.

[Severity: High]
This isn't a bug introduced by this patch, but can an uncleared IRQ status
cause a premature completion of a newly enqueued transfer after a timeout?

If a transfer times out, dw_i3c_master_dequeue_xfer_locked() resets the queues
via RESET_CTRL but does not seem to clear pending interrupt flags in the
INTR_STATUS register.

If the interrupt from the timed-out transfer is still pending and a new
transfer is enqueued, making it master->xferqueue.cur, would the IRQ handler
falsely complete the current transfer?

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
    ...
    dw_i3c_master_end_xfer_locked(master, status);
}

It seems the handler might read nresp = 0 and then erroneously call
complete(&xfer->comp) with ret = 0, signaling that the newly enqueued transfer
has completed successfully before it has actually finished.

[Severity: High]
This isn't a bug introduced by this patch, but does dw_i3c_master_daa()
ignore transfer errors and register phantom devices on timeout or NACK?

In dw_i3c_master_daa(), the result of the ENTDAA transfer is processed without
checking the return value of xfer->ret:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_daa() {
    ...
    if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
        dw_i3c_master_dequeue_xfer(master, xfer);

    newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);
    newdevs &= ~olddevs;
}

If the transfer times out, xfer->ret is set to -ETIMEDOUT and cmd->rx_len
remains 0. The mask then falsely encompasses all available unassigned device
slots, which could deceive the driver into thinking all devices were
successfully assigned. Does this flood the I3C subsystem with non-existent
phantom devices?

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

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