Re: [PATCH] EDAC/altera: Warm Reset option for Stratix10 peripheral DBE

[Thor Thayer]

Hi James,

On 6/4/19 12:23 PM, James Morse wrote:
> Hi Thor,
>
> (CC: +Mark, Lorenzo and Sudeep for PSCI.
> How should SYSTEM_RESET2 be used for a vendor-specific reset?
>
> The original patch is:
> lore.kernel.org/r/1559594269-10077-1-git-send-email-thor.thayer@xxxxxxxxxxxxxxx
> )

Thank you for reviewing my patch and bringing in additional expertise.

> On 03/06/2019 21:37, thor.thayer@xxxxxxxxxxxxxxx wrote:
> > From: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>
> >
> > The Stratix10 peripheral FIFO memories can recover from double
> > bit errors with a warm reset instead of a cold reset.
> > Add the option of a warm reset for peripheral (USB, Ethernet)
> > memories.
> >
> > CPU memories such as SDRAM and OCRAM require a cold reset for
> > DBEs.
> > Filter on whether the error is a SDRAM/OCRAM or a peripheral
> > FIFO memory to determine which reset to use when the warm
> > reset option is configured.
>
> ... so you want to make different SMC calls on each CPU after panic()?
Yes. The registers needed for a warm reset need to be setup in EL3 
(U-Boot). As part of this, I needed the other CPUs (3 of the 4) to idle 
in WFI.

> > diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> > index 8816f74a22b4..179601f14b48 100644
> > --- a/drivers/edac/altera_edac.c
> > +++ b/drivers/edac/altera_edac.c
> > @@ -2036,6 +2036,19 @@ static const struct irq_domain_ops a10_eccmgr_ic_ops = {
> >   /* panic routine issues reboot on non-zero panic_timeout */
> >   extern int panic_timeout;
> >   
> > +#ifdef CONFIG_EDAC_ALTERA_ARM64_WARM_RESET
> > +/* EL3 SMC call to setup CPUs for warm reset */
> > +void panic_smp_self_stop(void)
> > +{
> > +	struct arm_smccc_res result;
> > +
> > +	__cpu_disable();
> > +	cpu_relax();
> > +	arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE, S10_WARM_RESET_WFI_FLAG,
> > +		      S10_WARM_RESET_WFI_FLAG, 0, 0, 0, 0, 0, &result);
> > +}
> > +#endif
>
> Oooer!
>
> panic_smp_self_stop() isn't for drivers to override: only the arch code.
> __cpu_disable() is only for the cpu-hotplug machinery. Nothing else should touch it.
>
> Isn't this thing only called if another CPU out there is panic()ing too?
OK. I wasn't aware of that intention. Thanks.

On an SError, all the CPUs except the first one end up calling 
panic_smp_self_stop().

All the CPUs are calling the panic routine which follows this path:
arm64_serror_panic() -> nmi_panic()

In nmi_panic()
	If this is the first CPU to reach nmi_panic(), call panic()
	otherwise, call nmi_panic_self_stop() -> panic_smp_self_stop()

panic_smp_self_stop() is a while(1) cpu_relax() which is a yield but I 
needed a WFI.

> I think one of the problems here is arm64 leaves secondary CPUs running after panic().
> This would be better fixed by using the appropriate cpu_ops[]->cpu_die() call in arm64's
> ipi_cpu_stop().
Yes. I will look into this.

> As for passing platform-specific options, PSCI[0] has a 'reset_type' for SYSTEM_RESET2,
> which looks suspiciously like what you want here. I'm not sure how its expected to be
> used... hopefully the PSCI maintainers can give us some pointers.
>
> (The existing support is commit 4302e381a870 ("firmware/psci: add support for SYSTEM_RESET2"))
>
>
> Is it possible for firmware to do both the cold/warm reset work when SYSTEM_RESET is
> called? This would mean you don't have to care here and there are fewer choices to be made
> overall.
> If not, is there anything left behind that can give it the hint? Like non-zero error
> counters for the USB/Ethernet devices?

Hmm. I'll look into this. I was trying to handle more in Linux so that I 
could do a best effort of printout notification of the error, etc for 
the logs (even though I know getting this far or printing out isn't 
guaranteed)

> > @@ -2067,14 +2080,28 @@ static int s10_edac_dberr_handler(struct notifier_block *this,
> >   			regmap_write(edac->ecc_mgr_map,
> >   				     S10_SYSMGR_UE_ADDR_OFST, err_addr);
> >   			edac_printk(KERN_ERR, EDAC_DEVICE,
> > -				    "EDAC: [Fatal DBE on %s @ 0x%08X]\n",
> > -				    ed->edac_dev_name, err_addr);
> > +				    "EDAC: [Fatal DBE on %s [CPU=%d] @ 0x%08X]\n",
> > +				    ed->edac_dev_name, raw_smp_processor_id(),
> > +				    err_addr);
> >   			break;
> >   		}
> >   		/* Notify the System through SMC. Reboot delay = 1 second */
> > +#ifdef CONFIG_EDAC_ALTERA_ARM64_WARM_RESET
> > +		/* Handle peripheral FIFO DBE as Warm Resets */
> > +		if (dberror & S10_COLD_RESET_MASK) {
>
>
> > +			panic_timeout = 1;
>
> Isn't this value supposed to be provided on the kernel commandline? Surely this prevents
> debug using the commandline option to increase the delay?
>
> (I see you already change it)

I see your point but I only override the value when I have an ECC DBE panic.
> > +			arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE, dberror, 0, 0, 0,
> > +				      0, 0, 0, &result);
> > +		} else {
> > +			arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE,
> > +				      S10_WARM_RESET_WFI_FLAG | dberror, 0, 0,
> > +				      0, 0, 0, 0, &result);
> > +		}
> > +#else
> >   		panic_timeout = 1;
> >   		arm_smccc_smc(INTEL_SIP_SMC_ECC_DBE, dberror, 0, 0, 0, 0,
> >   			      0, 0, &result);
> > +#endif
> >   	}
> >   
> >   	return NOTIFY_DONE;
>
> What do these SMC do? Are they equivalent to the PSCI CPU online/offline calls?
>
> panic() notifiers aren't robust as they can be skipped if kdump is loaded.
These are calling into the firmware to 1) notify the firmware of an 
upcoming cold reset in the case of cold reset or 2) call the warm reset 
handler in the firmware.

I missed that limitation of the panic() notifiers.

Thanks for the insightful comments!

Thor
> Thanks,
>
> James
>
>
> [0]
> https://static.docs.arm.com/den0022/d/Power_State_Coordination_Interface_PDD_v1_1_DEN0022D.pdf