Re: [PATCH 04/12] i3c: mipi-i3c-hci: Fix race in DMA ring 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 04/12] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue
Date: Wed, 4 Mar 2026 19:58:56 +0200	[thread overview]
Message-ID: <9353717f-ef23-4e5c-aefe-fcd173952a32@intel.com> (raw)
In-Reply-To: <aaXjliGPkW01JIgj@lizhi-Precision-Tower-5810>

On 02/03/2026 21:23, Frank Li wrote:
> On Mon, Mar 02, 2026 at 10:43:34AM +0200, Adrian Hunter wrote:
>> On 27/02/2026 18:18, Frank Li wrote:
>>> On Fri, Feb 27, 2026 at 04:11:41PM +0200, Adrian Hunter wrote:
>>>> The HCI DMA dequeue path (hci_dma_dequeue_xfer()) may be invoked for
>>>> multiple transfers that timeout around the same time.  However, the
>>>> function is not serialized and can race with itself.
>>>>
>>>> When a timeout occurs, hci_dma_dequeue_xfer() stops the ring, processes
>>>> incomplete transfers, and then restarts the ring.  If another timeout
>>>> triggers a parallel call into the same function, the two instances may
>>>> interfere with each other - stopping or restarting the ring at unexpected
>>>> times.
>>>
>>> how to sync with another hci_dma_queue_xfer()?
>>
>> In theory, so long as the ring remains enabled, it should be possible
>> to enqueue transfers.
>>
>> Nevertheless, the use of the ring spin lock is added in "i3c: mipi-i3c-hci:
>> Fix race between DMA ring dequeue and the interrupt handler".  The same spin
>> lock is used in hci_dma_queue_xfer().
> 
> but not use spin lock in abort. So enqueue and
> "rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);" still happen at the same
> time.

It is not ideal, but it is not covered in this patch set.

> 
> Frank
>>
>>>
>>> Frank
>>>
>>>>
>>>> Add a mutex so that hci_dma_dequeue_xfer() is serialized with respect to
>>>> itself.
>>>>
>>>> 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 | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> index f60654fbe58e..5a9af561e4cb 100644
>>>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> @@ -133,6 +133,7 @@ struct hci_rh_data {
>>>>  	struct hci_xfer **src_xfers;
>>>>  	spinlock_t lock;
>>>>  	struct completion op_done;
>>>> +	struct mutex control_mutex;
>>>>  };
>>>>
>>>>  struct hci_rings_data {
>>>> @@ -347,6 +348,7 @@ static int hci_dma_init(struct i3c_hci *hci)
>>>>  		rh->regs = hci->base_regs + offset;
>>>>  		spin_lock_init(&rh->lock);
>>>>  		init_completion(&rh->op_done);
>>>> +		mutex_init(&rh->control_mutex);
>>>>
>>>>  		rh->xfer_entries = XFER_RING_ENTRIES;
>>>>
>>>> @@ -549,6 +551,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>>>>  	unsigned int i;
>>>>  	bool did_unqueue = false;
>>>>
>>>> +	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) {
>>>> --
>>>> 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

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