Re: [PATCH RESEND 2/3] irqchip/apple-aic: Only access IPI sysregs when use_fast_ipi is true

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Aug 29 2024 at 19:02, Nick Chan wrote:
> Starting from the A11 (T8015) SoC, Apple introuced system registers for
> fast IPI and UNCORE PMC control. These sysregs do not exist on earlier
> A7-A10 SoCs and trying to access them results in an instant crash.
>
> Restrict sysreg access within the AIC driver to configurations where
> use_fast_ipi is true to allow AIC to function properly on A7-A10 SoCs.

use_fast_ipi is an implementation detail and does not mean anything
here. It's sufficient to say:

   Only access system registers on SoCs which provide them.

Hmm?

> While at it, remove the IPI-always-ack path on aic_handle_fiq.

It's not while at it. It's part of handling this correctly.

> If we are able to reach there, we are on an IPI-capable system and
> should be using one of the IPI-capable compatibles, anyway.

'we' can't reach that code ever.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog


> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> Signed-off-by: Nick Chan <towinchenmi@gmail.com>

This Signed-off-by chain is invalid. If Konrad authored the patch then
you need to have a 'From: Konrad ...' line at the top of the change log.

If you worked together on this then this needs a Co-developed-by
tag. See Documentation/process/...

>  
> -	if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
> -		if (static_branch_likely(&use_fast_ipi)) {
> -			aic_handle_ipi(regs);
> -		} else {
> -			pr_err_ratelimited("Fast IPI fired. Acking.\n");
> -			write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> -		}
> +	if (static_branch_likely(&use_fast_ipi) &&
> +	    (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING)) {
> +		aic_handle_ipi(regs);
>  	}
>  
>  	if (TIMER_FIRING(read_sysreg(cntp_ctl_el0)))
> @@ -574,8 +571,9 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>  					  AIC_FIQ_HWIRQ(irq));
>  	}
>  
> -	if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
> -			(read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
> +	if (static_branch_likely(&use_fast_ipi) &&
> +	    (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ) &&
> +	    (read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
>  		/* Same story with uncore PMCs */
>  		pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
>  		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> @@ -811,7 +809,8 @@ static int aic_init_cpu(unsigned int cpu)
>  	/* Mask all hard-wired per-CPU IRQ/FIQ sources */
>  
>  	/* Pending Fast IPI FIQs */
> -	write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> +	if (static_branch_likely(&use_fast_ipi))
> +		write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>  
>  	/* Timer FIQs */
>  	sysreg_clear_set(cntp_ctl_el0, 0, ARCH_TIMER_CTRL_IT_MASK);
> @@ -832,8 +831,9 @@ static int aic_init_cpu(unsigned int cpu)
>  			   FIELD_PREP(PMCR0_IMODE, PMCR0_IMODE_OFF));
>  
>  	/* Uncore PMC FIQ */
> -	sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> -			   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
> +	if (static_branch_likely(&use_fast_ipi))
> +		sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
> +				   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));

Please see the bracket rules in the tip maintainers doc.

Thanks,

        tglx