Re: [PATCH] i3c: mipi-i3c-hci: fix atomic updates to RING_OPERATION1 register

[Adrian Hunter]

On 01/04/2026 08:53, Billy Tsai wrote:
>>> The RING_OPERATION1 register contains multiple bitfields (enqueue,
>>> software dequeue, and IBI dequeue pointers) that are updated from
>>> different contexts. Because these updates are performed via
>>> read-modify-write sequences, concurrent access from process and IRQ
>>> contexts can lead to lost updates.
>>>
>>> Example:
>>> CPU 0 (hci_dma_queue_xfer): reads RING_OPERATION1 (enq=5, deq=2)
>>> CPU 1 (hci_dma_xfer_done): reads RING_OPERATION1 (enq=5, deq=2)
> 
>> Add Adrian Hunter <adrian.hunter@xxxxxxxxx>, who add lock at equeue.
> 
>> https://lore.kernel.org/linux-i3c/20260306072451.11131-6-adrian.hunter@xxxxxxxxx/
> 
>> Dose above patch fix your problem ?
> 
> Thank you for pointing out the patch 
> 4decbbc8a8cf ("i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers") from Adrian Hunter.
> 
> While that patch addresses the parallel enqueue issue in hci_dma_queue_xfer(), it does not fully
> resolve the race conditions affecting the RING_OPERATION1 register and the overall ring state
> consistency. Specifically, I have identified several remaining vulnerabilities:
> 
>   1. Atomic RMW Race on RING_OPERATION1
>   The RING_OPERATION1 register bundles three distinct pointers: Command Enqueue, Software Dequeue,
>   and IBI Dequeue. Adrian's patch protects the RMW sequence in hci_dma_queue_xfer(), but it misses
>   the IBI completion path:
>    * Missing IBI Path: hci_dma_process_ibi() updates the IBI_DEQ_PTR field in RING_OPERATION1 without
>      taking the hci->lock.
>    * Race Scenario: If hci_dma_queue_xfer() (Process context) and hci_dma_process_ibi() (IRQ context)
>      execute simultaneously on different CPUs, they will both perform unsynchronized RMW operations on
>      the same register. The last writer will overwrite the pointer update of the first, leading to ring
>      corruption.

i3c_hci_irq_handler() holds hci->lock so hci_dma_process_ibi()
is called with the lock already held:

	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f0b5159637ca0b8feaaa95de0f5ea38f1ba26729

> 
>   1. Unsynchronized Completion Path
>   In hci_dma_xfer_done(), the patch 4decbbc8a8cf reads rh->done_ptr and enters the processing loop outside
>   of the spinlock. Furthermore, the RMW update of RING_OPERATION1 at the end of the function is only
>   partially protected. For full atomicity, the entire sequence from reading the current pointers to writing
>   back the updated pointers must be inside the critical section to prevent inconsistent state views between
>   the IRQ and Process contexts.

As above, hci->lock is already held

> 
>   1. Lack of Protection for Dequeue/Abort
>   The hci_dma_dequeue_xfer() path, which modifies rh->src_xfers and resets ring entries during a transfer
>   abort, remains unprotected by the spinlock in the existing upstream code. This leads to potential race
>   conditions where a completion interrupt might attempt to process a descriptor that is simultaneously being
>   cleared by the dequeue path.

The same patch adds hci->lock to hci_dma_dequeue_xfer()

> 
>   Conclusion:
>   My proposed patch provides a more comprehensive fix by expanding the lock coverage to ensure that all paths
>   accessing RING_OPERATION1 and shared ring state (Enqueue, SW Dequeue, and IBI Dequeue) are fully synchronized.
>   This ensures the structural integrity of the DMA rings under heavy, concurrent I/O and IBI loads.
> 
>   I believe both Adrian's improvements (like xfer_space management) and the expanded locking from my patch are
>   necessary for a robust fix.
> 
> Note: My based branch of this patch is i3c/fixes.

The relevant patches are there:

	https://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux.git/log/?h=i3c/fixes

And in mainline:

	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i3c/master/mipi-i3c-hci/core.c#n621

4167b8914463132654e01e16259847d097f8a7f7 i3c: mipi-i3c-hci: Use ETIMEDOUT instead of ETIME for timeout errors
fa9586bd77ada1e3861c7bef65f6bb9dcf8d9481 i3c: mipi-i3c-hci: Fix Hot-Join NACK
f3bcbfe1b8b0b836b772927f75f8cb6e759eb00a i3c: mipi-i3c-hci: Factor out DMA mapping from queuing path
fa12bb903bc3ed1826e355d267fe134bde95e23c i3c: mipi-i3c-hci: Consolidate spinlocks
4decbbc8a8cf0a69ab011d7c2c88ed3cd0a00ddd i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers
1dca8aee80eea76d2aae21265de5dd64f6ba0f09 i3c: mipi-i3c-hci: Fix race in DMA ring dequeue
f0b5159637ca0b8feaaa95de0f5ea38f1ba26729 i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and interrupt handler
b795e68bf3073d67bebbb5a44d93f49efc5b8cc7 i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue
ec3cfd835f7c4bbd23bc9ad909d2fdc772a578bb i3c: mipi-i3c-hci: Add missing TID field to no-op command descriptor
b6d586431ae20d5157ee468d0ef62ad26798ef13 i3c: mipi-i3c-hci: Restart DMA ring correctly after dequeue abort
7ac45bc68f089887ab3a70358057edb7e6b6084e i3c: mipi-i3c-hci: Consolidate common xfer processing logic
e44d2719225e618dde74c7056f8e6949f884095e i3c: mipi-i3c-hci: Fix race in DMA error handling in interrupt context
c6396b835a5e599c4df656112140f065bb544a24 i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization
9a258d1336f7ff3add8b92d566d3a421f03bf4d2 i3c: mipi-i3c-hci: Fallback to software reset when bus disable fails