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

[PATCH 00/12] 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.


Adrian Hunter (12):
      i3c: mipi-i3c-hci: Use ETIMEDOUT instead of ETIME for timeout errors
      i3c: mipi-i3c-hci: Fix Hot-Join NACK
      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 the 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   | 141 +++++++++++++++++-----------
 drivers/i3c/master/mipi-i3c-hci/dma.c    | 152 +++++++++++++++++--------------
 drivers/i3c/master/mipi-i3c-hci/hci.h    |   3 +
 6 files changed, 185 insertions(+), 128 deletions(-)


Regards
Adrian

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

[PATCH 01/12] 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>
---
 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

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

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:38PM +0200, Adrian Hunter wrote:
> 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>

>  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

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

[PATCH 02/12] 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>
---
 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

Re: [PATCH 02/12] i3c: mipi-i3c-hci: Fix Hot-Join NACK

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:39PM +0200, Adrian Hunter wrote:
> 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>
> ---
>  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);

Can you check varible master->hotjoin? so needn't change it when anable HJ
in future?

Frank

>  	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

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

Re: [PATCH 02/12] i3c: mipi-i3c-hci: Fix Hot-Join NACK

[Adrian Hunter]

On 27/02/2026 18:03, Frank Li wrote:
> On Fri, Feb 27, 2026 at 04:11:39PM +0200, Adrian Hunter wrote:
>> 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>
>> ---
>>  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);
> 
> Can you check varible master->hotjoin? so needn't change it when anable HJ
> in future?

This is the simplest bug-fix.  There is no need to consider future
support because anything needed can be done then.

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


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

Re: [PATCH 02/12] i3c: mipi-i3c-hci: Fix Hot-Join NACK

[Frank Li]

On Mon, Mar 02, 2026 at 10:42:33AM +0200, Adrian Hunter wrote:
> On 27/02/2026 18:03, Frank Li wrote:
> > On Fri, Feb 27, 2026 at 04:11:39PM +0200, Adrian Hunter wrote:
> >> 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>
> >> ---
> >>  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);
> >
> > Can you check varible master->hotjoin? so needn't change it when anable HJ
> > in future?
>
> This is the simplest bug-fix.  There is no need to consider future
> support because anything needed can be done then.

It is also simple if check varible, only one line change

HC_CONTROL_BUS_ENABLE | (master->hotjoin ? 0 : HC_CONTROL_HOT_JOIN_CTRL)

Anyways, it is up to you.

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

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

[PATCH 03/12] 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.

Refactor hci_dma_queue_xfer() to do all DMA mapping first, so that DMA
mapping is not done under spinlock.

Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/i3c/master/mipi-i3c-hci/dma.c | 79 ++++++++++++++++-----------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index b903a2da1fd1..f60654fbe58e 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;
 	spinlock_t lock;
 	struct completion op_done;
@@ -261,6 +261,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)
@@ -439,26 +440,63 @@ 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)
 {
 	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);
+	if (ret)
+		return ret;
 
 	/* For now we only use ring 0 */
 	ring = 0;
 	rh = &rings->headers[ring];
 
+	spin_lock_irq(&rh->lock);
+
+	if (n > rh->xfer_space) {
+		spin_unlock_irq(&rh->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++) {
 		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 +515,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 {
@@ -503,22 +529,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, i + 1);
-			return -EBUSY;
-		}
 	}
 
-	/* take care to update the hardware enqueue pointer atomically */
-	spin_lock_irq(&rh->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);
@@ -586,6 +600,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 (;;) {
@@ -617,10 +632,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(&rh->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

Re: [PATCH 03/12] i3c: mipi-i3c-hci: Fix race in DMA ring enqueue for parallel xfers

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:40PM +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.
>
> Refactor hci_dma_queue_xfer() to do all DMA mapping first, so that DMA
> mapping is not done under spinlock.

Can you move this part code into prepare patch?  like add helper
hci_dma_map_xfer_list()

Frank
>
> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/i3c/master/mipi-i3c-hci/dma.c | 79 ++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index b903a2da1fd1..f60654fbe58e 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;
>  	spinlock_t lock;
>  	struct completion op_done;
> @@ -261,6 +261,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)
> @@ -439,26 +440,63 @@ 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)
>  {
>  	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);
> +	if (ret)
> +		return ret;
>
>  	/* For now we only use ring 0 */
>  	ring = 0;
>  	rh = &rings->headers[ring];
>
> +	spin_lock_irq(&rh->lock);
> +
> +	if (n > rh->xfer_space) {
> +		spin_unlock_irq(&rh->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++) {
>  		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 +515,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 {
> @@ -503,22 +529,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, i + 1);
> -			return -EBUSY;
> -		}
>  	}
>
> -	/* take care to update the hardware enqueue pointer atomically */
> -	spin_lock_irq(&rh->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);
> @@ -586,6 +600,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 (;;) {
> @@ -617,10 +632,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(&rh->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

[PATCH 04/12] 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>
---
 drivers/i3c/master/mipi-i3c-hci/dma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index f60654fbe58e..5a9af561e4cb 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -133,6 +133,7 @@ struct hci_rh_data {
 	struct hci_xfer **src_xfers;
 	spinlock_t lock;
 	struct completion op_done;
+	struct mutex control_mutex;
 };
 
 struct hci_rings_data {
@@ -347,6 +348,7 @@ static int hci_dma_init(struct i3c_hci *hci)
 		rh->regs = hci->base_regs + offset;
 		spin_lock_init(&rh->lock);
 		init_completion(&rh->op_done);
+		mutex_init(&rh->control_mutex);
 
 		rh->xfer_entries = XFER_RING_ENTRIES;
 
@@ -549,6 +551,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 	unsigned int i;
 	bool did_unqueue = false;
 
+	guard(mutex)(&rh->control_mutex);
+
 	/* stop the ring */
 	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
 	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
-- 
2.51.0


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

Re: [PATCH 04/12] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:41PM +0200, Adrian Hunter wrote:
> The HCI DMA dequeue path (hci_dma_dequeue_xfer()) may be invoked for
> multiple transfers that timeout around the same time.  However, the
> function is not serialized and can race with itself.
>
> When a timeout occurs, hci_dma_dequeue_xfer() stops the ring, processes
> incomplete transfers, and then restarts the ring.  If another timeout
> triggers a parallel call into the same function, the two instances may
> interfere with each other - stopping or restarting the ring at unexpected
> times.

how to sync with another hci_dma_queue_xfer()?

Frank

>
> Add a mutex so that hci_dma_dequeue_xfer() is serialized with respect to
> itself.
>
> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/i3c/master/mipi-i3c-hci/dma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index f60654fbe58e..5a9af561e4cb 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -133,6 +133,7 @@ struct hci_rh_data {
>  	struct hci_xfer **src_xfers;
>  	spinlock_t lock;
>  	struct completion op_done;
> +	struct mutex control_mutex;
>  };
>
>  struct hci_rings_data {
> @@ -347,6 +348,7 @@ static int hci_dma_init(struct i3c_hci *hci)
>  		rh->regs = hci->base_regs + offset;
>  		spin_lock_init(&rh->lock);
>  		init_completion(&rh->op_done);
> +		mutex_init(&rh->control_mutex);
>
>  		rh->xfer_entries = XFER_RING_ENTRIES;
>
> @@ -549,6 +551,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  	unsigned int i;
>  	bool did_unqueue = false;
>
> +	guard(mutex)(&rh->control_mutex);
> +
>  	/* stop the ring */
>  	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
>  	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
> --
> 2.51.0
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

Re: [PATCH 04/12] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Adrian Hunter]

On 27/02/2026 18:18, Frank Li wrote:
> On Fri, Feb 27, 2026 at 04:11:41PM +0200, Adrian Hunter wrote:
>> The HCI DMA dequeue path (hci_dma_dequeue_xfer()) may be invoked for
>> multiple transfers that timeout around the same time.  However, the
>> function is not serialized and can race with itself.
>>
>> When a timeout occurs, hci_dma_dequeue_xfer() stops the ring, processes
>> incomplete transfers, and then restarts the ring.  If another timeout
>> triggers a parallel call into the same function, the two instances may
>> interfere with each other - stopping or restarting the ring at unexpected
>> times.
> 
> how to sync with another hci_dma_queue_xfer()?

In theory, so long as the ring remains enabled, it should be possible
to enqueue transfers.

Nevertheless, the use of the ring spin lock is added in "i3c: mipi-i3c-hci:
Fix race between DMA ring dequeue and the interrupt handler".  The same spin
lock is used in hci_dma_queue_xfer().

> 
> Frank
> 
>>
>> Add a mutex so that hci_dma_dequeue_xfer() is serialized with respect to
>> itself.
>>
>> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/i3c/master/mipi-i3c-hci/dma.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> index f60654fbe58e..5a9af561e4cb 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -133,6 +133,7 @@ struct hci_rh_data {
>>  	struct hci_xfer **src_xfers;
>>  	spinlock_t lock;
>>  	struct completion op_done;
>> +	struct mutex control_mutex;
>>  };
>>
>>  struct hci_rings_data {
>> @@ -347,6 +348,7 @@ static int hci_dma_init(struct i3c_hci *hci)
>>  		rh->regs = hci->base_regs + offset;
>>  		spin_lock_init(&rh->lock);
>>  		init_completion(&rh->op_done);
>> +		mutex_init(&rh->control_mutex);
>>
>>  		rh->xfer_entries = XFER_RING_ENTRIES;
>>
>> @@ -549,6 +551,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>>  	unsigned int i;
>>  	bool did_unqueue = false;
>>
>> +	guard(mutex)(&rh->control_mutex);
>> +
>>  	/* stop the ring */
>>  	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
>>  	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
>> --
>> 2.51.0
>>
>>
>> --
>> linux-i3c mailing list
>> linux-i3c@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-i3c


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

Re: [PATCH 04/12] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Frank Li]

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

but not use spin lock in abort. So enqueue and
"rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);" still happen at the same
time.

Frank
>
> >
> > Frank
> >
> >>
> >> Add a mutex so that hci_dma_dequeue_xfer() is serialized with respect to
> >> itself.
> >>
> >> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> ---
> >>  drivers/i3c/master/mipi-i3c-hci/dma.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> index f60654fbe58e..5a9af561e4cb 100644
> >> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> @@ -133,6 +133,7 @@ struct hci_rh_data {
> >>  	struct hci_xfer **src_xfers;
> >>  	spinlock_t lock;
> >>  	struct completion op_done;
> >> +	struct mutex control_mutex;
> >>  };
> >>
> >>  struct hci_rings_data {
> >> @@ -347,6 +348,7 @@ static int hci_dma_init(struct i3c_hci *hci)
> >>  		rh->regs = hci->base_regs + offset;
> >>  		spin_lock_init(&rh->lock);
> >>  		init_completion(&rh->op_done);
> >> +		mutex_init(&rh->control_mutex);
> >>
> >>  		rh->xfer_entries = XFER_RING_ENTRIES;
> >>
> >> @@ -549,6 +551,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
> >>  	unsigned int i;
> >>  	bool did_unqueue = false;
> >>
> >> +	guard(mutex)(&rh->control_mutex);
> >> +
> >>  	/* stop the ring */
> >>  	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
> >>  	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
> >> --
> >> 2.51.0
> >>
> >>
> >> --
> >> linux-i3c mailing list
> >> linux-i3c@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-i3c
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

Re: [PATCH 04/12] i3c: mipi-i3c-hci: Fix race in DMA ring dequeue

[Adrian Hunter]

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

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

> 
> Frank
>>
>>>
>>> Frank
>>>
>>>>
>>>> Add a mutex so that hci_dma_dequeue_xfer() is serialized with respect to
>>>> itself.
>>>>
>>>> Fixes: 9ad9a52cce282 ("i3c/master: introduce the mipi-i3c-hci driver")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/i3c/master/mipi-i3c-hci/dma.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> index f60654fbe58e..5a9af561e4cb 100644
>>>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>>>> @@ -133,6 +133,7 @@ struct hci_rh_data {
>>>>  	struct hci_xfer **src_xfers;
>>>>  	spinlock_t lock;
>>>>  	struct completion op_done;
>>>> +	struct mutex control_mutex;
>>>>  };
>>>>
>>>>  struct hci_rings_data {
>>>> @@ -347,6 +348,7 @@ static int hci_dma_init(struct i3c_hci *hci)
>>>>  		rh->regs = hci->base_regs + offset;
>>>>  		spin_lock_init(&rh->lock);
>>>>  		init_completion(&rh->op_done);
>>>> +		mutex_init(&rh->control_mutex);
>>>>
>>>>  		rh->xfer_entries = XFER_RING_ENTRIES;
>>>>
>>>> @@ -549,6 +551,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>>>>  	unsigned int i;
>>>>  	bool did_unqueue = false;
>>>>
>>>> +	guard(mutex)(&rh->control_mutex);
>>>> +
>>>>  	/* stop the ring */
>>>>  	rh_reg_write(RING_CONTROL, RING_CTRL_ABORT);
>>>>  	if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) {
>>>> --
>>>> 2.51.0
>>>>
>>>>
>>>> --
>>>> linux-i3c mailing list
>>>> linux-i3c@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-i3c
>>
>>
>> --
>> linux-i3c mailing list
>> linux-i3c@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-i3c


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

[PATCH 05/12] i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and the 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 ring
spinlock to cover the dequeue path and the relevant parts of the
interrupt handler.  In the interrupt handler, clear the completed entry in
src_xfers[] 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>
---
 drivers/i3c/master/mipi-i3c-hci/dma.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 5a9af561e4cb..8d5f808e03ea 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -564,6 +564,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
 		WARN_ON(1);
 	}
 
+	spin_lock_irq(&rh->lock);
+
 	for (i = 0; i < n; i++) {
 		struct hci_xfer *xfer = xfer_list + i;
 		int idx = xfer->ring_entry;
@@ -597,6 +599,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(&rh->lock);
+
 	return did_unqueue;
 }
 
@@ -607,6 +611,8 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
 	unsigned int done_cnt = 0;
 	struct hci_xfer *xfer;
 
+	spin_lock(&rh->lock);
+
 	for (;;) {
 		op2_val = rh_reg_read(RING_OPERATION2);
 		if (done_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val))
@@ -622,6 +628,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) {
@@ -639,8 +646,6 @@ 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(&rh->lock);
 	rh->xfer_space += done_cnt;
 	op1_val = rh_reg_read(RING_OPERATION1);
 	op1_val &= ~RING_OP1_CR_SW_DEQ_PTR;
-- 
2.51.0


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

Re: [PATCH 05/12] i3c: mipi-i3c-hci: Fix race between DMA ring dequeue and the interrupt handler

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:42PM +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 ring
> spinlock to cover the dequeue path and the relevant parts of the
> interrupt handler.  In the interrupt handler, clear the completed entry in
> src_xfers[] 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>
>  drivers/i3c/master/mipi-i3c-hci/dma.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index 5a9af561e4cb..8d5f808e03ea 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -564,6 +564,8 @@ static bool hci_dma_dequeue_xfer(struct i3c_hci *hci,
>  		WARN_ON(1);
>  	}
>
> +	spin_lock_irq(&rh->lock);
> +
>  	for (i = 0; i < n; i++) {
>  		struct hci_xfer *xfer = xfer_list + i;
>  		int idx = xfer->ring_entry;
> @@ -597,6 +599,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(&rh->lock);
> +
>  	return did_unqueue;
>  }
>
> @@ -607,6 +611,8 @@ static void hci_dma_xfer_done(struct i3c_hci *hci, struct hci_rh_data *rh)
>  	unsigned int done_cnt = 0;
>  	struct hci_xfer *xfer;
>
> +	spin_lock(&rh->lock);
> +
>  	for (;;) {
>  		op2_val = rh_reg_read(RING_OPERATION2);
>  		if (done_ptr == FIELD_GET(RING_OP2_CR_DEQ_PTR, op2_val))
> @@ -622,6 +628,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) {
> @@ -639,8 +646,6 @@ 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(&rh->lock);
>  	rh->xfer_space += done_cnt;
>  	op1_val = rh_reg_read(RING_OPERATION1);
>  	op1_val &= ~RING_OP1_CR_SW_DEQ_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

[PATCH 06/12] 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, reinitializing 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>
---
 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 8d5f808e03ea..dff96b84479e 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -550,18 +550,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)(&rh->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(&rh->lock);
-- 
2.51.0


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

Re: [PATCH 06/12] i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:43PM +0200, Adrian Hunter wrote:
> 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, reinitializing 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>
> ---
>  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 8d5f808e03ea..dff96b84479e 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -550,18 +550,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)(&rh->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) {

Do you need readl_poll_timeout() here to make sure hardware actual stopped?

Frank

> +			/*
> +			 * 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(&rh->lock);
> --
> 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 06/12] i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue

[Adrian Hunter]

On 27/02/2026 18:25, Frank Li wrote:
> On Fri, Feb 27, 2026 at 04:11:43PM +0200, Adrian Hunter wrote:
>> 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, reinitializing 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>
>> ---
>>  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 8d5f808e03ea..dff96b84479e 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -550,18 +550,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)(&rh->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) {
> 
> Do you need readl_poll_timeout() here to make sure hardware actual stopped?

No the completion already waits for op_done

> 
> Frank
> 
>> +			/*
>> +			 * 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(&rh->lock);
>> --
>> 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 06/12] i3c: mipi-i3c-hci: Correct RING_CTRL_ABORT handling in DMA dequeue

[Frank Li]

On Mon, Mar 02, 2026 at 10:45:11AM +0200, Adrian Hunter wrote:
> On 27/02/2026 18:25, Frank Li wrote:
> > On Fri, Feb 27, 2026 at 04:11:43PM +0200, Adrian Hunter wrote:
> >> 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, reinitializing 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>
> >> ---
> >>  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 8d5f808e03ea..dff96b84479e 100644
> >> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> >> @@ -550,18 +550,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)(&rh->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) {
> >
> > Do you need readl_poll_timeout() here to make sure hardware actual stopped?
>
> No the completion already waits for op_done

Okay, suppose check return value of wait_for_completion_timeout().  read
register also work.

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

Frank
>
> >
> > Frank
> >
> >> +			/*
> >> +			 * 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(&rh->lock);
> >> --
> >> 2.51.0
> >>
> >>
> >> --
> >> linux-i3c mailing list
> >> linux-i3c@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-i3c
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[PATCH 07/12] 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>
---
 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 dff96b84479e..a11d0743fb80 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -586,7 +586,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

Re: [PATCH 07/12] i3c: mipi-i3c-hci: Add missing TID field to no-op command descriptor

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:44PM +0200, Adrian Hunter wrote:
> 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>
>  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 dff96b84479e..a11d0743fb80 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -586,7 +586,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

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

[PATCH 08/12] 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>
---
 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 a11d0743fb80..323c5ead82a5 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -604,7 +604,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(&rh->lock);
 
-- 
2.51.0


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

Re: [PATCH 08/12] i3c: mipi-i3c-hci: Restart DMA ring correctly after dequeue abort

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:45PM +0200, Adrian Hunter wrote:
> 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>
>  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 a11d0743fb80..323c5ead82a5 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -604,7 +604,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(&rh->lock);
>
> --
> 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

[PATCH 09/12] i3c: mipi-i3c-hci: Consolidate common xfer processing logic

[Adrian Hunter]

Several parts of the MIPI I3C HCI driver duplicate the same sequence for
queueing 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>
---
 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 4877a321edf9..9cfe12fb9a0e 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 337b7ab1cb06..6680b8640c5a 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -87,6 +87,7 @@ struct hci_xfer {
 	unsigned int data_len;
 	unsigned int cmd_tid;
 	struct completion *completion;
+	unsigned long timeout;
 	union {
 		struct {
 			/* PIO specific */
@@ -154,5 +155,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

Re: [PATCH 09/12] i3c: mipi-i3c-hci: Consolidate common xfer processing logic

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:46PM +0200, Adrian Hunter wrote:
> Several parts of the MIPI I3C HCI driver duplicate the same sequence for
> queueing 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>
>  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 4877a321edf9..9cfe12fb9a0e 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 337b7ab1cb06..6680b8640c5a 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -87,6 +87,7 @@ struct hci_xfer {
>  	unsigned int data_len;
>  	unsigned int cmd_tid;
>  	struct completion *completion;
> +	unsigned long timeout;
>  	union {
>  		struct {
>  			/* PIO specific */
> @@ -154,5 +155,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

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

[PATCH 10/12] 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>
---
 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 9cfe12fb9a0e..741f543aae68 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 323c5ead82a5..4951fd0a63ac 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -613,6 +613,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;
@@ -880,29 +885,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);
@@ -918,6 +902,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 6680b8640c5a..0f5e5bad9fb1 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -121,6 +121,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

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

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:47PM +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>
> ---
>  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 9cfe12fb9a0e..741f543aae68 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)) {

It is also an error, why not call handle_error() here.

Frank
> +			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 323c5ead82a5..4951fd0a63ac 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -613,6 +613,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;
> @@ -880,29 +885,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);
> @@ -918,6 +902,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 6680b8640c5a..0f5e5bad9fb1 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -121,6 +121,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 10/12] i3c: mipi-i3c-hci: Fix race in DMA error handling in interrupt context

[Adrian Hunter]

On 27/02/2026 18:40, Frank Li wrote:
> On Fri, Feb 27, 2026 at 04:11:47PM +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>
>> ---
>>  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 9cfe12fb9a0e..741f543aae68 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)) {
> 
> It is also an error, why not call handle_error() here.

It is different between PIO and DMA.  DMA has an extra need to
restart the DMA ring after an error.  PIO does not need that,
so for PIO ->handle_error() should do nothing, which is not the
same as ->dequeue_xfer().

> 
> Frank
>> +			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 323c5ead82a5..4951fd0a63ac 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
>> @@ -613,6 +613,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;
>> @@ -880,29 +885,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);
>> @@ -918,6 +902,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 6680b8640c5a..0f5e5bad9fb1 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
>> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
>> @@ -121,6 +121,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

[PATCH 11/12] 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 enforce correct ordering around transitions of irq_inactive, add
explicit memory barriers: place smp_wmb() before updates that clear or set
the flag, and add a corresponding smp_rmb() after testing it in the IRQ
handler.  While kernel I/O accessors already provide ordering guarantees in
practice, adding explicit barriers makes the intent unambiguous and correct
on all architectures.

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>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 741f543aae68..2909e3d35d8b 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -152,6 +152,10 @@ 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);
 
+	/* Ensure changes are visible to interrupt handler */
+	smp_wmb();
+	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,6 +188,8 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
 	int irq = platform_get_irq(pdev, 0);
 
 	reg_write(INTR_SIGNAL_ENABLE, 0x0);
+	/* Ensure changes are visible to interrupt handler */
+	smp_wmb();
 	hci->irq_inactive = true;
 	synchronize_irq(irq);
 }
@@ -592,6 +598,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 	 */
 	if (hci->irq_inactive)
 		return IRQ_NONE;
+	/* Ensure irq_inactive is read first */
+	smp_rmb();
 
 	val = reg_read(INTR_STATUS);
 	reg_write(INTR_STATUS, val);
@@ -779,10 +787,12 @@ static int i3c_hci_runtime_resume(struct device *dev)
 
 	mipi_i3c_hci_dat_v1.restore(hci);
 
-	hci->irq_inactive = false;
-
 	hci->io->resume(hci);
 
+	/* Ensure changes are visible to interrupt handler */
+	smp_wmb();
+	hci->irq_inactive = false;
+
 	/* Enable bus with Hot-Join disabled */
 	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
 
@@ -970,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

Re: [PATCH 11/12] i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:48PM +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 enforce correct ordering around transitions of irq_inactive, add
> explicit memory barriers: place smp_wmb() before updates that clear or set
> the flag, and add a corresponding smp_rmb() after testing it in the IRQ
> handler.  While kernel I/O accessors already provide ordering guarantees in
> practice, adding explicit barriers makes the intent unambiguous and correct
> on all architectures.
>
> 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>
> ---
>  drivers/i3c/master/mipi-i3c-hci/core.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 741f543aae68..2909e3d35d8b 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -152,6 +152,10 @@ 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);
>
> +	/* Ensure changes are visible to interrupt handler */
> +	smp_wmb();
> +	hci->irq_inactive = false;
> +

I think you should use atomic_t, which already consider barry.

Frank
>  	/* 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,6 +188,8 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
>  	int irq = platform_get_irq(pdev, 0);
>
>  	reg_write(INTR_SIGNAL_ENABLE, 0x0);
> +	/* Ensure changes are visible to interrupt handler */
> +	smp_wmb();
>  	hci->irq_inactive = true;
>  	synchronize_irq(irq);
>  }
> @@ -592,6 +598,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>  	 */
>  	if (hci->irq_inactive)
>  		return IRQ_NONE;
> +	/* Ensure irq_inactive is read first */
> +	smp_rmb();
>
>  	val = reg_read(INTR_STATUS);
>  	reg_write(INTR_STATUS, val);
> @@ -779,10 +787,12 @@ static int i3c_hci_runtime_resume(struct device *dev)
>
>  	mipi_i3c_hci_dat_v1.restore(hci);
>
> -	hci->irq_inactive = false;
> -
>  	hci->io->resume(hci);
>
> +	/* Ensure changes are visible to interrupt handler */
> +	smp_wmb();
> +	hci->irq_inactive = false;
> +
>  	/* Enable bus with Hot-Join disabled */
>  	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
>
> @@ -970,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 11/12] i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization

[Adrian Hunter]

On 27/02/2026 18:42, Frank Li wrote:
> On Fri, Feb 27, 2026 at 04:11:48PM +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 enforce correct ordering around transitions of irq_inactive, add
>> explicit memory barriers: place smp_wmb() before updates that clear or set
>> the flag, and add a corresponding smp_rmb() after testing it in the IRQ
>> handler.  While kernel I/O accessors already provide ordering guarantees in
>> practice, adding explicit barriers makes the intent unambiguous and correct
>> on all architectures.
>>
>> 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>
>> ---
>>  drivers/i3c/master/mipi-i3c-hci/core.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
>> index 741f543aae68..2909e3d35d8b 100644
>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
>> @@ -152,6 +152,10 @@ 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);
>>
>> +	/* Ensure changes are visible to interrupt handler */
>> +	smp_wmb();
>> +	hci->irq_inactive = false;
>> +
> 
> I think you should use atomic_t, which already consider barry.

No, atomic operations do not provided barriers by default.
This is very much about ordering.  Making sure memory changes
will also be seen by another CPU when it sees irq_inactive = false

> 
> Frank
>>  	/* 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,6 +188,8 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
>>  	int irq = platform_get_irq(pdev, 0);
>>
>>  	reg_write(INTR_SIGNAL_ENABLE, 0x0);
>> +	/* Ensure changes are visible to interrupt handler */
>> +	smp_wmb();
>>  	hci->irq_inactive = true;
>>  	synchronize_irq(irq);
>>  }
>> @@ -592,6 +598,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>>  	 */
>>  	if (hci->irq_inactive)
>>  		return IRQ_NONE;
>> +	/* Ensure irq_inactive is read first */
>> +	smp_rmb();
>>
>>  	val = reg_read(INTR_STATUS);
>>  	reg_write(INTR_STATUS, val);
>> @@ -779,10 +787,12 @@ static int i3c_hci_runtime_resume(struct device *dev)
>>
>>  	mipi_i3c_hci_dat_v1.restore(hci);
>>
>> -	hci->irq_inactive = false;
>> -
>>  	hci->io->resume(hci);
>>
>> +	/* Ensure changes are visible to interrupt handler */
>> +	smp_wmb();
>> +	hci->irq_inactive = false;
>> +
>>  	/* Enable bus with Hot-Join disabled */
>>  	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
>>
>> @@ -970,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 11/12] i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization

[Frank Li]

On Mon, Mar 02, 2026 at 10:43:53AM +0200, Adrian Hunter wrote:
> On 27/02/2026 18:42, Frank Li wrote:
> > On Fri, Feb 27, 2026 at 04:11:48PM +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 enforce correct ordering around transitions of irq_inactive, add
> >> explicit memory barriers: place smp_wmb() before updates that clear or set
> >> the flag, and add a corresponding smp_rmb() after testing it in the IRQ
> >> handler.  While kernel I/O accessors already provide ordering guarantees in
> >> practice, adding explicit barriers makes the intent unambiguous and correct
> >> on all architectures.
> >>
> >> 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>
> >> ---
> >>  drivers/i3c/master/mipi-i3c-hci/core.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> >> index 741f543aae68..2909e3d35d8b 100644
> >> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> >> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> >> @@ -152,6 +152,10 @@ 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);
> >>
> >> +	/* Ensure changes are visible to interrupt handler */
> >> +	smp_wmb();
> >> +	hci->irq_inactive = false;
> >> +
> >
> > I think you should use atomic_t, which already consider barry.
>
> No, atomic operations do not provided barriers by default.
> This is very much about ordering.  Making sure memory changes
> will also be seen by another CPU when it sees irq_inactive = false

Okay thanks. But I am not sure if it necesary because reg_read and reg_write
already include dma_wmb();

>
> >
> > Frank
> >>  	/* 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,6 +188,8 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
> >>  	int irq = platform_get_irq(pdev, 0);
> >>
> >>  	reg_write(INTR_SIGNAL_ENABLE, 0x0);
> >> +	/* Ensure changes are visible to interrupt handler */
> >> +	smp_wmb();

reg_write() include dma_wmb() before write register,  If you want to make
sure write 0 INTR_SIGNAL_ENABLE before hci->irq_inactive = true.

smp_wmb() is not enough, which just serilaze between two core's access.

need dma_wmb() to make write 0 INTR_SIGNAL_ENABLE happen before
hci->irq_inactive = true.

Frank

> >>  	hci->irq_inactive = true;
> >>  	synchronize_irq(irq);
> >>  }
> >> @@ -592,6 +598,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
> >>  	 */
> >>  	if (hci->irq_inactive)
> >>  		return IRQ_NONE;
> >> +	/* Ensure irq_inactive is read first */
> >> +	smp_rmb();
> >>
> >>  	val = reg_read(INTR_STATUS);
> >>  	reg_write(INTR_STATUS, val);
> >> @@ -779,10 +787,12 @@ static int i3c_hci_runtime_resume(struct device *dev)
> >>
> >>  	mipi_i3c_hci_dat_v1.restore(hci);
> >>
> >> -	hci->irq_inactive = false;
> >> -
> >>  	hci->io->resume(hci);
> >>
> >> +	/* Ensure changes are visible to interrupt handler */
> >> +	smp_wmb();
> >> +	hci->irq_inactive = false;
> >> +
> >>  	/* Enable bus with Hot-Join disabled */
> >>  	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
> >>
> >> @@ -970,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

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

Re: [PATCH 11/12] i3c: mipi-i3c-hci: Fix handling of shared IRQs during early initialization

[Adrian Hunter]

On 02/03/2026 21:18, Frank Li wrote:
> On Mon, Mar 02, 2026 at 10:43:53AM +0200, Adrian Hunter wrote:
>> On 27/02/2026 18:42, Frank Li wrote:
>>> On Fri, Feb 27, 2026 at 04:11:48PM +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 enforce correct ordering around transitions of irq_inactive, add
>>>> explicit memory barriers: place smp_wmb() before updates that clear or set
>>>> the flag, and add a corresponding smp_rmb() after testing it in the IRQ
>>>> handler.  While kernel I/O accessors already provide ordering guarantees in
>>>> practice, adding explicit barriers makes the intent unambiguous and correct
>>>> on all architectures.
>>>>
>>>> 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>
>>>> ---
>>>>  drivers/i3c/master/mipi-i3c-hci/core.c | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
>>>> index 741f543aae68..2909e3d35d8b 100644
>>>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
>>>> @@ -152,6 +152,10 @@ 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);
>>>>
>>>> +	/* Ensure changes are visible to interrupt handler */
>>>> +	smp_wmb();
>>>> +	hci->irq_inactive = false;
>>>> +
>>>
>>> I think you should use atomic_t, which already consider barry.
>>
>> No, atomic operations do not provided barriers by default.
>> This is very much about ordering.  Making sure memory changes
>> will also be seen by another CPU when it sees irq_inactive = false
> 
> Okay thanks. But I am not sure if it necesary because reg_read and reg_write
> already include dma_wmb();
> 
>>
>>>
>>> Frank
>>>>  	/* 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,6 +188,8 @@ void i3c_hci_sync_irq_inactive(struct i3c_hci *hci)
>>>>  	int irq = platform_get_irq(pdev, 0);
>>>>
>>>>  	reg_write(INTR_SIGNAL_ENABLE, 0x0);
>>>> +	/* Ensure changes are visible to interrupt handler */
>>>> +	smp_wmb();
> 
> reg_write() include dma_wmb() before write register,  If you want to make
> sure write 0 INTR_SIGNAL_ENABLE before hci->irq_inactive = true.
> 
> smp_wmb() is not enough, which just serilaze between two core's access.
> 
> need dma_wmb() to make write 0 INTR_SIGNAL_ENABLE happen before
> hci->irq_inactive = true.

DMA memory barriers are primarily for synchronizing CPU and device
via memory without registers.  In this driver (and most drivers), the
actual trigger (or doorbell) to notify the device that memory is ready
to be used, is a register write - in this case, to the enqueue pointer
in the OPERATIONS1 register.

This patch is primarily to cover the case where the interrupt handler
gets called before initialization is complete (because it is a shared IRQ).
In that case, the driver's data structures like hci->io_data may not be set
up, and the interrupt handler incurs a NULL-pointer dereference.  So
the ordering is about ordinary memory use, not DMA.

Nevertheless, in V2, the memory barriers are dropped in favour of a
spinlock.  That required unifying the different spin locks, but the
result is simpler to understand, I think.

> 
> Frank
> 
>>>>  	hci->irq_inactive = true;
>>>>  	synchronize_irq(irq);
>>>>  }
>>>> @@ -592,6 +598,8 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>>>>  	 */
>>>>  	if (hci->irq_inactive)
>>>>  		return IRQ_NONE;
>>>> +	/* Ensure irq_inactive is read first */
>>>> +	smp_rmb();
>>>>
>>>>  	val = reg_read(INTR_STATUS);
>>>>  	reg_write(INTR_STATUS, val);
>>>> @@ -779,10 +787,12 @@ static int i3c_hci_runtime_resume(struct device *dev)
>>>>
>>>>  	mipi_i3c_hci_dat_v1.restore(hci);
>>>>
>>>> -	hci->irq_inactive = false;
>>>> -
>>>>  	hci->io->resume(hci);
>>>>
>>>> +	/* Ensure changes are visible to interrupt handler */
>>>> +	smp_wmb();
>>>> +	hci->irq_inactive = false;
>>>> +
>>>>  	/* Enable bus with Hot-Join disabled */
>>>>  	reg_set(HC_CONTROL, HC_CONTROL_BUS_ENABLE | HC_CONTROL_HOT_JOIN_CTRL);
>>>>
>>>> @@ -970,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


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

[PATCH 12/12] 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>
---
 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 2909e3d35d8b..648ce87e3b7e 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -182,6 +182,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);
@@ -198,7 +226,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);
 }
 
@@ -628,34 +657,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);
@@ -766,8 +767,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 12/12] i3c: mipi-i3c-hci: Fallback to software reset when bus disable fails

[Frank Li]

On Fri, Feb 27, 2026 at 04:11:49PM +0200, Adrian Hunter wrote:
> 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>
>  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 2909e3d35d8b..648ce87e3b7e 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -182,6 +182,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);
> @@ -198,7 +226,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);
>  }
>
> @@ -628,34 +657,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);
> @@ -766,8 +767,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

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