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

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

[Billy Tsai]

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)
CPU 0: updates enq to 6, writes back (enq=6, deq=2)
CPU 1: updates deq to 3, writes back (enq=5, deq=3) <--Pointer 6 is LOST!

Fix this by wrapping all accesses to RING_OPERATION1 and associated
ring state variables (xfer_space, done_ptr, ibi_chunk_ptr) in the unified
hci->lock. This ensures that the read-modify-write sequence is atomic
across enqueue, dequeue, and completion paths.

Specifically:
- Add hci->lock protection to hci_dma_xfer_done() and
hci_dma_process_ibi().
- Ensure hci_dma_queue_xfer() and hci_dma_dequeue_xfer() use the unified
  lock consistently.
- Ensure the software-maintained ring pointers are synchronized with
  the hardware pointer updates.

Fixes: 9ad9a52cce28 ("i3c/master: introduce the mipi-i3c-hci driver")
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/i3c/master/mipi-i3c-hci/dma.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index e487ef52f6b4..3d967157411e 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -617,10 +617,12 @@ static int hci_dma_handle_error(struct i3c_hci *hci, struct hci_xfer *xfer_list,
 static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 {
 	u32 op1_val, op2_val, resp, *ring_resp;
-	unsigned int tid, done_ptr = rh->done_ptr;
+	unsigned int tid, done_ptr;
 	unsigned int done_cnt = 0;
 	struct hci_xfer *xfer;
 
+	spin_lock(&hci->lock);
+	done_ptr = rh->done_ptr;
 	for (;;) {
 		op2_val = rh_reg_read(RING_OPERATION2);
 		if (done_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val))
@@ -659,6 +661,7 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 	op1_val &= ~RING_OP1_CR_SW_DEQ_PTR;
 	op1_val |= FIELD_PREP(RING_OP1_CR_SW_DEQ_PTR, done_ptr);
 	rh_reg_write(RING_OPERATION1, op1_val);
+	spin_unlock(&hci->lock);
 }
 
 static int hci_dma_request_ibi(struct i3c_hci *hci, struct i3c_dev_desc *dev,
@@ -716,6 +719,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 	void *ring_ibi_data;
 	dma_addr_t ring_ibi_data_dma;
 
+	spin_lock(&hci->lock);
 	op1_val = rh_reg_read(RING_OPERATION1);
 	deq_ptr = FIELD_GET(RING_OP1_IBI_DEQ_PTR, op1_val);
 
@@ -767,6 +771,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 		dev_dbg(&hci->master.dev,
 			"no LAST_STATUS available (e=%d d=%d)",
 			enq_ptr, deq_ptr);
+		spin_unlock(&hci->lock);
 		return;
 	}
 	deq_ptr = last_ptr + 1;
@@ -849,6 +854,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 
 	/* and tell the hardware about freed chunks */
 	rh_reg_write(CHUNK_CONTROL, rh_reg_read(CHUNK_CONTROL) + ibi_chunks);
+	spin_unlock(&hci->lock);
 }
 
 static bool hci_dma_irq_handler(struct i3c_hci *hci)

---
base-commit: f311a05784634febd299f03476b80f3f18489767
change-id: 20260331-i3c-hci-dma-lock-a700a140c845

Best regards,
-- 
Billy Tsai <billy_tsai@aspeedtech.com>

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

[Frank Li]

On Tue, Mar 31, 2026 at 08:12:23PM +0800, 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@intel.com>, who add lock at equeue.

https://lore.kernel.org/linux-i3c/20260306072451.11131-6-adrian.hunter@intel.com/

Dose above patch fix your problem ?

Frank

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

[Billy Tsai]

> > 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@intel.com>, who add lock at equeue.

> https://lore.kernel.org/linux-i3c/20260306072451.11131-6-adrian.hunter@intel.com/

> 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.

  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.

  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.

  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.

Billy Tsai

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@intel.com>, who add lock at equeue.
> 
>> https://lore.kernel.org/linux-i3c/20260306072451.11131-6-adrian.hunter@intel.com/
> 
>> 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

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

[Billy Tsai]

Hi Adrian,

Thank you for the clarification. I overlooked the lock in i3c_hci_irq_handler() in core.c.

Best regards,

Billy Tsai

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

[Alexandre Belloni]

Hello,

On 01/04/2026 08:04:45+0000, Billy Tsai wrote:
> Hi Adrian,
> 
> Thank you for the clarification. I overlooked the lock in i3c_hci_irq_handler() in core.c.

Just to be clear, did you use AI to find the issue ?


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Billy Tsai]

On 01/04/2026 08:04:45+0000, Billy Tsai wrote:
> > Hi Adrian,
> >
> > Thank you for the clarification. I overlooked the lock in i3c_hci_irq_handler() in core.c.

> Just to be clear, did you use AI to find the issue ?

We have encountered a similar issue before, but addressed it using a different approach:
https://github.com/AspeedTech-BMC/linux/commit/aaf426c3d00ca06a5c0bd3667591ab579896726e
https://github.com/AspeedTech-BMC/linux/commit/a9428fffc7e64242b880ceee5443d094bc6cae14

For the upstream version, I noticed that you have also added fixes for this issue. I used
an AI tool to review and compare your solution with ours, and it highlighted the 
differences in this patch.

Thanks