Re: [RFC PATCH 04/36] arm64: suspend: rely on daif helpers to handle PMR

[Jinjie Ruan <ruanjinjie@huawei.com>]

On 7/9/2026 8:13 PM, Vladimir Murzin wrote:
> From: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> 
> Commit 77345ef70445 ("arm64: suspend: Use cpuidle context helpers
> in cpu_suspend()") added cpuidle helpers to handle PMR manipulation
> and restoration to ensure that the CPU receives interrupts when suspended
> and pseudo-NMIs are enabled.
> 
> However, those helpers are called in between a pair of `local_daif_save()`
> and `local_daif_restore()`, which already configure the PMR as expected.
> Effectively, `arm_cpuidle_save_irq_context()` is a no-op here,
> even when using pseudo-NMIs, and `arm_cpuidle_restore_irq_context()`
> would not restore proper interrupt masking configuration early enough
> if there were unexpected changes during suspend or resume.
> (This can be observed with Trusted Firmware A (TF-A) at EL3 handling
> suspend through PSCI. Even though it should not be the case, TF-A can
> reset `ICC_PMR_EL1` during CPU_SUSPEND, thus resuming the kernel
> with an inconsistent priority mask value on hardware implementing
> more than the minimum number of priority levels, such as Morello.)
> 
> Thus : remove the cpuidle context helpers as they do not do anything,
> but keep the comment mentioning the need for interrupts to reach the CPU
> if we are using pseudo-NMIs.


local_daif_save()/local_daif_restore() has done all relevant PMR and
DAIF save and restore tasks.

LGTM
Reviewed-by: Jinjie Ruan <ruanjinjie@huawei.com>

> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
>  arch/arm64/kernel/suspend.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index eaaff94329cd..c41724a40b75 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -99,7 +99,6 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  	int ret = 0;
>  	unsigned long flags;
>  	struct sleep_stack_data state;
> -	struct arm_cpuidle_irq_context context;
>  
>  	/*
>  	 * Some portions of CPU state (e.g. PSTATE.{PAN,DIT}) are initialized
> @@ -121,6 +120,9 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  	 * Strictly speaking the trace_hardirqs_off() here is superfluous,
>  	 * hardirqs should be firmly off by now. This really ought to use
>  	 * something like raw_local_daif_save().
> +	 *
> +	 * This also unmasks interrupts in PMR in order to reliably
> +	 * resume if we're using pseudo-NMIs.
>  	 */
>  	flags = local_daif_save();
>  
> @@ -131,12 +133,6 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  	 */
>  	pause_graph_tracing();
>  
> -	/*
> -	 * Switch to using DAIF.IF instead of PMR in order to reliably
> -	 * resume if we're using pseudo-NMIs.
> -	 */
> -	arm_cpuidle_save_irq_context(&context);
> -
>  	ct_cpuidle_enter();
>  
>  	if (__cpu_suspend_enter(&state)) {
> @@ -159,8 +155,6 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  		__cpu_suspend_exit();
>  	}
>  
> -	arm_cpuidle_restore_irq_context(&context);
> -
>  	unpause_graph_tracing();
>  
>  	/*