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

[Adrian Hunter <adrian.hunter@intel.com>]

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