[PATCH V2 00/14] i3c: mipi-i3c-hci: Fixes for v7.0

[PATCH V2 00/14] i3c: mipi-i3c-hci: Fixes for v7.0

[Adrian Hunter]

Hi

Here are some fixes to the MIPI I3C HCI driver.  Please consider them
as fixes for v7.0.

They are mostly related to error recovery and a lack of serialization.

There is a minor conflict with the "i3c: mipi-i3c-hci-pci: Enable IBI
while runtime suspended for Intel controllers" patch set.  So I will
re-send that patch set in in due course.


Changes in V2:

    i3c: mipi-i3c-hci: Use ETIMEDOUT instead of ETIME for timeout errors
	Added Frank's Rev'd-by

    i3c: mipi-i3c-hci: Fix Hot-Join NACK
	Added Frank's Rev'd-by

    i3c: mipi-i3c-hci: Factor out DMA mapping from queuing path
	New patch split out from "i3c: mipi-i3c-hci: Fix race in DMA ring
	enqueue for parallel xfers"

    i3c: mipi-i3c-hci: Consolidate spinlocks
	New patch

    i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers
	Refactor of hci_dma_queue_xfer() to do all DMA mapping first, is
	now a separate earlier patch: "i3c: mipi-i3c-hci: Factor out DMA
	mapping from queuing path"

    i3c: mipi-i3c-hci: Fix race in DMA ring dequeue
	Mutex now defined in struct i3c_hci instead of struct hci_rh_data

    i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and interrupt handler
	Now uses unified spinlock (as per earlier patch "i3c: mipi-i3c-hci:
	Consolidate spinlocks") and extends coverage to the entire interrupt
	handler

    i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue
	Add Frank's Rev'd-by
	Mutex is now defined in struct i3c_hci

    i3c: mipi-i3c-hci: Add missing TID field to no-op command descriptor
	Add Frank's Rev'd-by

    i3c: mipi-i3c-hci: Restart DMA ring correctly after dequeue abort
	Add Frank's Rev'd-by

    i3c: mipi-i3c-hci: Consolidate common xfer processing logic
	Add Frank's Rev'd-by

    i3c: mipi-i3c-hci: Fix race in DMA error handling in interrupt context
	None

    i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization
	Place irq_inactive under the protection of the now unified
	spinlock, eliminating the need for explict memory barriers.
	Correctly position the update of irq_inactive in
	i3c_hci_sync_irq_inactive() to after synchronize_irq()

    i3c: mipi-i3c-hci: Fallback to software reset when bus disable fails
	Add Frank's Rev'd-by


Adrian Hunter (14):
      i3c: mipi-i3c-hci: Use ETIMEDOUT instead of ETIME for timeout errors
      i3c: mipi-i3c-hci: Fix Hot-Join NACK
      i3c: mipi-i3c-hci: Factor out DMA mapping from queuing path
      i3c: mipi-i3c-hci: Consolidate spinlocks
      i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers
      i3c: mipi-i3c-hci: Fix race in DMA ring dequeue
      i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and interrupt handler
      i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue
      i3c: mipi-i3c-hci: Add missing TID field to no-op command descriptor
      i3c: mipi-i3c-hci: Restart DMA ring correctly after dequeue abort
      i3c: mipi-i3c-hci: Consolidate common xfer processing logic
      i3c: mipi-i3c-hci: Fix race in DMA error handling in interrupt context
      i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization
      i3c: mipi-i3c-hci: Fallback to software reset when bus disable fails

 drivers/i3c/master/mipi-i3c-hci/cmd.h    |   1 +
 drivers/i3c/master/mipi-i3c-hci/cmd_v1.c |   8 +-
 drivers/i3c/master/mipi-i3c-hci/cmd_v2.c |   8 +-
 drivers/i3c/master/mipi-i3c-hci/core.c   | 143 +++++++++++++++++-----------
 drivers/i3c/master/mipi-i3c-hci/dma.c    | 156 ++++++++++++++++---------------
 drivers/i3c/master/mipi-i3c-hci/hci.h    |   5 +
 drivers/i3c/master/mipi-i3c-hci/pio.c    |  16 +---
 7 files changed, 190 insertions(+), 147 deletions(-)


Regards
Adrian

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

[PATCH V2 01/14] i3c: mipi-i3c-hci: Use ETIMEDOUT instead of ETIME for timeout errors

[Adrian Hunter]

The MIPI I3C HCI driver currently returns -ETIME for various timeout
conditions, while other I3C master drivers consistently use -ETIMEDOUT
for the same class of errors.  Align the HCI driver with the rest of the
subsystem by replacing all uses of -ETIME with -ETIMEDOUT.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Added Frank's Rev'd-by


 drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 2 +-
 drivers/i3c/master/mipi-i3c-hci/cmd_v2.c | 2 +-
 drivers/i3c/master/mipi-i3c-hci/core.c   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
index fe260461e7e6..831a261f6c56 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
@@ -334,7 +334,7 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
 		hci->io->queue_xfer(hci, xfer, 1);
 		if (!wait_for_completion_timeout(&done, HZ) &&
 		    hci->io->dequeue_xfer(hci, xfer, 1)) {
-			ret = -ETIME;
+			ret = -ETIMEDOUT;
 			break;
 		}
 		if ((RESP_STATUS(xfer->response) == RESP_ERR_ADDR_HEADER ||
diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
index 3729e6419581..054beee36da5 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
@@ -275,7 +275,7 @@ static int hci_cmd_v2_daa(struct i3c_hci *hci)
 		hci->io->queue_xfer(hci, xfer, 2);
 		if (!wait_for_completion_timeout(&done, HZ) &&
 		    hci->io->dequeue_xfer(hci, xfer, 2)) {
-			ret = -ETIME;
+			ret = -ETIMEDOUT;
 			break;
 		}
 		if (RESP_STATUS(xfer[0].response) != RESP_SUCCESS) {
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 5879bba78164..dbe93df0c70e 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -261,7 +261,7 @@ static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m,
 		goto out;
 	if (!wait_for_completion_timeout(&done, HZ) &&
 	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
-		ret = -ETIME;
+		ret = -ETIMEDOUT;
 		goto out;
 	}
 	for (i = prefixed; i < nxfers; i++) {
@@ -340,7 +340,7 @@ static int i3c_hci_i3c_xfers(struct i3c_dev_desc *dev,
 		goto out;
 	if (!wait_for_completion_timeout(&done, HZ) &&
 	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
-		ret = -ETIME;
+		ret = -ETIMEDOUT;
 		goto out;
 	}
 	for (i = 0; i < nxfers; i++) {
@@ -388,7 +388,7 @@ static int i3c_hci_i2c_xfers(struct i2c_dev_desc *dev,
 		goto out;
 	if (!wait_for_completion_timeout(&done, m->i2c.timeout) &&
 	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
-		ret = -ETIME;
+		ret = -ETIMEDOUT;
 		goto out;
 	}
 	for (i = 0; i < nxfers; i++) {
-- 
2.51.0


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

[PATCH V2 02/14] i3c: mipi-i3c-hci: Fix Hot-Join NACK

[Adrian Hunter]

The MIPI I3C HCI host controller driver does not implement Hot-Join
handling, yet Hot-Join response control defaults to allowing devices to
Hot-Join the bus.  Configure HC_CONTROL_HOT_JOIN_CTRL to NACK all Hot-Join
attempts.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Added Frank's Rev'd-by


 drivers/i3c/master/mipi-i3c-hci/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index dbe93df0c70e..4877a321edf9 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -152,7 +152,8 @@ static int i3c_hci_bus_init(struct i3c_master_controller *m)
 	if (hci->quirks & HCI_QUIRK_RESP_BUF_THLD)
 		amd_set_resp_buf_thld(hci);
 
-	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE);
+	/* Enable bus with Hot-Join disabled */
+	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
 	dev_dbg(&hci->master.dev, "HC_CONTROL = %#x", reg_read(HC_CONTROL));
 
 	return 0;
@@ -764,7 +765,8 @@ static int i3c_hci_runtime_resume(struct device *dev)
 
 	hci->io->resume(hci);
 
-	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE);
+	/* Enable bus with Hot-Join disabled */
+	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
 
 	return 0;
 }
-- 
2.51.0


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

[PATCH V2 03/14] i3c: mipi-i3c-hci: Factor out DMA mapping from queuing path

[Adrian Hunter]

Prepare for fixing a race in the DMA ring enqueue path when handling
parallel transfers.  Move all DMA mapping out of hci_dma_queue_xfer()
and into a new helper that performs the mapping up front.

This refactoring allows the upcoming fix to extend the spinlock coverage
around the enqueue operation without performing DMA mapping under the
spinlock.

No functional change is intended in this patch.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	New patch split out from "i3c: mipi-i3c-hci: Fix race in DMA ring
	enqueue for parallel xfers"


 drivers/i3c/master/mipi-i3c-hci/dma.c | 49 ++++++++++++++++++---------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index b903a2da1fd1..ba451f026386 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -439,6 +439,33 @@ static void hci_dma_unmap_xfer(struct i3c_hci *hci,
 	}
 }
 
+static struct i3c_dma *hci_dma_map_xfer(struct device *dev, struct hci_xfer *xfer)
+{
+	enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	bool need_bounce = device_iommu_mapped(dev) && xfer->rnw && (xfer->data_len & 3);
+
+	return i3c_master_dma_map_single(dev, xfer->data, xfer->data_len, need_bounce, dir);
+}
+
+static int hci_dma_map_xfer_list(struct i3c_hci *hci, struct device *dev,
+				 struct hci_xfer *xfer_list, int n)
+{
+	for (int i = 0; i < n; i++) {
+		struct hci_xfer *xfer = xfer_list + i;
+
+		if (!xfer->data)
+			continue;
+
+		xfer->dma = hci_dma_map_xfer(dev, xfer);
+		if (!xfer->dma) {
+			hci_dma_unmap_xfer(hci, xfer_list, i);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 static int hci_dma_queue_xfer(struct i3c_hci *hci,
 			      struct hci_xfer *xfer_list, int n)
 {
@@ -446,6 +473,11 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	struct hci_rh_data *rh;
 	unsigned int i, ring, enqueue_ptr;
 	u32 op1_val, op2_val;
+	int ret;
+
+	ret = hci_dma_map_xfer_list(hci, rings->sysdev, xfer_list, n);
+	if (ret)
+		return ret;
 
 	/* For now we only use ring 0 */
 	ring = 0;
@@ -456,9 +488,6 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	for (i = 0; i < n; i++) {
 		struct hci_xfer *xfer = xfer_list + i;
 		u32 *ring_data = rh->xfer + rh->xfer_struct_sz * enqueue_ptr;
-		enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE :
-							  DMA_TO_DEVICE;
-		bool need_bounce;
 
 		/* store cmd descriptor */
 		*ring_data++ = xfer->cmd_desc[0];
@@ -477,18 +506,6 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 
 		/* 2nd and 3rd words of Data Buffer Descriptor Structure */
 		if (xfer->data) {
-			need_bounce = device_iommu_mapped(rings->sysdev) &&
-				      xfer->rnw &&
-				      xfer->data_len != ALIGN(xfer->data_len, 4);
-			xfer->dma = i3c_master_dma_map_single(rings->sysdev,
-							      xfer->data,
-							      xfer->data_len,
-							      need_bounce,
-							      dir);
-			if (!xfer->dma) {
-				hci_dma_unmap_xfer(hci, xfer_list, i);
-				return -ENOMEM;
-			}
 			*ring_data++ = lower_32_bits(xfer->dma->addr);
 			*ring_data++ = upper_32_bits(xfer->dma->addr);
 		} else {
@@ -511,7 +528,7 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 		op2_val = rh_reg_read(RING_OPERATION2);
 		if (enqueue_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val)) {
 			/* the ring is full */
-			hci_dma_unmap_xfer(hci, xfer_list, i + 1);
+			hci_dma_unmap_xfer(hci, xfer_list, n);
 			return -EBUSY;
 		}
 	}
-- 
2.51.0


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

[PATCH V2 04/14] i3c: mipi-i3c-hci: Consolidate spinlocks

[Adrian Hunter]

The MIPI I3C HCI driver currently uses separate spinlocks for different
contexts (PIO vs. DMA rings).  This split is unnecessary and complicates
upcoming fixes.  The driver does not support concurrent PIO and DMA
operation, and it only supports a single DMA ring, so a single lock is
sufficient for all paths.

Introduce a unified spinlock in struct i3c_hci, switch both PIO and DMA
code to use it, and remove the per-context locks.

No functional change is intended in this patch.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	New patch


 drivers/i3c/master/mipi-i3c-hci/core.c |  2 ++
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 14 ++++++--------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  1 +
 drivers/i3c/master/mipi-i3c-hci/pio.c  | 16 +++++++---------
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 4877a321edf9..faf5eae2409f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -926,6 +926,8 @@ static int i3c_hci_probe(struct platform_device *pdev)
 	if (!hci)
 		return -ENOMEM;
 
+	spin_lock_init(&hci->lock);
+
 	/*
 	 * Multi-bus instances share the same MMIO address range, but not
 	 * necessarily in separate contiguous sub-ranges. To avoid overlapping
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index ba451f026386..2442cedd5c2a 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -131,7 +131,6 @@ struct hci_rh_data {
 	unsigned int xfer_struct_sz, resp_struct_sz, ibi_status_sz, ibi_chunk_sz;
 	unsigned int done_ptr, ibi_chunk_ptr;
 	struct hci_xfer **src_xfers;
-	spinlock_t lock;
 	struct completion op_done;
 };
 
@@ -344,7 +343,6 @@ static int hci_dma_init(struct i3c_hci *hci)
 			goto err_out;
 		rh = &rings->headers[i];
 		rh->regs = hci->base_regs + offset;
-		spin_lock_init(&rh->lock);
 		init_completion(&rh->op_done);
 
 		rh->xfer_entries = XFER_RING_ENTRIES;
@@ -534,12 +532,12 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	}
 
 	/* take care to update the hardware enqueue pointer atomically */
-	spin_lock_irq(&rh->lock);
+	spin_lock_irq(&hci->lock);
 	op1_val = rh_reg_read(RING_OPERATION1);
 	op1_val &= ~RING_OP1_CR_ENQ_PTR;
 	op1_val |= FIELD_PREP(RING_OP1_CR_ENQ_PTR, enqueue_ptr);
 	rh_reg_write(RING_OPERATION1, op1_val);
-	spin_unlock_irq(&rh->lock);
+	spin_unlock_irq(&hci->lock);
 
 	return 0;
 }
@@ -637,12 +635,12 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 	}
 
 	/* take care to update the software dequeue pointer atomically */
-	spin_lock(&rh->lock);
+	spin_lock(&hci->lock);
 	op1_val = rh_reg_read(RING_OPERATION1);
 	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(&rh->lock);
+	spin_unlock(&hci->lock);
 }
 
 static int hci_dma_request_ibi(struct i3c_hci *hci, struct i3c_dev_desc *dev,
@@ -823,12 +821,12 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 
 done:
 	/* take care to update the ibi dequeue pointer atomically */
-	spin_lock(&rh->lock);
+	spin_lock(&hci->lock);
 	op1_val = rh_reg_read(RING_OPERATION1);
 	op1_val &= ~RING_OP1_IBI_DEQ_PTR;
 	op1_val |= FIELD_PREP(RING_OP1_IBI_DEQ_PTR, deq_ptr);
 	rh_reg_write(RING_OPERATION1, op1_val);
-	spin_unlock(&rh->lock);
+	spin_unlock(&hci->lock);
 
 	/* update the chunk pointer */
 	rh->ibi_chunk_ptr += ibi_chunks;
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 337b7ab1cb06..f1dd502c071f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -50,6 +50,7 @@ struct i3c_hci {
 	const struct hci_io_ops *io;
 	void *io_data;
 	const struct hci_cmd_ops *cmd;
+	spinlock_t lock;
 	atomic_t next_cmd_tid;
 	bool irq_inactive;
 	u32 caps;
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
index f8825ac81408..02866c2237fa 100644
--- a/drivers/i3c/master/mipi-i3c-hci/pio.c
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
@@ -123,7 +123,6 @@ struct hci_pio_ibi_data {
 };
 
 struct hci_pio_data {
-	spinlock_t lock;
 	struct hci_xfer *curr_xfer, *xfer_queue;
 	struct hci_xfer *curr_rx, *rx_queue;
 	struct hci_xfer *curr_tx, *tx_queue;
@@ -212,7 +211,6 @@ static int hci_pio_init(struct i3c_hci *hci)
 		return -ENOMEM;
 
 	hci->io_data = pio;
-	spin_lock_init(&pio->lock);
 
 	__hci_pio_init(hci, &size_val);
 
@@ -631,7 +629,7 @@ static int hci_pio_queue_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
 		xfer[i].data_left = xfer[i].data_len;
 	}
 
-	spin_lock_irq(&pio->lock);
+	spin_lock_irq(&hci->lock);
 	prev_queue_tail = pio->xfer_queue;
 	pio->xfer_queue = &xfer[n - 1];
 	if (pio->curr_xfer) {
@@ -645,7 +643,7 @@ static int hci_pio_queue_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
 			pio_reg_read(INTR_STATUS),
 			pio_reg_read(INTR_SIGNAL_ENABLE));
 	}
-	spin_unlock_irq(&pio->lock);
+	spin_unlock_irq(&hci->lock);
 	return 0;
 }
 
@@ -716,14 +714,14 @@ static bool hci_pio_dequeue_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int
 	struct hci_pio_data *pio = hci->io_data;
 	int ret;
 
-	spin_lock_irq(&pio->lock);
+	spin_lock_irq(&hci->lock);
 	dev_dbg(&hci->master.dev, "n=%d status=%#x/%#x", n,
 		pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
 	dev_dbg(&hci->master.dev, "main_status = %#x/%#x",
 		readl(hci->base_regs + 0x20), readl(hci->base_regs + 0x28));
 
 	ret = hci_pio_dequeue_xfer_common(hci, pio, xfer, n);
-	spin_unlock_irq(&pio->lock);
+	spin_unlock_irq(&hci->lock);
 	return ret;
 }
 
@@ -1016,13 +1014,13 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
 	struct hci_pio_data *pio = hci->io_data;
 	u32 status;
 
-	spin_lock(&pio->lock);
+	spin_lock(&hci->lock);
 	status = pio_reg_read(INTR_STATUS);
 	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
 		status, pio->enabled_irqs);
 	status &= pio->enabled_irqs | STAT_LATENCY_WARNINGS;
 	if (!status) {
-		spin_unlock(&pio->lock);
+		spin_unlock(&hci->lock);
 		return false;
 	}
 
@@ -1058,7 +1056,7 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
 	pio_reg_write(INTR_SIGNAL_ENABLE, pio->enabled_irqs);
 	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
 		pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
-	spin_unlock(&pio->lock);
+	spin_unlock(&hci->lock);
 	return true;
 }
 
-- 
2.51.0


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

[PATCH V2 05/14] i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers

[Adrian Hunter]

The I3C subsystem allows multiple transfers to be queued concurrently.
However, the MIPI I3C HCI driver's DMA enqueue path, hci_dma_queue_xfer(),
lacks sufficient serialization.

In particular, the allocation of the enqueue_ptr and its subsequent update
in the RING_OPERATION1 register, must be done atomically.  Otherwise, for
example, it would be possible for 2 transfers to be allocated the same
enqueue_ptr.

Extend the use of the existing spinlock for that purpose.  Keep a count of
the number of xfers enqueued so that it is easy to determine if the ring
has enough space.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	Refactor of hci_dma_queue_xfer() to do all DMA mapping first, is
	now a separate earlier patch: "i3c: mipi-i3c-hci: Factor out DMA
	mapping from queuing path"


 drivers/i3c/master/mipi-i3c-hci/dma.c | 32 +++++++++++++--------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 2442cedd5c2a..74b255ad6d0f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -129,7 +129,7 @@ struct hci_rh_data {
 	dma_addr_t xfer_dma, resp_dma, ibi_status_dma, ibi_data_dma;
 	unsigned int xfer_entries, ibi_status_entries, ibi_chunks_total;
 	unsigned int xfer_struct_sz, resp_struct_sz, ibi_status_sz, ibi_chunk_sz;
-	unsigned int done_ptr, ibi_chunk_ptr;
+	unsigned int done_ptr, ibi_chunk_ptr, xfer_space;
 	struct hci_xfer **src_xfers;
 	struct completion op_done;
 };
@@ -260,6 +260,7 @@ static void hci_dma_init_rh(struct i3c_hci *hci, struct hci_rh_data *rh, int i)
 
 	rh->done_ptr = 0;
 	rh->ibi_chunk_ptr = 0;
+	rh->xfer_space = rh->xfer_entries;
 }
 
 static void hci_dma_init_rings(struct i3c_hci *hci)
@@ -470,7 +471,7 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	struct hci_rings_data *rings = hci->io_data;
 	struct hci_rh_data *rh;
 	unsigned int i, ring, enqueue_ptr;
-	u32 op1_val, op2_val;
+	u32 op1_val;
 	int ret;
 
 	ret = hci_dma_map_xfer_list(hci, rings->sysdev, xfer_list, n);
@@ -481,6 +482,14 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	ring = 0;
 	rh = &rings->headers[ring];
 
+	spin_lock_irq(&hci->lock);
+
+	if (n > rh->xfer_space) {
+		spin_unlock_irq(&hci->lock);
+		hci_dma_unmap_xfer(hci, xfer_list, n);
+		return -EBUSY;
+	}
+
 	op1_val = rh_reg_read(RING_OPERATION1);
 	enqueue_ptr = FIELD_GET(RING_OP1_CR_ENQ_PTR, op1_val);
 	for (i = 0; i < n; i++) {
@@ -518,22 +527,10 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 		xfer->ring_entry = enqueue_ptr;
 
 		enqueue_ptr = (enqueue_ptr + 1) % rh->xfer_entries;
-
-		/*
-		 * We may update the hardware view of the enqueue pointer
-		 * only if we didn't reach its dequeue pointer.
-		 */
-		op2_val = rh_reg_read(RING_OPERATION2);
-		if (enqueue_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val)) {
-			/* the ring is full */
-			hci_dma_unmap_xfer(hci, xfer_list, n);
-			return -EBUSY;
-		}
 	}
 
-	/* take care to update the hardware enqueue pointer atomically */
-	spin_lock_irq(&hci->lock);
-	op1_val = rh_reg_read(RING_OPERATION1);
+	rh->xfer_space -= n;
+
 	op1_val &= ~RING_OP1_CR_ENQ_PTR;
 	op1_val |= FIELD_PREP(RING_OP1_CR_ENQ_PTR, enqueue_ptr);
 	rh_reg_write(RING_OPERATION1, op1_val);
@@ -601,6 +598,7 @@ 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 done_cnt = 0;
 	struct hci_xfer *xfer;
 
 	for (;;) {
@@ -632,10 +630,12 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 
 		done_ptr = (done_ptr + 1) % rh->xfer_entries;
 		rh->done_ptr = done_ptr;
+		done_cnt += 1;
 	}
 
 	/* take care to update the software dequeue pointer atomically */
 	spin_lock(&hci->lock);
+	rh->xfer_space += done_cnt;
 	op1_val = rh_reg_read(RING_OPERATION1);
 	op1_val &= ~RING_OP1_CR_SW_DEQ_PTR;
 	op1_val |= FIELD_PREP(RING_OP1_CR_SW_DEQ_PTR, done_ptr);
-- 
2.51.0


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

[PATCH V2 06/14] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Adrian Hunter]

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.

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


Changes in V2:

	Mutex now defined in struct i3c_hci instead of struct hci_rh_data


 drivers/i3c/master/mipi-i3c-hci/core.c | 1 +
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 2 ++
 drivers/i3c/master/mipi-i3c-hci/hci.h  | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index faf5eae2409f..061e84a5c412 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -927,6 +927,7 @@ static int i3c_hci_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	spin_lock_init(&hci->lock);
+	mutex_init(&hci->control_mutex);
 
 	/*
 	 * Multi-bus instances share the same MMIO address range, but not
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 74b255ad6d0f..f7d411e5e11f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -547,6 +547,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 	unsigned int i;
 	bool did_unqueue = false;
 
+	guard(mutex)(&hci->control_mutex);
+
 	/* stop the ring */
 	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
 	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index f1dd502c071f..9c63d80f7fc4 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -51,6 +51,7 @@ struct i3c_hci {
 	void *io_data;
 	const struct hci_cmd_ops *cmd;
 	spinlock_t lock;
+	struct mutex control_mutex;
 	atomic_t next_cmd_tid;
 	bool irq_inactive;
 	u32 caps;
-- 
2.51.0


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

[PATCH V2 07/14] i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and interrupt handler

[Adrian Hunter]

The DMA ring bookkeeping in the MIPI I3C HCI driver is updated from two
contexts: the DMA ring dequeue path (hci_dma_dequeue_xfer()) and the
interrupt handler (hci_dma_xfer_done()).  Both modify the ring's
in-flight transfer state - specifically rh->src_xfers[] and
xfer->ring_entry - but without any serialization.  This allows the two
paths to race, potentially leading to inconsistent ring state.

Serialize access to the shared ring state by extending the existing
spinlock to cover the DMA dequeue path and the entire interrupt handler.
Since the core IRQ handler now holds this lock, remove the per-function
locking from the PIO and DMA sub-handlers.

Additionally, clear the completed entry in rh->src_xfers[] in
hci_dma_xfer_done() so it cannot be matched or completed again.

Finally, place the ring restart sequence under the same lock in
hci_dma_dequeue_xfer() to avoid concurrent enqueue or completion
operations while the ring state is being modified.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	Now uses unified spinlock (as per earlier patch "i3c: mipi-i3c-hci:
	Consolidate spinlocks") and extends coverage to the entire interrupt
	handler


 drivers/i3c/master/mipi-i3c-hci/core.c |  2 ++
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 11 +++++------
 drivers/i3c/master/mipi-i3c-hci/pio.c  |  6 +-----
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 061e84a5c412..adf35b7fa498 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -567,6 +567,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 	irqreturn_t result = IRQ_NONE;
 	u32 val;
 
+	guard(spinlock)(&hci->lock);
+
 	/*
 	 * The IRQ can be shared, so the handler may be called when the IRQ is
 	 * due to a different device. That could happen when runtime suspended,
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index f7d411e5e11f..d7840ff69e59 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -560,6 +560,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 		WARN_ON(1);
 	}
 
+	spin_lock_irq(&hci->lock);
+
 	for (i = 0; i < n; i++) {
 		struct hci_xfer *xfer = xfer_list + i;
 		int idx = xfer->ring_entry;
@@ -593,6 +595,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 	/* restart the ring */
 	rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE);
 
+	spin_unlock_irq(&hci->lock);
+
 	return did_unqueue;
 }
 
@@ -618,6 +622,7 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 			dev_dbg(&hci->master.dev, "orphaned ring entry");
 		} else {
 			hci_dma_unmap_xfer(hci, xfer, 1);
+			rh->src_xfers[done_ptr] = NULL;
 			xfer->ring_entry = -1;
 			xfer->response = resp;
 			if (tid != xfer->cmd_tid) {
@@ -635,14 +640,11 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 		done_cnt += 1;
 	}
 
-	/* take care to update the software dequeue pointer atomically */
-	spin_lock(&hci->lock);
 	rh->xfer_space += done_cnt;
 	op1_val = rh_reg_read(RING_OPERATION1);
 	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,
@@ -822,13 +824,10 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 	i3c_master_queue_ibi(dev, slot);
 
 done:
-	/* take care to update the ibi dequeue pointer atomically */
-	spin_lock(&hci->lock);
 	op1_val = rh_reg_read(RING_OPERATION1);
 	op1_val &= ~RING_OP1_IBI_DEQ_PTR;
 	op1_val |= FIELD_PREP(RING_OP1_IBI_DEQ_PTR, deq_ptr);
 	rh_reg_write(RING_OPERATION1, op1_val);
-	spin_unlock(&hci->lock);
 
 	/* update the chunk pointer */
 	rh->ibi_chunk_ptr += ibi_chunks;
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
index 02866c2237fa..8f48a81e65ab 100644
--- a/drivers/i3c/master/mipi-i3c-hci/pio.c
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
@@ -1014,15 +1014,12 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
 	struct hci_pio_data *pio = hci->io_data;
 	u32 status;
 
-	spin_lock(&hci->lock);
 	status = pio_reg_read(INTR_STATUS);
 	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
 		status, pio->enabled_irqs);
 	status &= pio->enabled_irqs | STAT_LATENCY_WARNINGS;
-	if (!status) {
-		spin_unlock(&hci->lock);
+	if (!status)
 		return false;
-	}
 
 	if (status & STAT_IBI_STATUS_THLD)
 		hci_pio_process_ibi(hci, pio);
@@ -1056,7 +1053,6 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
 	pio_reg_write(INTR_SIGNAL_ENABLE, pio->enabled_irqs);
 	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
 		pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
-	spin_unlock(&hci->lock);
 	return true;
 }
 
-- 
2.51.0


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

[PATCH V2 08/14] i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue

[Adrian Hunter]

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, re-initializing 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>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Add Frank's Rev'd-by
	Mutex is now defined in struct i3c_hci


 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 d7840ff69e59..a3e8e01a35c9 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -546,18 +546,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)(&hci->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) {
+			/*
+			 * 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(&hci->lock);
-- 
2.51.0


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

[PATCH V2 09/14] i3c: mipi-i3c-hci: Add missing TID field to no-op command descriptor

[Adrian Hunter]

The internal control command descriptor used for no-op commands includes a
Transaction ID (TID) field, but the no-op command constructed in
hci_dma_dequeue_xfer() omitted it.  As a result, the hardware receives a
no-op descriptor without the expected TID.

This bug has gone unnoticed because the TID is currently not validated in
the no-op completion path, but the descriptor format requires it to be
present.

Add the missing TID field when generating a no-op descriptor so that its
layout matches the defined command structure.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Add Frank's Rev'd-by


 drivers/i3c/master/mipi-i3c-hci/cmd.h | 1 +
 drivers/i3c/master/mipi-i3c-hci/dma.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd.h b/drivers/i3c/master/mipi-i3c-hci/cmd.h
index 1d6dd2c5d01a..b1bf87daa651 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd.h
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd.h
@@ -17,6 +17,7 @@
 #define CMD_0_TOC			W0_BIT_(31)
 #define CMD_0_ROC			W0_BIT_(30)
 #define CMD_0_ATTR			W0_MASK(2, 0)
+#define CMD_0_TID			W0_MASK(6, 3)
 
 /*
  * Response Descriptor Structure
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index a3e8e01a35c9..239a195df7c7 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -582,7 +582,7 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 			u32 *ring_data = rh->xfer + rh->xfer_struct_sz * idx;
 
 			/* store no-op cmd descriptor */
-			*ring_data++ = FIELD_PREP(CMD_0_ATTR, 0x7);
+			*ring_data++ = FIELD_PREP(CMD_0_ATTR, 0x7) | FIELD_PREP(CMD_0_TID, xfer->cmd_tid);
 			*ring_data++ = 0;
 			if (hci->cmd == &mipi_i3c_hci_cmd_v2) {
 				*ring_data++ = 0;
-- 
2.51.0


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

[PATCH V2 10/14] i3c: mipi-i3c-hci: Restart DMA ring correctly after dequeue abort

[Adrian Hunter]

The DMA dequeue path attempts to restart the ring after aborting an
in-flight transfer, but the current sequence is incomplete. The controller
must be brought out of the aborted state and the ring control registers
must be programmed in the correct order: first clearing ABORT, then
re-enabling the ring and asserting RUN_STOP to resume operation.

Add the missing controller resume step and update the ring control writes
so that the ring is restarted using the proper sequence.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Add Frank's Rev'd-by


 drivers/i3c/master/mipi-i3c-hci/dma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 239a195df7c7..41b83f07fdab 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -600,7 +600,9 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 	}
 
 	/* restart the ring */
+	mipi_i3c_hci_resume(hci);
 	rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE);
+	rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE | RING_CTRL_RUN_STOP);
 
 	spin_unlock_irq(&hci->lock);
 
-- 
2.51.0


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

[PATCH V2 11/14] i3c: mipi-i3c-hci: Consolidate common xfer processing logic

[Adrian Hunter]

Several parts of the MIPI I3C HCI driver duplicate the same sequence for
queuing a transfer, waiting for completion, and handling timeouts. This
logic appears in five separate locations and will be affected by an
upcoming fix.

Refactor the repeated code into a new helper, i3c_hci_process_xfer(), and
store the timeout value in the hci_xfer structure so that callers do not
need to pass it as a separate parameter.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Add Frank's Rev'd-by


 drivers/i3c/master/mipi-i3c-hci/cmd_v1.c |  8 ++---
 drivers/i3c/master/mipi-i3c-hci/cmd_v2.c |  8 ++---
 drivers/i3c/master/mipi-i3c-hci/core.c   | 43 ++++++++++++++----------
 drivers/i3c/master/mipi-i3c-hci/hci.h    |  2 ++
 4 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
index 831a261f6c56..75d452d7f6af 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
@@ -331,12 +331,10 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
 			CMD_A0_ROC | CMD_A0_TOC;
 		xfer->cmd_desc[1] = 0;
 		xfer->completion = &done;
-		hci->io->queue_xfer(hci, xfer, 1);
-		if (!wait_for_completion_timeout(&done, HZ) &&
-		    hci->io->dequeue_xfer(hci, xfer, 1)) {
-			ret = -ETIMEDOUT;
+		xfer->timeout = HZ;
+		ret = i3c_hci_process_xfer(hci, xfer, 1);
+		if (ret)
 			break;
-		}
 		if ((RESP_STATUS(xfer->response) == RESP_ERR_ADDR_HEADER ||
 		     RESP_STATUS(xfer->response) == RESP_ERR_NACK) &&
 		    RESP_DATA_LENGTH(xfer->response) == 1) {
diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
index 054beee36da5..39eec26a363c 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
@@ -253,6 +253,7 @@ static int hci_cmd_v2_daa(struct i3c_hci *hci)
 	xfer[0].rnw = true;
 	xfer[0].cmd_desc[1] = CMD_A1_DATA_LENGTH(8);
 	xfer[1].completion = &done;
+	xfer[1].timeout = HZ;
 
 	for (;;) {
 		ret = i3c_master_get_free_addr(&hci->master, next_addr);
@@ -272,12 +273,9 @@ static int hci_cmd_v2_daa(struct i3c_hci *hci)
 			CMD_A0_ASSIGN_ADDRESS(next_addr) |
 			CMD_A0_ROC |
 			CMD_A0_TOC;
-		hci->io->queue_xfer(hci, xfer, 2);
-		if (!wait_for_completion_timeout(&done, HZ) &&
-		    hci->io->dequeue_xfer(hci, xfer, 2)) {
-			ret = -ETIMEDOUT;
+		ret = i3c_hci_process_xfer(hci, xfer, 2);
+		if (ret)
 			break;
-		}
 		if (RESP_STATUS(xfer[0].response) != RESP_SUCCESS) {
 			ret = 0;  /* no more devices to be assigned */
 			break;
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index adf35b7fa498..4a80671536f0 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -213,6 +213,25 @@ void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci)
 	reg_write(DCT_SECTION, FIELD_PREP(DCT_TABLE_INDEX, 0));
 }
 
+int i3c_hci_process_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
+{
+	struct completion *done = xfer[n - 1].completion;
+	unsigned long timeout = xfer[n - 1].timeout;
+	int ret;
+
+	ret = hci->io->queue_xfer(hci, xfer, n);
+	if (ret)
+		return ret;
+
+	if (!wait_for_completion_timeout(done, timeout) &&
+	    hci->io->dequeue_xfer(hci, xfer, n)) {
+		dev_err(&hci->master.dev, "%s: timeout error\n", __func__);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m,
 				struct i3c_ccc_cmd *ccc)
 {
@@ -253,18 +272,14 @@ static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m,
 	last = i - 1;
 	xfer[last].cmd_desc[0] |= CMD_0_TOC;
 	xfer[last].completion = &done;
+	xfer[last].timeout = HZ;
 
 	if (prefixed)
 		xfer--;
 
-	ret = hci->io->queue_xfer(hci, xfer, nxfers);
+	ret = i3c_hci_process_xfer(hci, xfer, nxfers);
 	if (ret)
 		goto out;
-	if (!wait_for_completion_timeout(&done, HZ) &&
-	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
-		ret = -ETIMEDOUT;
-		goto out;
-	}
 	for (i = prefixed; i < nxfers; i++) {
 		if (ccc->rnw)
 			ccc->dests[i - prefixed].payload.len =
@@ -335,15 +350,11 @@ static int i3c_hci_i3c_xfers(struct i3c_dev_desc *dev,
 	last = i - 1;
 	xfer[last].cmd_desc[0] |= CMD_0_TOC;
 	xfer[last].completion = &done;
+	xfer[last].timeout = HZ;
 
-	ret = hci->io->queue_xfer(hci, xfer, nxfers);
+	ret = i3c_hci_process_xfer(hci, xfer, nxfers);
 	if (ret)
 		goto out;
-	if (!wait_for_completion_timeout(&done, HZ) &&
-	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
-		ret = -ETIMEDOUT;
-		goto out;
-	}
 	for (i = 0; i < nxfers; i++) {
 		if (i3c_xfers[i].rnw)
 			i3c_xfers[i].len = RESP_DATA_LENGTH(xfer[i].response);
@@ -383,15 +394,11 @@ static int i3c_hci_i2c_xfers(struct i2c_dev_desc *dev,
 	last = i - 1;
 	xfer[last].cmd_desc[0] |= CMD_0_TOC;
 	xfer[last].completion = &done;
+	xfer[last].timeout = m->i2c.timeout;
 
-	ret = hci->io->queue_xfer(hci, xfer, nxfers);
+	ret = i3c_hci_process_xfer(hci, xfer, nxfers);
 	if (ret)
 		goto out;
-	if (!wait_for_completion_timeout(&done, m->i2c.timeout) &&
-	    hci->io->dequeue_xfer(hci, xfer, nxfers)) {
-		ret = -ETIMEDOUT;
-		goto out;
-	}
 	for (i = 0; i < nxfers; i++) {
 		if (RESP_STATUS(xfer[i].response) != RESP_SUCCESS) {
 			ret = -EIO;
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 9c63d80f7fc4..850016e3d4fe 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -89,6 +89,7 @@ struct hci_xfer {
 	unsigned int data_len;
 	unsigned int cmd_tid;
 	struct completion *completion;
+	unsigned long timeout;
 	union {
 		struct {
 			/* PIO specific */
@@ -156,5 +157,6 @@ void mipi_i3c_hci_dct_index_reset(struct i3c_hci *hci);
 void amd_set_od_pp_timing(struct i3c_hci *hci);
 void amd_set_resp_buf_thld(struct i3c_hci *hci);
 void i3c_hci_sync_irq_inactive(struct i3c_hci *hci);
+int i3c_hci_process_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
 
 #endif
-- 
2.51.0


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

[PATCH V2 12/14] i3c: mipi-i3c-hci: Fix race in DMA error handling in interrupt context

[Adrian Hunter]

The DMA ring halts whenever a transfer encounters an error. The interrupt
handler previously attempted to detect this situation and restart the ring
if a transfer completed at the same time. However, this restart logic runs
entirely in interrupt context and is inherently racy: it interacts with
other paths manipulating the ring state, and fully serializing it within
the interrupt handler is not practical.

Move this error-recovery logic out of the interrupt handler and into the
transfer-processing path (i3c_hci_process_xfer()), where serialization and
state management are already controlled. Introduce a new optional I/O-ops
callback, handle_error(), invoked when a completed transfer reports an
error. For DMA operation, the implementation simply calls the existing
dequeue function, which safely aborts and restarts the ring when needed.

This removes the fragile ring-restart logic from the interrupt handler and
centralizes error handling where proper sequencing can be ensured.

Fixes: ccdb2e0e3b00d ("i3c: mipi-i3c-hci: Add Intel specific quirk to ring resuming")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	None


 drivers/i3c/master/mipi-i3c-hci/core.c | 19 ++++++++++++----
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 31 +++++++-------------------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  1 +
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 4a80671536f0..b98952d12d7c 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -223,10 +223,21 @@ int i3c_hci_process_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
 	if (ret)
 		return ret;
 
-	if (!wait_for_completion_timeout(done, timeout) &&
-	    hci->io->dequeue_xfer(hci, xfer, n)) {
-		dev_err(&hci->master.dev, "%s: timeout error\n", __func__);
-		return -ETIMEDOUT;
+	if (!wait_for_completion_timeout(done, timeout)) {
+		if (hci->io->dequeue_xfer(hci, xfer, n)) {
+			dev_err(&hci->master.dev, "%s: timeout error\n", __func__);
+			return -ETIMEDOUT;
+		}
+		return 0;
+	}
+
+	if (hci->io->handle_error) {
+		bool error = false;
+
+		for (int i = 0; i < n && !error; i++)
+			error = RESP_STATUS(xfer[i].response);
+		if (error)
+			return hci->io->handle_error(hci, xfer, n);
 	}
 
 	return 0;
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 41b83f07fdab..e487ef52f6b4 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -609,6 +609,11 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 	return did_unqueue;
 }
 
+static int hci_dma_handle_error(struct i3c_hci *hci, struct hci_xfer *xfer_list, int n)
+{
+	return hci_dma_dequeue_xfer(hci, xfer_list, n) ? -EIO : 0;
+}
+
 static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 {
 	u32 op1_val, op2_val, resp, *ring_resp;
@@ -870,29 +875,8 @@ static bool hci_dma_irq_handler(struct i3c_hci *hci)
 			hci_dma_xfer_done(hci, rh);
 		if (status & INTR_RING_OP)
 			complete(&rh->op_done);
-
-		if (status & INTR_TRANSFER_ABORT) {
-			u32 ring_status;
-
-			dev_notice_ratelimited(&hci->master.dev,
-				"Ring %d: Transfer Aborted\n", i);
-			mipi_i3c_hci_resume(hci);
-			ring_status = rh_reg_read(RING_STATUS);
-			if (!(ring_status & RING_STATUS_RUNNING) &&
-			    status & INTR_TRANSFER_COMPLETION &&
-			    status & INTR_TRANSFER_ERR) {
-				/*
-				 * Ring stop followed by run is an Intel
-				 * specific required quirk after resuming the
-				 * halted controller. Do it only when the ring
-				 * is not in running state after a transfer
-				 * error.
-				 */
-				rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE);
-				rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE |
-							   RING_CTRL_RUN_STOP);
-			}
-		}
+		if (status & INTR_TRANSFER_ABORT)
+			dev_dbg(&hci->master.dev, "Ring %d: Transfer Aborted\n", i);
 		if (status & INTR_IBI_RING_FULL)
 			dev_err_ratelimited(&hci->master.dev,
 				"Ring %d: IBI Ring Full Condition\n", i);
@@ -908,6 +892,7 @@ const struct hci_io_ops mipi_i3c_hci_dma = {
 	.cleanup		= hci_dma_cleanup,
 	.queue_xfer		= hci_dma_queue_xfer,
 	.dequeue_xfer		= hci_dma_dequeue_xfer,
+	.handle_error		= hci_dma_handle_error,
 	.irq_handler		= hci_dma_irq_handler,
 	.request_ibi		= hci_dma_request_ibi,
 	.free_ibi		= hci_dma_free_ibi,
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 850016e3d4fe..9ac9d0e342f4 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -123,6 +123,7 @@ struct hci_io_ops {
 	bool (*irq_handler)(struct i3c_hci *hci);
 	int (*queue_xfer)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
 	bool (*dequeue_xfer)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
+	int (*handle_error)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
 	int (*request_ibi)(struct i3c_hci *hci, struct i3c_dev_desc *dev,
 			   const struct i3c_ibi_setup *req);
 	void (*free_ibi)(struct i3c_hci *hci, struct i3c_dev_desc *dev);
-- 
2.51.0


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

[PATCH V2 13/14] i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization

[Adrian Hunter]

Shared interrupts may fire unexpectedly, including during periods when the
controller is not yet fully initialized. Commit b9a15012a1452
("i3c: mipi-i3c-hci: Add optional Runtime PM support") addressed this issue
for the runtime-suspended state, but the same problem can also occur before
the bus is enabled for the first time.

Ensure the IRQ handler ignores interrupts until initialization is complete
by making consistent use of the existing irq_inactive flag.  The flag is
now set to false immediately before enabling the bus.

To guarantee correct ordering with respect to the IRQ handler, protect
all transitions of irq_inactive with the same spinlock used inside the
handler.

Fixes: b8460480f62e1 ("i3c: mipi-i3c-hci: Allow for Multi-Bus Instances")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	Place irq_inactive under the protection of the now unified
	spinlock, eliminating the need for explict memory barriers.
	Correctly position the update of irq_inactive in
	i3c_hci_sync_irq_inactive() to after synchronize_irq()


 drivers/i3c/master/mipi-i3c-hci/core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index b98952d12d7c..d5e91af7d569 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -152,6 +152,9 @@ static int i3c_hci_bus_init(struct i3c_master_controller *m)
 	if (hci->quirks & HCI_QUIRK_RESP_BUF_THLD)
 		amd_set_resp_buf_thld(hci);
 
+	scoped_guard(spinlock_irqsave, &hci->lock)
+		hci->irq_inactive = false;
+
 	/* Enable bus with Hot-Join disabled */
 	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
 	dev_dbg(&hci->master.dev, "HC_CONTROL = %#x", reg_read(HC_CONTROL));
@@ -184,8 +187,9 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
 	int irq = platform_get_irq(pdev, 0);
 
 	reg_write(INTR_SIGNAL_ENABLE, 0x0);
-	hci->irq_inactive = true;
 	synchronize_irq(irq);
+	scoped_guard(spinlock_irqsave, &hci->lock)
+		hci->irq_inactive = true;
 }
 
 static void i3c_hci_bus_cleanup(struct i3c_master_controller *m)
@@ -781,10 +785,11 @@ static int i3c_hci_runtime_resume(struct device *dev)
 
 	mipi_i3c_hci_dat_v1.restore(hci);
 
-	hci->irq_inactive = false;
-
 	hci->io->resume(hci);
 
+	scoped_guard(spinlock_irqsave, &hci->lock)
+		hci->irq_inactive = false;
+
 	/* Enable bus with Hot-Join disabled */
 	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
 
@@ -975,6 +980,8 @@ static int i3c_hci_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	hci->irq_inactive = true;
+
 	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(&pdev->dev, irq, i3c_hci_irq_handler,
 			       IRQF_SHARED, NULL, hci);
-- 
2.51.0


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

[PATCH V2 14/14] i3c: mipi-i3c-hci: Fallback to software reset when bus disable fails

[Adrian Hunter]

Disruption of the MIPI I3C HCI controller's internal state can cause
i3c_hci_bus_disable() to fail when attempting to shut down the bus.

In the code paths where bus disable is invoked - bus clean-up and runtime
suspend - the controller does not need to remain operational afterward, so
a full controller reset is a safe recovery mechanism.

Add a fallback to issue a software reset when disabling the bus fails.
This ensures the bus is reliably halted even if the controller's state
machine is stuck or unresponsive.

The fallback is used both during bus clean-up and in the runtime suspend
path.  In the latter case, ensure interrupts are quiesced after reset.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---


Changes in V2:

	Add Frank's Rev'd-by


 drivers/i3c/master/mipi-i3c-hci/core.c | 65 ++++++++++++++------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index d5e91af7d569..284f3ed7af8c 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -181,6 +181,34 @@ static int i3c_hci_bus_disable(struct i3c_hci *hci)
 	return ret;
 }
 
+static int i3c_hci_software_reset(struct i3c_hci *hci)
+{
+	u32 regval;
+	int ret;
+
+	/*
+	 * SOFT_RST must be clear before we write to it.
+	 * Then we must wait until it clears again.
+	 */
+	ret = readx_poll_timeout(reg_read, RESET_CONTROL, regval,
+				 !(regval & SOFT_RST), 0, 10 * USEC_PER_MSEC);
+	if (ret) {
+		dev_err(&hci->master.dev, "%s: Software reset stuck\n", __func__);
+		return ret;
+	}
+
+	reg_write(RESET_CONTROL, SOFT_RST);
+
+	ret = readx_poll_timeout(reg_read, RESET_CONTROL, regval,
+				 !(regval & SOFT_RST), 0, 10 * USEC_PER_MSEC);
+	if (ret) {
+		dev_err(&hci->master.dev, "%s: Software reset failed\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
 void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
 {
 	struct platform_device *pdev = to_platform_device(hci->master.dev.parent);
@@ -196,7 +224,8 @@ static void i3c_hci_bus_cleanup(struct i3c_master_controller *m)
 {
 	struct i3c_hci *hci = to_i3c_hci(m);
 
-	i3c_hci_bus_disable(hci);
+	if (i3c_hci_bus_disable(hci))
+		i3c_hci_software_reset(hci);
 	hci->io->cleanup(hci);
 }
 
@@ -626,34 +655,6 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 	return result;
 }
 
-static int i3c_hci_software_reset(struct i3c_hci *hci)
-{
-	u32 regval;
-	int ret;
-
-	/*
-	 * SOFT_RST must be clear before we write to it.
-	 * Then we must wait until it clears again.
-	 */
-	ret = readx_poll_timeout(reg_read, RESET_CONTROL, regval,
-				 !(regval & SOFT_RST), 0, 10 * USEC_PER_MSEC);
-	if (ret) {
-		dev_err(&hci->master.dev, "%s: Software reset stuck\n", __func__);
-		return ret;
-	}
-
-	reg_write(RESET_CONTROL, SOFT_RST);
-
-	ret = readx_poll_timeout(reg_read, RESET_CONTROL, regval,
-				 !(regval & SOFT_RST), 0, 10 * USEC_PER_MSEC);
-	if (ret) {
-		dev_err(&hci->master.dev, "%s: Software reset failed\n", __func__);
-		return ret;
-	}
-
-	return 0;
-}
-
 static inline bool is_version_1_1_or_newer(struct i3c_hci *hci)
 {
 	return hci->version_major > 1 || (hci->version_major == 1 && hci->version_minor > 0);
@@ -764,8 +765,12 @@ static int i3c_hci_runtime_suspend(struct device *dev)
 	int ret;
 
 	ret = i3c_hci_bus_disable(hci);
-	if (ret)
+	if (ret) {
+		/* Fall back to software reset to disable the bus */
+		ret = i3c_hci_software_reset(hci);
+		i3c_hci_sync_irq_inactive(hci);
 		return ret;
+	}
 
 	hci->io->suspend(hci);
 
-- 
2.51.0


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

Re: [PATCH V2 03/14] i3c: mipi-i3c-hci: Factor out DMA mapping from queuing path

[Frank Li]

On Wed, Mar 04, 2026 at 08:16:54PM +0200, Adrian Hunter wrote:
> Prepare for fixing a race in the DMA ring enqueue path when handling
> parallel transfers.  Move all DMA mapping out of hci_dma_queue_xfer()
> and into a new helper that performs the mapping up front.
>
> This refactoring allows the upcoming fix to extend the spinlock coverage
> around the enqueue operation without performing DMA mapping under the
> spinlock.
>
> No functional change is intended in this patch.
>
> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
>
> Changes in V2:
>
> 	New patch split out from "i3c: mipi-i3c-hci: Fix race in DMA ring
> 	enqueue for parallel xfers"
>
>
>  drivers/i3c/master/mipi-i3c-hci/dma.c | 49 ++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index b903a2da1fd1..ba451f026386 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -439,6 +439,33 @@ static void hci_dma_unmap_xfer(struct i3c_hci *hci,
>  	}
>  }
>
> +static struct i3c_dma *hci_dma_map_xfer(struct device *dev, struct hci_xfer *xfer)
> +{
> +	enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +	bool need_bounce = device_iommu_mapped(dev) && xfer->rnw && (xfer->data_len & 3);
> +
> +	return i3c_master_dma_map_single(dev, xfer->data, xfer->data_len, need_bounce, dir);
> +}
> +
> +static int hci_dma_map_xfer_list(struct i3c_hci *hci, struct device *dev,
> +				 struct hci_xfer *xfer_list, int n)
> +{
> +	for (int i = 0; i < n; i++) {
> +		struct hci_xfer *xfer = xfer_list + i;
> +
> +		if (!xfer->data)
> +			continue;
> +
> +		xfer->dma = hci_dma_map_xfer(dev, xfer);
> +		if (!xfer->dma) {
> +			hci_dma_unmap_xfer(hci, xfer_list, i);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  			      struct hci_xfer *xfer_list, int n)
>  {
> @@ -446,6 +473,11 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	struct hci_rh_data *rh;
>  	unsigned int i, ring, enqueue_ptr;
>  	u32 op1_val, op2_val;
> +	int ret;
> +
> +	ret = hci_dma_map_xfer_list(hci, rings->sysdev, xfer_list, n);
> +	if (ret)
> +		return ret;
>
>  	/* For now we only use ring 0 */
>  	ring = 0;
> @@ -456,9 +488,6 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	for (i = 0; i < n; i++) {
>  		struct hci_xfer *xfer = xfer_list + i;
>  		u32 *ring_data = rh->xfer + rh->xfer_struct_sz * enqueue_ptr;
> -		enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE :
> -							  DMA_TO_DEVICE;
> -		bool need_bounce;
>
>  		/* store cmd descriptor */
>  		*ring_data++ = xfer->cmd_desc[0];
> @@ -477,18 +506,6 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>
>  		/* 2nd and 3rd words of Data Buffer Descriptor Structure */
>  		if (xfer->data) {
> -			need_bounce = device_iommu_mapped(rings->sysdev) &&
> -				      xfer->rnw &&
> -				      xfer->data_len != ALIGN(xfer->data_len, 4);
> -			xfer->dma = i3c_master_dma_map_single(rings->sysdev,
> -							      xfer->data,
> -							      xfer->data_len,
> -							      need_bounce,
> -							      dir);
> -			if (!xfer->dma) {
> -				hci_dma_unmap_xfer(hci, xfer_list, i);
> -				return -ENOMEM;
> -			}
>  			*ring_data++ = lower_32_bits(xfer->dma->addr);
>  			*ring_data++ = upper_32_bits(xfer->dma->addr);
>  		} else {
> @@ -511,7 +528,7 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  		op2_val = rh_reg_read(RING_OPERATION2);
>  		if (enqueue_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val)) {
>  			/* the ring is full */
> -			hci_dma_unmap_xfer(hci, xfer_list, i + 1);
> +			hci_dma_unmap_xfer(hci, xfer_list, n);
>  			return -EBUSY;
>  		}
>  	}
> --
> 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

Re: [PATCH V2 04/14] i3c: mipi-i3c-hci: Consolidate spinlocks

[Frank Li]

On Wed, Mar 04, 2026 at 08:16:55PM +0200, Adrian Hunter wrote:
> The MIPI I3C HCI driver currently uses separate spinlocks for different
> contexts (PIO vs. DMA rings).  This split is unnecessary and complicates
> upcoming fixes.  The driver does not support concurrent PIO and DMA
> operation, and it only supports a single DMA ring, so a single lock is
> sufficient for all paths.
>
> Introduce a unified spinlock in struct i3c_hci, switch both PIO and DMA
> code to use it, and remove the per-context locks.
>
> No functional change is intended in this patch.
>
> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
>
> Changes in V2:
>
> 	New patch
>
>
>  drivers/i3c/master/mipi-i3c-hci/core.c |  2 ++
>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 14 ++++++--------
>  drivers/i3c/master/mipi-i3c-hci/hci.h  |  1 +
>  drivers/i3c/master/mipi-i3c-hci/pio.c  | 16 +++++++---------
>  4 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 4877a321edf9..faf5eae2409f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -926,6 +926,8 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  	if (!hci)
>  		return -ENOMEM;
>
> +	spin_lock_init(&hci->lock);
> +
>  	/*
>  	 * Multi-bus instances share the same MMIO address range, but not
>  	 * necessarily in separate contiguous sub-ranges. To avoid overlapping
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index ba451f026386..2442cedd5c2a 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -131,7 +131,6 @@ struct hci_rh_data {
>  	unsigned int xfer_struct_sz, resp_struct_sz, ibi_status_sz, ibi_chunk_sz;
>  	unsigned int done_ptr, ibi_chunk_ptr;
>  	struct hci_xfer **src_xfers;
> -	spinlock_t lock;
>  	struct completion op_done;
>  };
>
> @@ -344,7 +343,6 @@ static int hci_dma_init(struct i3c_hci *hci)
>  			goto err_out;
>  		rh = &rings->headers[i];
>  		rh->regs = hci->base_regs + offset;
> -		spin_lock_init(&rh->lock);
>  		init_completion(&rh->op_done);
>
>  		rh->xfer_entries = XFER_RING_ENTRIES;
> @@ -534,12 +532,12 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	}
>
>  	/* take care to update the hardware enqueue pointer atomically */
> -	spin_lock_irq(&rh->lock);
> +	spin_lock_irq(&hci->lock);
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	op1_val &= ~RING_OP1_CR_ENQ_PTR;
>  	op1_val |= FIELD_PREP(RING_OP1_CR_ENQ_PTR, enqueue_ptr);
>  	rh_reg_write(RING_OPERATION1, op1_val);
> -	spin_unlock_irq(&rh->lock);
> +	spin_unlock_irq(&hci->lock);
>
>  	return 0;
>  }
> @@ -637,12 +635,12 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
>  	}
>
>  	/* take care to update the software dequeue pointer atomically */
> -	spin_lock(&rh->lock);
> +	spin_lock(&hci->lock);
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	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(&rh->lock);
> +	spin_unlock(&hci->lock);
>  }
>
>  static int hci_dma_request_ibi(struct i3c_hci *hci, struct i3c_dev_desc *dev,
> @@ -823,12 +821,12 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>
>  done:
>  	/* take care to update the ibi dequeue pointer atomically */
> -	spin_lock(&rh->lock);
> +	spin_lock(&hci->lock);
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	op1_val &= ~RING_OP1_IBI_DEQ_PTR;
>  	op1_val |= FIELD_PREP(RING_OP1_IBI_DEQ_PTR, deq_ptr);
>  	rh_reg_write(RING_OPERATION1, op1_val);
> -	spin_unlock(&rh->lock);
> +	spin_unlock(&hci->lock);
>
>  	/* update the chunk pointer */
>  	rh->ibi_chunk_ptr += ibi_chunks;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> index 337b7ab1cb06..f1dd502c071f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -50,6 +50,7 @@ struct i3c_hci {
>  	const struct hci_io_ops *io;
>  	void *io_data;
>  	const struct hci_cmd_ops *cmd;
> +	spinlock_t lock;
>  	atomic_t next_cmd_tid;
>  	bool irq_inactive;
>  	u32 caps;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
> index f8825ac81408..02866c2237fa 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/pio.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
> @@ -123,7 +123,6 @@ struct hci_pio_ibi_data {
>  };
>
>  struct hci_pio_data {
> -	spinlock_t lock;
>  	struct hci_xfer *curr_xfer, *xfer_queue;
>  	struct hci_xfer *curr_rx, *rx_queue;
>  	struct hci_xfer *curr_tx, *tx_queue;
> @@ -212,7 +211,6 @@ static int hci_pio_init(struct i3c_hci *hci)
>  		return -ENOMEM;
>
>  	hci->io_data = pio;
> -	spin_lock_init(&pio->lock);
>
>  	__hci_pio_init(hci, &size_val);
>
> @@ -631,7 +629,7 @@ static int hci_pio_queue_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
>  		xfer[i].data_left = xfer[i].data_len;
>  	}
>
> -	spin_lock_irq(&pio->lock);
> +	spin_lock_irq(&hci->lock);
>  	prev_queue_tail = pio->xfer_queue;
>  	pio->xfer_queue = &xfer[n - 1];
>  	if (pio->curr_xfer) {
> @@ -645,7 +643,7 @@ static int hci_pio_queue_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
>  			pio_reg_read(INTR_STATUS),
>  			pio_reg_read(INTR_SIGNAL_ENABLE));
>  	}
> -	spin_unlock_irq(&pio->lock);
> +	spin_unlock_irq(&hci->lock);
>  	return 0;
>  }
>
> @@ -716,14 +714,14 @@ static bool hci_pio_dequeue_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int
>  	struct hci_pio_data *pio = hci->io_data;
>  	int ret;
>
> -	spin_lock_irq(&pio->lock);
> +	spin_lock_irq(&hci->lock);
>  	dev_dbg(&hci->master.dev, "n=%d status=%#x/%#x", n,
>  		pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
>  	dev_dbg(&hci->master.dev, "main_status = %#x/%#x",
>  		readl(hci->base_regs + 0x20), readl(hci->base_regs + 0x28));
>
>  	ret = hci_pio_dequeue_xfer_common(hci, pio, xfer, n);
> -	spin_unlock_irq(&pio->lock);
> +	spin_unlock_irq(&hci->lock);
>  	return ret;
>  }
>
> @@ -1016,13 +1014,13 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
>  	struct hci_pio_data *pio = hci->io_data;
>  	u32 status;
>
> -	spin_lock(&pio->lock);
> +	spin_lock(&hci->lock);
>  	status = pio_reg_read(INTR_STATUS);
>  	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
>  		status, pio->enabled_irqs);
>  	status &= pio->enabled_irqs | STAT_LATENCY_WARNINGS;
>  	if (!status) {
> -		spin_unlock(&pio->lock);
> +		spin_unlock(&hci->lock);
>  		return false;
>  	}
>
> @@ -1058,7 +1056,7 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
>  	pio_reg_write(INTR_SIGNAL_ENABLE, pio->enabled_irqs);
>  	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
>  		pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
> -	spin_unlock(&pio->lock);
> +	spin_unlock(&hci->lock);
>  	return true;
>  }
>
> --
> 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

Re: [PATCH V2 05/14] i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers

[Frank Li]

On Wed, Mar 04, 2026 at 08:16:56PM +0200, Adrian Hunter wrote:
> The I3C subsystem allows multiple transfers to be queued concurrently.
> However, the MIPI I3C HCI driver's DMA enqueue path, hci_dma_queue_xfer(),
> lacks sufficient serialization.
>
> In particular, the allocation of the enqueue_ptr and its subsequent update
> in the RING_OPERATION1 register, must be done atomically.  Otherwise, for
> example, it would be possible for 2 transfers to be allocated the same
> enqueue_ptr.
>
> Extend the use of the existing spinlock for that purpose.  Keep a count of
> the number of xfers enqueued so that it is easy to determine if the ring
> has enough space.
>
> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
>
> Changes in V2:
>
> 	Refactor of hci_dma_queue_xfer() to do all DMA mapping first, is
> 	now a separate earlier patch: "i3c: mipi-i3c-hci: Factor out DMA
> 	mapping from queuing path"
>
>
>  drivers/i3c/master/mipi-i3c-hci/dma.c | 32 +++++++++++++--------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index 2442cedd5c2a..74b255ad6d0f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -129,7 +129,7 @@ struct hci_rh_data {
>  	dma_addr_t xfer_dma, resp_dma, ibi_status_dma, ibi_data_dma;
>  	unsigned int xfer_entries, ibi_status_entries, ibi_chunks_total;
>  	unsigned int xfer_struct_sz, resp_struct_sz, ibi_status_sz, ibi_chunk_sz;
> -	unsigned int done_ptr, ibi_chunk_ptr;
> +	unsigned int done_ptr, ibi_chunk_ptr, xfer_space;
>  	struct hci_xfer **src_xfers;
>  	struct completion op_done;
>  };
> @@ -260,6 +260,7 @@ static void hci_dma_init_rh(struct i3c_hci *hci, struct hci_rh_data *rh, int i)
>
>  	rh->done_ptr = 0;
>  	rh->ibi_chunk_ptr = 0;
> +	rh->xfer_space = rh->xfer_entries;
>  }
>
>  static void hci_dma_init_rings(struct i3c_hci *hci)
> @@ -470,7 +471,7 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	struct hci_rings_data *rings = hci->io_data;
>  	struct hci_rh_data *rh;
>  	unsigned int i, ring, enqueue_ptr;
> -	u32 op1_val, op2_val;
> +	u32 op1_val;
>  	int ret;
>
>  	ret = hci_dma_map_xfer_list(hci, rings->sysdev, xfer_list, n);
> @@ -481,6 +482,14 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	ring = 0;
>  	rh = &rings->headers[ring];
>
> +	spin_lock_irq(&hci->lock);
> +
> +	if (n > rh->xfer_space) {
> +		spin_unlock_irq(&hci->lock);
> +		hci_dma_unmap_xfer(hci, xfer_list, n);
> +		return -EBUSY;
> +	}
> +
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	enqueue_ptr = FIELD_GET(RING_OP1_CR_ENQ_PTR, op1_val);
>  	for (i = 0; i < n; i++) {
> @@ -518,22 +527,10 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  		xfer->ring_entry = enqueue_ptr;
>
>  		enqueue_ptr = (enqueue_ptr + 1) % rh->xfer_entries;
> -
> -		/*
> -		 * We may update the hardware view of the enqueue pointer
> -		 * only if we didn't reach its dequeue pointer.
> -		 */
> -		op2_val = rh_reg_read(RING_OPERATION2);
> -		if (enqueue_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val)) {
> -			/* the ring is full */
> -			hci_dma_unmap_xfer(hci, xfer_list, n);
> -			return -EBUSY;
> -		}
>  	}
>
> -	/* take care to update the hardware enqueue pointer atomically */
> -	spin_lock_irq(&hci->lock);
> -	op1_val = rh_reg_read(RING_OPERATION1);
> +	rh->xfer_space -= n;
> +
>  	op1_val &= ~RING_OP1_CR_ENQ_PTR;
>  	op1_val |= FIELD_PREP(RING_OP1_CR_ENQ_PTR, enqueue_ptr);
>  	rh_reg_write(RING_OPERATION1, op1_val);
> @@ -601,6 +598,7 @@ 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 done_cnt = 0;
>  	struct hci_xfer *xfer;
>
>  	for (;;) {
> @@ -632,10 +630,12 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
>
>  		done_ptr = (done_ptr + 1) % rh->xfer_entries;
>  		rh->done_ptr = done_ptr;
> +		done_cnt += 1;
>  	}
>
>  	/* take care to update the software dequeue pointer atomically */
>  	spin_lock(&hci->lock);
> +	rh->xfer_space += done_cnt;
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	op1_val &= ~RING_OP1_CR_SW_DEQ_PTR;
>  	op1_val |= FIELD_PREP(RING_OP1_CR_SW_DEQ_PTR, done_ptr);
> --
> 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

Re: [PATCH V2 06/14] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Frank Li]

On Wed, Mar 04, 2026 at 08:16:57PM +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.
>
> 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>
> ---
>
>
> Changes in V2:
>
> 	Mutex now defined in struct i3c_hci instead of struct hci_rh_data
>
>
>  drivers/i3c/master/mipi-i3c-hci/core.c | 1 +
>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 2 ++
>  drivers/i3c/master/mipi-i3c-hci/hci.h  | 1 +
>  3 files changed, 4 insertions(+)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index faf5eae2409f..061e84a5c412 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -927,6 +927,7 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>
>  	spin_lock_init(&hci->lock);
> +	mutex_init(&hci->control_mutex);
>
>  	/*
>  	 * Multi-bus instances share the same MMIO address range, but not
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index 74b255ad6d0f..f7d411e5e11f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -547,6 +547,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  	unsigned int i;
>  	bool did_unqueue = false;
>
> +	guard(mutex)(&hci->control_mutex);
> +

if dequeue use mutex, equeue also need use the same mutex protect to make
sure no equeue happen during dequeue. Or you can guaratee equeue never
happen when dequeue.

Frank

>  	/* stop the ring */
>  	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
>  	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> index f1dd502c071f..9c63d80f7fc4 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -51,6 +51,7 @@ struct i3c_hci {
>  	void *io_data;
>  	const struct hci_cmd_ops *cmd;
>  	spinlock_t lock;
> +	struct mutex control_mutex;
>  	atomic_t next_cmd_tid;
>  	bool irq_inactive;
>  	u32 caps;
> --
> 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

Re: [PATCH V2 07/14] i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and interrupt handler

[Frank Li]

On Wed, Mar 04, 2026 at 08:16:58PM +0200, Adrian Hunter wrote:
> The DMA ring bookkeeping in the MIPI I3C HCI driver is updated from two
> contexts: the DMA ring dequeue path (hci_dma_dequeue_xfer()) and the
> interrupt handler (hci_dma_xfer_done()).  Both modify the ring's
> in-flight transfer state - specifically rh->src_xfers[] and
> xfer->ring_entry - but without any serialization.  This allows the two
> paths to race, potentially leading to inconsistent ring state.
>
> Serialize access to the shared ring state by extending the existing
> spinlock to cover the DMA dequeue path and the entire interrupt handler.
> Since the core IRQ handler now holds this lock, remove the per-function
> locking from the PIO and DMA sub-handlers.
>
> Additionally, clear the completed entry in rh->src_xfers[] in
> hci_dma_xfer_done() so it cannot be matched or completed again.
>
> Finally, place the ring restart sequence under the same lock in
> hci_dma_dequeue_xfer() to avoid concurrent enqueue or completion
> operations while the ring state is being modified.
>
> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
>
> Changes in V2:
>
> 	Now uses unified spinlock (as per earlier patch "i3c: mipi-i3c-hci:
> 	Consolidate spinlocks") and extends coverage to the entire interrupt
> 	handler
>
>
>  drivers/i3c/master/mipi-i3c-hci/core.c |  2 ++
>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 11 +++++------
>  drivers/i3c/master/mipi-i3c-hci/pio.c  |  6 +-----
>  3 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 061e84a5c412..adf35b7fa498 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -567,6 +567,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>  	irqreturn_t result = IRQ_NONE;
>  	u32 val;
>
> +	guard(spinlock)(&hci->lock);
> +
>  	/*
>  	 * The IRQ can be shared, so the handler may be called when the IRQ is
>  	 * due to a different device. That could happen when runtime suspended,
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index f7d411e5e11f..d7840ff69e59 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -560,6 +560,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  		WARN_ON(1);
>  	}
>
> +	spin_lock_irq(&hci->lock);
> +
>  	for (i = 0; i < n; i++) {
>  		struct hci_xfer *xfer = xfer_list + i;
>  		int idx = xfer->ring_entry;
> @@ -593,6 +595,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  	/* restart the ring */
>  	rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE);
>
> +	spin_unlock_irq(&hci->lock);
> +
>  	return did_unqueue;
>  }
>
> @@ -618,6 +622,7 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
>  			dev_dbg(&hci->master.dev, "orphaned ring entry");
>  		} else {
>  			hci_dma_unmap_xfer(hci, xfer, 1);
> +			rh->src_xfers[done_ptr] = NULL;
>  			xfer->ring_entry = -1;
>  			xfer->response = resp;
>  			if (tid != xfer->cmd_tid) {
> @@ -635,14 +640,11 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
>  		done_cnt += 1;
>  	}
>
> -	/* take care to update the software dequeue pointer atomically */
> -	spin_lock(&hci->lock);
>  	rh->xfer_space += done_cnt;
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	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,
> @@ -822,13 +824,10 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
>  	i3c_master_queue_ibi(dev, slot);
>
>  done:
> -	/* take care to update the ibi dequeue pointer atomically */
> -	spin_lock(&hci->lock);
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	op1_val &= ~RING_OP1_IBI_DEQ_PTR;
>  	op1_val |= FIELD_PREP(RING_OP1_IBI_DEQ_PTR, deq_ptr);
>  	rh_reg_write(RING_OPERATION1, op1_val);
> -	spin_unlock(&hci->lock);
>
>  	/* update the chunk pointer */
>  	rh->ibi_chunk_ptr += ibi_chunks;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
> index 02866c2237fa..8f48a81e65ab 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/pio.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
> @@ -1014,15 +1014,12 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
>  	struct hci_pio_data *pio = hci->io_data;
>  	u32 status;
>
> -	spin_lock(&hci->lock);
>  	status = pio_reg_read(INTR_STATUS);
>  	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
>  		status, pio->enabled_irqs);
>  	status &= pio->enabled_irqs | STAT_LATENCY_WARNINGS;
> -	if (!status) {
> -		spin_unlock(&hci->lock);
> +	if (!status)
>  		return false;
> -	}
>
>  	if (status & STAT_IBI_STATUS_THLD)
>  		hci_pio_process_ibi(hci, pio);
> @@ -1056,7 +1053,6 @@ static bool hci_pio_irq_handler(struct i3c_hci *hci)
>  	pio_reg_write(INTR_SIGNAL_ENABLE, pio->enabled_irqs);
>  	dev_dbg(&hci->master.dev, "PIO_INTR_STATUS %#x/%#x",
>  		pio_reg_read(INTR_STATUS), pio_reg_read(INTR_SIGNAL_ENABLE));
> -	spin_unlock(&hci->lock);
>  	return true;
>  }
>
> --
> 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

Re: [PATCH V2 12/14] i3c: mipi-i3c-hci: Fix race in DMA error handling in interrupt context

[Frank Li]

On Wed, Mar 04, 2026 at 08:17:03PM +0200, Adrian Hunter wrote:
> The DMA ring halts whenever a transfer encounters an error. The interrupt
> handler previously attempted to detect this situation and restart the ring
> if a transfer completed at the same time. However, this restart logic runs
> entirely in interrupt context and is inherently racy: it interacts with
> other paths manipulating the ring state, and fully serializing it within
> the interrupt handler is not practical.
>
> Move this error-recovery logic out of the interrupt handler and into the
> transfer-processing path (i3c_hci_process_xfer()), where serialization and
> state management are already controlled. Introduce a new optional I/O-ops
> callback, handle_error(), invoked when a completed transfer reports an
> error. For DMA operation, the implementation simply calls the existing
> dequeue function, which safely aborts and restarts the ring when needed.
>
> This removes the fragile ring-restart logic from the interrupt handler and
> centralizes error handling where proper sequencing can be ensured.
>
> Fixes: ccdb2e0e3b00d ("i3c: mipi-i3c-hci: Add Intel specific quirk to ring resuming")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
>
> Changes in V2:
>
> 	None
>
>
>  drivers/i3c/master/mipi-i3c-hci/core.c | 19 ++++++++++++----
>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 31 +++++++-------------------
>  drivers/i3c/master/mipi-i3c-hci/hci.h  |  1 +
>  3 files changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 4a80671536f0..b98952d12d7c 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -223,10 +223,21 @@ int i3c_hci_process_xfer(struct i3c_hci *hci, struct hci_xfer *xfer, int n)
>  	if (ret)
>  		return ret;
>
> -	if (!wait_for_completion_timeout(done, timeout) &&
> -	    hci->io->dequeue_xfer(hci, xfer, n)) {
> -		dev_err(&hci->master.dev, "%s: timeout error\n", __func__);
> -		return -ETIMEDOUT;
> +	if (!wait_for_completion_timeout(done, timeout)) {
> +		if (hci->io->dequeue_xfer(hci, xfer, n)) {
> +			dev_err(&hci->master.dev, "%s: timeout error\n", __func__);
> +			return -ETIMEDOUT;
> +		}
> +		return 0;
> +	}
> +
> +	if (hci->io->handle_error) {
> +		bool error = false;
> +
> +		for (int i = 0; i < n && !error; i++)
> +			error = RESP_STATUS(xfer[i].response);
> +		if (error)
> +			return hci->io->handle_error(hci, xfer, n);
>  	}
>
>  	return 0;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index 41b83f07fdab..e487ef52f6b4 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -609,6 +609,11 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  	return did_unqueue;
>  }
>
> +static int hci_dma_handle_error(struct i3c_hci *hci, struct hci_xfer *xfer_list, int n)
> +{
> +	return hci_dma_dequeue_xfer(hci, xfer_list, n) ? -EIO : 0;
> +}
> +
>  static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
>  {
>  	u32 op1_val, op2_val, resp, *ring_resp;
> @@ -870,29 +875,8 @@ static bool hci_dma_irq_handler(struct i3c_hci *hci)
>  			hci_dma_xfer_done(hci, rh);
>  		if (status & INTR_RING_OP)
>  			complete(&rh->op_done);
> -
> -		if (status & INTR_TRANSFER_ABORT) {
> -			u32 ring_status;
> -
> -			dev_notice_ratelimited(&hci->master.dev,
> -				"Ring %d: Transfer Aborted\n", i);
> -			mipi_i3c_hci_resume(hci);
> -			ring_status = rh_reg_read(RING_STATUS);
> -			if (!(ring_status & RING_STATUS_RUNNING) &&
> -			    status & INTR_TRANSFER_COMPLETION &&
> -			    status & INTR_TRANSFER_ERR) {
> -				/*
> -				 * Ring stop followed by run is an Intel
> -				 * specific required quirk after resuming the
> -				 * halted controller. Do it only when the ring
> -				 * is not in running state after a transfer
> -				 * error.
> -				 */
> -				rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE);
> -				rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE |
> -							   RING_CTRL_RUN_STOP);
> -			}
> -		}
> +		if (status & INTR_TRANSFER_ABORT)
> +			dev_dbg(&hci->master.dev, "Ring %d: Transfer Aborted\n", i);
>  		if (status & INTR_IBI_RING_FULL)
>  			dev_err_ratelimited(&hci->master.dev,
>  				"Ring %d: IBI Ring Full Condition\n", i);
> @@ -908,6 +892,7 @@ const struct hci_io_ops mipi_i3c_hci_dma = {
>  	.cleanup		= hci_dma_cleanup,
>  	.queue_xfer		= hci_dma_queue_xfer,
>  	.dequeue_xfer		= hci_dma_dequeue_xfer,
> +	.handle_error		= hci_dma_handle_error,
>  	.irq_handler		= hci_dma_irq_handler,
>  	.request_ibi		= hci_dma_request_ibi,
>  	.free_ibi		= hci_dma_free_ibi,
> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> index 850016e3d4fe..9ac9d0e342f4 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -123,6 +123,7 @@ struct hci_io_ops {
>  	bool (*irq_handler)(struct i3c_hci *hci);
>  	int (*queue_xfer)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
>  	bool (*dequeue_xfer)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
> +	int (*handle_error)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
>  	int (*request_ibi)(struct i3c_hci *hci, struct i3c_dev_desc *dev,
>  			   const struct i3c_ibi_setup *req);
>  	void (*free_ibi)(struct i3c_hci *hci, struct i3c_dev_desc *dev);
> --
> 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

Re: [PATCH V2 13/14] i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization

[Frank Li]

On Wed, Mar 04, 2026 at 08:17:04PM +0200, Adrian Hunter wrote:
> Shared interrupts may fire unexpectedly, including during periods when the
> controller is not yet fully initialized. Commit b9a15012a1452
> ("i3c: mipi-i3c-hci: Add optional Runtime PM support") addressed this issue
> for the runtime-suspended state, but the same problem can also occur before
> the bus is enabled for the first time.
>
> Ensure the IRQ handler ignores interrupts until initialization is complete
> by making consistent use of the existing irq_inactive flag.  The flag is
> now set to false immediately before enabling the bus.
>
> To guarantee correct ordering with respect to the IRQ handler, protect
> all transitions of irq_inactive with the same spinlock used inside the
> handler.
>
> Fixes: b8460480f62e1 ("i3c: mipi-i3c-hci: Allow for Multi-Bus Instances")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
>
> Changes in V2:
>
> 	Place irq_inactive under the protection of the now unified
> 	spinlock, eliminating the need for explict memory barriers.
> 	Correctly position the update of irq_inactive in
> 	i3c_hci_sync_irq_inactive() to after synchronize_irq()
>
>
>  drivers/i3c/master/mipi-i3c-hci/core.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index b98952d12d7c..d5e91af7d569 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -152,6 +152,9 @@ static int i3c_hci_bus_init(struct i3c_master_controller *m)
>  	if (hci->quirks & HCI_QUIRK_RESP_BUF_THLD)
>  		amd_set_resp_buf_thld(hci);
>
> +	scoped_guard(spinlock_irqsave, &hci->lock)
> +		hci->irq_inactive = false;
> +
>  	/* Enable bus with Hot-Join disabled */
>  	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
>  	dev_dbg(&hci->master.dev, "HC_CONTROL = %#x", reg_read(HC_CONTROL));
> @@ -184,8 +187,9 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
>  	int irq = platform_get_irq(pdev, 0);
>
>  	reg_write(INTR_SIGNAL_ENABLE, 0x0);
> -	hci->irq_inactive = true;
>  	synchronize_irq(irq);
> +	scoped_guard(spinlock_irqsave, &hci->lock)
> +		hci->irq_inactive = true;
>  }
>
>  static void i3c_hci_bus_cleanup(struct i3c_master_controller *m)
> @@ -781,10 +785,11 @@ static int i3c_hci_runtime_resume(struct device *dev)
>
>  	mipi_i3c_hci_dat_v1.restore(hci);
>
> -	hci->irq_inactive = false;
> -
>  	hci->io->resume(hci);
>
> +	scoped_guard(spinlock_irqsave, &hci->lock)
> +		hci->irq_inactive = false;
> +
>  	/* Enable bus with Hot-Join disabled */
>  	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
>
> @@ -975,6 +980,8 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>
> +	hci->irq_inactive = true;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	ret = devm_request_irq(&pdev->dev, irq, i3c_hci_irq_handler,
>  			       IRQF_SHARED, NULL, hci);
> --
> 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

Re: [PATCH V2 06/14] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Adrian Hunter]

On 04/03/2026 22:00, Frank Li wrote:
> On Wed, Mar 04, 2026 at 08:16:57PM +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.
>>
>> 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>
>> ---
>>
>>
>> Changes in V2:
>>
>> 	Mutex now defined in struct i3c_hci instead of struct hci_rh_data
>>
>>
>>  drivers/i3c/master/mipi-i3c-hci/core.c | 1 +
>>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 2 ++
>>  drivers/i3c/master/mipi-i3c-hci/hci.h  | 1 +
>>  3 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
>> index faf5eae2409f..061e84a5c412 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
>> @@ -927,6 +927,7 @@ static int i3c_hci_probe(struct platform_device *pdev)
>>  		return -ENOMEM;
>>
>>  	spin_lock_init(&hci->lock);
>> +	mutex_init(&hci->control_mutex);
>>
>>  	/*
>>  	 * Multi-bus instances share the same MMIO address range, but not
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> index 74b255ad6d0f..f7d411e5e11f 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -547,6 +547,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>>  	unsigned int i;
>>  	bool did_unqueue = false;
>>
>> +	guard(mutex)(&hci->control_mutex);
>> +
> 
> if dequeue use mutex, equeue also need use the same mutex protect to make
> sure no equeue happen during dequeue. Or you can guaratee equeue never
> happen when dequeue.

There are several other issues in this area, and they will be
addressed in later patch sets.

For this specific case, the enqueue path is already protected
by the existing spinlock, and we want to keep that path as lightweight
as possible.   Adding a mutex there would unnecessarily penalize the
common fast path, so it's not the right trade‑off.

Also, the ABORT handling is only relevant when a timeout occurs
while the ring is still running.  That situation only arises when the
timeout value is too aggressive or the controller ends up stuck - both
of which are rare.  Compared to ordinary error conditions (e.g. a NACK)
that stop the ring immediately, this race is lower priority.

This patch set is already quite large, so it makes sense to defer
broader coordination changes to a follow‑up series.


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

Re: [PATCH V2 06/14] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Frank Li]

On Thu, Mar 05, 2026 at 12:13:43PM +0200, Adrian Hunter wrote:
> On 04/03/2026 22:00, Frank Li wrote:
> > On Wed, Mar 04, 2026 at 08:16:57PM +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.
> >>
> >> 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>
> >> ---
> >>
> >>
> >> Changes in V2:
> >>
> >> 	Mutex now defined in struct i3c_hci instead of struct hci_rh_data
> >>
> >>
> >>  drivers/i3c/master/mipi-i3c-hci/core.c | 1 +
> >>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 2 ++
> >>  drivers/i3c/master/mipi-i3c-hci/hci.h  | 1 +
> >>  3 files changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> >> index faf5eae2409f..061e84a5c412 100644
> >> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> >> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> >> @@ -927,6 +927,7 @@ static int i3c_hci_probe(struct platform_device *pdev)
> >>  		return -ENOMEM;
> >>
> >>  	spin_lock_init(&hci->lock);
> >> +	mutex_init(&hci->control_mutex);
> >>
> >>  	/*
> >>  	 * Multi-bus instances share the same MMIO address range, but not
> >> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> index 74b255ad6d0f..f7d411e5e11f 100644
> >> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> @@ -547,6 +547,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
> >>  	unsigned int i;
> >>  	bool did_unqueue = false;
> >>
> >> +	guard(mutex)(&hci->control_mutex);
> >> +
> >
> > if dequeue use mutex, equeue also need use the same mutex protect to make
> > sure no equeue happen during dequeue. Or you can guaratee equeue never
> > happen when dequeue.
>
> There are several other issues in this area, and they will be
> addressed in later patch sets.
>
> For this specific case, the enqueue path is already protected
> by the existing spinlock, and we want to keep that path as lightweight
> as possible.   Adding a mutex there would unnecessarily penalize the
> common fast path, so it's not the right trade‑off.
>
> Also, the ABORT handling is only relevant when a timeout occurs
> while the ring is still running.  That situation only arises when the
> timeout value is too aggressive or the controller ends up stuck - both
> of which are rare.  Compared to ordinary error conditions (e.g. a NACK)
> that stop the ring immediately, this race is lower priority.
>
> This patch set is already quite large, so it makes sense to defer
> broader coordination changes to a follow‑up series.

Make sense.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
>
> --
> 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