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

On 8/3/2026 5:04 PM, Pranjal Shrivastava wrote:
> 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 <[email protected]>
>> Signed-off-by: Pratyush Brahma <[email protected]>
>> Signed-off-by: Prakash Gupta <[email protected]>
>> @@ -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 <[email protected]>
> 

Thanks Pranjal for review feedback. I have addressed the comments in v4.

Thanks,
Prakash
/com