Re: [PATCH] Fixed: ARM64 GIC ITS could not resume from suspend

[Marc Zyngier]

On Tue, 13 Apr 2021 07:40:10 +0100,
414777006@xxxxxx wrote:
> 
> From: Mengguang Peng <pengmengguang@xxxxxxxxxxxxxx>
> 
> - After ITS suspend, in the ATF(arm-trusted-firmware),
>   gicv3_rdistif_init_restore() just restore GICR_CTLR.Enable_LPIs bit
>   of boot cpu.
> 
> - In its_cpu_init_lpis() of kernel, gic_data_rdist()->lpi_enable
>   will block setting GICR_CTLR_ENABLE_LPIS bit of the other CPUs
>   when ITS resume after suspend.
> 
> Signed-off-by: Mengguang Peng <pengmengguang@xxxxxxxxxxxxxx>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index ed46e60..8167397 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4777,7 +4777,7 @@ static int its_save_disable(void)
>  static void its_restore_enable(void)
>  {
>  	struct its_node *its;
> -	int ret;
> +	int ret, cpu;
>  
>  	raw_spin_lock(&its_lock);
>  	list_for_each_entry(its, &its_nodes, entry) {
> @@ -4831,6 +4831,22 @@ static void its_restore_enable(void)
>  		    GITS_TYPER_HCC(gic_read_typer(base + GITS_TYPER)))
>  			its_cpu_init_collection(its);
>  	}
> +
> +	/*
> +	 * Enable LPIs: firmware just restore GICR_CTLR.Enable_LPIs
> +	 * of boot cpu, the other CPUs also should be restore.
> +	 */
> +	for_each_online_cpu(cpu) {
> +		void __iomem *rbase = gic_data_rdist_cpu(cpu)->rd_base;
> +		u32 val;
> +
> +		/* Enable LPIs */
> +		val = readl_relaxed(rbase + GICR_CTLR);
> +		if (val)
> +			continue;
> +		val |= GICR_CTLR_ENABLE_LPIS;
> +		writel_relaxed(val, rbase + GICR_CTLR);
> +	}
>  	raw_spin_unlock(&its_lock);
>  }

This looks completely flawed. If GICR_CTLR.Enable_LPIs is clear on
resume, how can you trust the rest of the bits in the register? How
can you trust *any* register in the redistributor?

And what makes you think it is valid or safe to blindly enable LPIs
without even checking whether they were enabled the first place?

I will reiterate my take on this: if the firmware messes with the RD
on suspend, please address the problem in the firmware so that the RDs
are correctly restored on each CPU.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.