Re: [PATCH 06/12] i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue

From: Adrian Hunter <adrian.hunter@intel.com>
To: Frank Li <Frank.li@nxp.com>
Cc: <alexandre.belloni@bootlin.com>, <linux-i3c@lists.infradead.org>
Subject: Re: [PATCH 06/12] i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue
Date: Mon, 2 Mar 2026 10:45:11 +0200	[thread overview]
Message-ID: <e20c5876-01ba-4153-8317-25e70337144b@intel.com> (raw)
In-Reply-To: <aaHFjYAWOlvNkYDQ@lizhi-Precision-Tower-5810>

On 27/02/2026 18:25, Frank Li wrote:
> On Fri, Feb 27, 2026 at 04:11:43PM +0200, Adrian Hunter wrote:
>> The logic used to abort the DMA ring contains several flaws:
>>
>>  1. The driver unconditionally issues a ring abort even when the ring has
>>     already stopped.
>>  2. The completion used to wait for abort completion is never
>>     re-initialized, resulting in incorrect wait behavior.
>>  3. The abort sequence unintentionally clears RING_CTRL_ENABLE, which
>>     resets hardware ring pointers and disrupts the controller state.
>>  4. If the ring is already stopped, the abort operation should be
>>     considered successful without attempting further action.
>>
>> Fix the abort handling by checking whether the ring is running before
>> issuing an abort, reinitializing the completion when needed, ensuring that
>> RING_CTRL_ENABLE remains asserted during abort, and treating an already
>> stopped ring as a successful condition.
>>
>> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/i3c/master/mipi-i3c-hci/dma.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> index 8d5f808e03ea..dff96b84479e 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -550,18 +550,25 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>>  	struct hci_rh_data *rh = &rings->headers[xfer_list[0].ring_number];
>>  	unsigned int i;
>>  	bool did_unqueue = false;
>> +	u32 ring_status;
>>
>>  	guard(mutex)(&rh->control_mutex);
>>
>> -	/* stop the ring */
>> -	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
>> -	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
>> -		/*
>> -		 * We're deep in it if ever this condition is ever met.
>> -		 * Hardware might still be writing to memory, etc.
>> -		 */
>> -		dev_crit(&hci->master.dev, "unable to abort the ring\n");
>> -		WARN_ON(1);
>> +	ring_status = rh_reg_read(RING_STATUS);
>> +	if (ring_status & RING_STATUS_RUNNING) {
>> +		/* stop the ring */
>> +		reinit_completion(&rh->op_done);
>> +		rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE | RING_CTRL_ABORT);
>> +		wait_for_completion_timeout(&rh->op_done, HZ);
>> +		ring_status = rh_reg_read(RING_STATUS);
>> +		if (ring_status & RING_STATUS_RUNNING) {
> 
> Do you need readl_poll_timeout() here to make sure hardware actual stopped?

No the completion already waits for op_done

> 
> Frank
> 
>> +			/*
>> +			 * We're deep in it if ever this condition is ever met.
>> +			 * Hardware might still be writing to memory, etc.
>> +			 */
>> +			dev_crit(&hci->master.dev, "unable to abort the ring\n");
>> +			WARN_ON(1);
>> +		}
>>  	}
>>
>>  	spin_lock_irq(&rh->lock);
>> --
>> 2.51.0
>>
>>
>> --
>> linux-i3c mailing list
>> linux-i3c@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-i3c


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