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

  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