| Message ID | 20180927192711.26455-2-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM/PSCI: Fix THUMB2_KERNEL entry points | expand |
On Thu, Sep 27, 2018 at 12:27:09PM -0700, Florian Fainelli wrote: > When THUMB2_KERNEL is enabled, we would be failing to resume from an > idle or system suspend call where the reentry point is set to > cpu_resume() because that function is in Thumb2. Utilize > cpu_resume_arm() for ARM 32-bit kernels which takes care of the mode > switching for us. Looking at the PSCI spec, if bit[0] of the entry point address is set, the CPU should be placed into thumb state. So either there's a FW bug here, or perhaps we're stripping bit[0] when we do the __pa_symbol() dance. Which firmware have you seen this with? Thanks, Mark. > Fixes: 8b6f2499ac45 ("ARM: 8511/1: ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware") > Fixes: faf7ec4a92c0 ("drivers: firmware: psci: add system suspend support") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/firmware/psci.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > index c80ec1d03274..f8b154384483 100644 > --- a/drivers/firmware/psci.c > +++ b/drivers/firmware/psci.c > @@ -400,9 +400,14 @@ int psci_cpu_init_idle(unsigned int cpu) > static int psci_suspend_finisher(unsigned long index) > { > u32 *state = __this_cpu_read(psci_power_state); > + unsigned long reentry; > > - return psci_ops.cpu_suspend(state[index - 1], > - __pa_symbol(cpu_resume)); > +#ifdef CONFIG_ARM > + reentry = __pa_symbol(cpu_resume_arm); > +#else > + reentry = __pa_symbol(cpu_resume); > +#endif > + return psci_ops.cpu_suspend(state[index - 1], reentry); > } > > int psci_cpu_suspend_enter(unsigned long index) > @@ -437,8 +442,15 @@ CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops); > > static int psci_system_suspend(unsigned long unused) > { > + unsigned long reentry; > + > +#ifdef CONFIG_ARM > + reentry = __pa_symbol(cpu_resume_arm); > +#else > + reentry = __pa_symbol(cpu_resume); > +#endif > return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND), > - __pa_symbol(cpu_resume), 0, 0); > + reentry, 0, 0); > } > > static int psci_system_suspend_enter(suspend_state_t state) > -- > 2.17.1 >
On 09/28/2018 06:47 AM, Mark Rutland wrote: > On Thu, Sep 27, 2018 at 12:27:09PM -0700, Florian Fainelli wrote: >> When THUMB2_KERNEL is enabled, we would be failing to resume from an >> idle or system suspend call where the reentry point is set to >> cpu_resume() because that function is in Thumb2. Utilize >> cpu_resume_arm() for ARM 32-bit kernels which takes care of the mode >> switching for us. > > Looking at the PSCI spec, if bit[0] of the entry point address is set, > the CPU should be placed into thumb state. > > So either there's a FW bug here, or perhaps we're stripping bit[0] when > we do the __pa_symbol() dance. > > Which firmware have you seen this with? This is a custom implementation use with ARCH_BRCMSTB, the same way I managed to miss it during code review, I missed it again here, sorry about that and thanks for pointing me in the right direction.
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index c80ec1d03274..f8b154384483 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -400,9 +400,14 @@ int psci_cpu_init_idle(unsigned int cpu) static int psci_suspend_finisher(unsigned long index) { u32 *state = __this_cpu_read(psci_power_state); + unsigned long reentry; - return psci_ops.cpu_suspend(state[index - 1], - __pa_symbol(cpu_resume)); +#ifdef CONFIG_ARM + reentry = __pa_symbol(cpu_resume_arm); +#else + reentry = __pa_symbol(cpu_resume); +#endif + return psci_ops.cpu_suspend(state[index - 1], reentry); } int psci_cpu_suspend_enter(unsigned long index) @@ -437,8 +442,15 @@ CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops); static int psci_system_suspend(unsigned long unused) { + unsigned long reentry; + +#ifdef CONFIG_ARM + reentry = __pa_symbol(cpu_resume_arm); +#else + reentry = __pa_symbol(cpu_resume); +#endif return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND), - __pa_symbol(cpu_resume), 0, 0); + reentry, 0, 0); } static int psci_system_suspend_enter(suspend_state_t state)
When THUMB2_KERNEL is enabled, we would be failing to resume from an idle or system suspend call where the reentry point is set to cpu_resume() because that function is in Thumb2. Utilize cpu_resume_arm() for ARM 32-bit kernels which takes care of the mode switching for us. Fixes: 8b6f2499ac45 ("ARM: 8511/1: ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware") Fixes: faf7ec4a92c0 ("drivers: firmware: psci: add system suspend support") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/firmware/psci.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)