Re: [PATCH V3 13/16] i3c: mipi-i3c-hci: Wait for NoOp commands to complete

[Frank Li]

On Mon, May 04, 2026 at 02:33:49PM +0300, Adrian Hunter wrote:
> When a transfer list is only partially completed due to an error,
> hci_dma_dequeue_xfer() overwrites the remaining DMA ring entries with
> NoOp commands and restarts the ring to flush them out.
>
> While NoOp commands are expected to complete successfully, they may still
> fail to complete if the DMA ring is stuck.  Explicitly wait for the NoOp
> commands to finish, and trigger controller recovery if they do not
> complete or report an error.
>
> This ensures that partially completed transfer lists are reliably
> resolved and that a stuck ring is recovered promptly.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---

Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
>
>
> Changes in V3:
>
> 	None
>
> Changes in V2:
>
> 	Rename completing_xfer to final_xfer
> 	Add missing reinit_completion()
>
>
>  drivers/i3c/master/mipi-i3c-hci/dma.c | 39 ++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index 376062c0fcbf..90fa621c9d56 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -696,11 +696,33 @@ static void hci_dma_recovery(struct i3c_hci *hci)
>  	dev_err(&hci->master.dev, "Recovery %s\n", ret ? "failed!" : "done");
>  }
>
> +static bool hci_dma_wait_for_noop(struct i3c_hci *hci, struct hci_xfer *xfer_list, int n,
> +				  int noop_pos)
> +{
> +	struct completion *done = xfer_list->final_xfer->completion;
> +	bool timeout = !wait_for_completion_timeout(done, HZ);
> +	u32 error = timeout;
> +
> +	for (int i = noop_pos; i < n && !error; i++)
> +		error = RESP_STATUS(xfer_list[i].response);
> +
> +	if (!error)
> +		return true;
> +
> +	if (timeout)
> +		dev_err(&hci->master.dev, "NoOp timeout error\n");
> +	else
> +		dev_err(&hci->master.dev, "NoOp error %u\n", error);
> +
> +	return false;
> +}
> +
>  static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  				 struct hci_xfer *xfer_list, int n)
>  {
>  	struct hci_rings_data *rings = hci->io_data;
>  	struct hci_rh_data *rh = &rings->headers[xfer_list[0].ring_number];
> +	int noop_pos = -1;
>  	unsigned int i;
>  	bool did_unqueue = false;
>  	u32 ring_status;
> @@ -708,7 +730,7 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  	guard(mutex)(&hci->control_mutex);
>
>  	spin_lock_irq(&hci->lock);
> -
> +restart:
>  	ring_status = rh_reg_read(RING_STATUS);
>  	if (ring_status & RING_STATUS_RUNNING) {
>  		/*
> @@ -765,11 +787,10 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  				*ring_data++ = 0;
>  			}
>
> -			/* disassociate this xfer struct */
> -			rh->src_xfers[idx] = NULL;
> -
> -			/* and unmap it */
> -			hci_dma_unmap_xfer(hci, xfer, 1);
> +			if (noop_pos < 0) {
> +				reinit_completion(xfer->final_xfer->completion);
> +				noop_pos = i;
> +			}
>
>  			did_unqueue = true;
>  		}
> @@ -801,6 +822,12 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>
>  	wait_for_completion_timeout(&rh->op_done, HZ);
>
> +	if (did_unqueue && !hci_dma_wait_for_noop(hci, xfer_list, n, noop_pos)) {
> +		spin_lock_irq(&hci->lock);
> +		hci->recovery_needed = true;
> +		goto restart;
> +	}
> +
>  	return did_unqueue;
>  }
>
> --
> 2.51.0
>