Re: [PATCH v3] iommu/arm-smmu: Use pm_runtime in fault handlers

[Pranjal Shrivastava <praan@google.com>]

On Tue, Jun 30, 2026 at 02:32:46PM +0530, Prakash Gupta wrote:

Hi Prakash, 

On a second thought, I'd like to discuss one more thing

> Commit d4a44f0750bb ("iommu/arm-smmu: Invoke pm_runtime across the driver")
> enabled pm_runtime for the arm-smmu device. On systems where the SMMU
> sits in a power domain, all register accesses must be done while the
> device is runtime active to avoid unclocked register reads and
> potential NoC errors.
> 
> So far, this has not been an issue for most SMMU clients because
> stall-on-fault is enabled by default. While a translation fault is
> being handled, the SMMU stalls further translations for that context
> bank, so the fault handler would not race with a powered-down SMMU.
> 
> Adreno SMMU now disables stall-on-fault in the presence of fault
> storms to avoid saturating SMMU resources and hanging the GMU. With
> stall-on-fault disabled, the SMMU can generate faults while its power
> domain may no longer be enabled, which makes unclocked accesses to
> fault-status registers in the SMMU fault handlers possible.
> 
> Guard the context and global fault handlers with
> arm_smmu_rpm_get_if_active() and arm_smmu_rpm_put() so that all SMMU
> fault register accesses are done with the SMMU powered. If the SMMU is
> not runtime active, the fault can be safely ignored as
> arm_smmu_device_reset() clears fault registers on resume.
> 
> Additionally, disable fault reporting in arm_smmu_runtime_suspend()
> before powering down. pm_runtime_get_if_active() returns 0 during
> RPM_SUSPENDING, so without this, level-triggered fault interrupts would
> cause an interrupt storm while the device is being suspended.
> arm_smmu_device_reset() re-enables fault reporting on resume.
> 
> Fixes: b13044092c1e ("drm/msm: Temporarily disable stall-on-fault after a page fault")
> Co-developed-by: Pratyush Brahma <pratyush.brahma@oss.qualcomm.com>
> Signed-off-by: Pratyush Brahma <pratyush.brahma@oss.qualcomm.com>
> Signed-off-by: Prakash Gupta <prakash.gupta@oss.qualcomm.com>
> @@ -2306,6 +2329,25 @@ static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)

[...]

I believe, there is a small race condition in the suspend path that can
lead to unclocked register access crashes. (Something similar to what
I've attempted to handle in arm-smmu-v3 [1])

In arm_smmu_runtime_suspend(), we disable interrupt reporting in sCR0 and
SCTLR, and then immediately call clk_bulk_disable(). This disables the
interrupt generation but what about the interrupt handlers running
*during* suspend? I believe we could have this race:

CPU 0 (Suspend Context)                    CPU 1 (Interrupt/ISR Context)
-----------------------                    -----------------------------
                                        1. arm_smmu_context_fault() starts.
                                        2. rpm_get_if_active() returns 1.
                                           (Clocks are ON)
3. arm_smmu_runtime_suspend()
   - Clears CFIE/GFIE in registers
     (stops new IRQs from firing)
4. clk_bulk_disable()
   (Clocks are CUT)
                                        5. Attempts MMIO access (e.g, to
                                           clear CB_FSR or CB_RESUME).
                                           --> [CRASH] Unclocked MMIO access

I believe similar to arm-smmu-v3 [1], we must call synchronize_irq()
on context interrupts after disabling them in the SCTLR but before
we cut the clocks. This forces CPU 0's suspend thread to sleep and wait
for any active ISRs to safely drain while the SMMU still has clocks.

We can simply add this loop to arm_smmu_runtime_suspend():

>  static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>  {
>  	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +	int i;
> +	u32 reg;
> +
> +	/*
> +	 * Disable fault reporting before powering down to prevent unclocked
> +	 * register accesses in the fault handlers if an interrupt races with
> +	 * the suspend callback (e.g. device in RPM_SUSPENDING state).
> +	 * arm_smmu_device_reset() re-enables fault reporting on resume.
> +	 */
> +	reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> +	reg &= ~(ARM_SMMU_sCR0_GFRE | ARM_SMMU_sCR0_GFIE |
> +		 ARM_SMMU_sCR0_GCFGFRE | ARM_SMMU_sCR0_GCFGFIE);
> +	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_sCR0, reg);
> +
> +	for (i = 0; i < smmu->num_context_banks; i++) {
> +		reg = arm_smmu_cb_read(smmu, i, ARM_SMMU_CB_SCTLR);
> +		reg &= ~(ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE);
> +		arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_SCTLR, reg);
> +	}

    for (i = 0; i < smmu->num_context_irqs; i++)
        synchronize_irq(smmu->irqs[i]);

>  
>  	clk_bulk_disable(smmu->num_clks, smmu->clks);
>  

Since we're disabling those interrupts and fixing concurrency, 
this seems like the perfect opportunity to add the sync_irq too..

What do you think?

With that loop added:

Reviewed-by: Pranjal Shrivastava <praan@google.com>

Thanks,
Praan

[1] https://lore.kernel.org/all/20260728210928.1050849-10-praan@google.com/