[PATCH v5] ARM: vfp: Always save VFP state in vfp_pm_suspend

[PATCH v5] ARM: vfp: Always save VFP state in vfp_pm_suspend

[Daniel Drake]

From: Colin Cross <ccross@android.com>

vfp_pm_suspend should save the VFP state any time there is
a vfp_current_hw_state.  If it only saves when the VFP is enabled,
the state can get lost when, on a UP system:
   Thread 1 uses the VFP
   Context switch occurs to thread 2, VFP is disabled but the
      VFP context is not saved to allow lazy save and restore
   Thread 2 initiates suspend
   vfp_pm_suspend is called with the VFP disabled, but the
      context has not been saved.

Modify vfp_pm_suspend to save the VFP context whenever
vfp_current_hw_state is set.

Signed-off-by: Colin Cross <ccross@android.com>
Cc: Binghua Duan <binghua.duan@csr.com>
Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
Signed-off-by: Barry Song <21cnbao@gmail.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/vfp/vfpmodule.c |    4 ++++
 1 file changed, 4 insertions(+)

Fixes a real-life issue found on OLPC laptops:
http://lists.arm.linux.org.uk/lurker/message/20120715.044946.c30fff2b.en.html

v4->v5: last_VFP_context has been renamed to vfp_current_hw_state.

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 58696192..c86fc52 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -457,6 +457,10 @@ static int vfp_pm_suspend(void)
 
 		/* disable, just in case */
 		fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+	} else if (vfp_current_hw_state[ti->cpu]) {
+		fmxr(FPEXC, fpexc | FPEXC_EN);
+		vfp_save_state(vfp_current_hw_state[ti->cpu], fpexc);
+		fmxr(FPEXC, fpexc);
 	}
 
 	/* clear any information we had about last context state */
-- 
1.7.10.4

[PATCH v5] ARM: vfp: Always save VFP state in vfp_pm_suspend

[Barry Song]

2012/7/16 Daniel Drake <dsd@laptop.org>:
> From: Colin Cross <ccross@android.com>
>
> vfp_pm_suspend should save the VFP state any time there is
> a vfp_current_hw_state.  If it only saves when the VFP is enabled,
> the state can get lost when, on a UP system:
>    Thread 1 uses the VFP
>    Context switch occurs to thread 2, VFP is disabled but the
>       VFP context is not saved to allow lazy save and restore
>    Thread 2 initiates suspend
>    vfp_pm_suspend is called with the VFP disabled, but the
>       context has not been saved.
>
> Modify vfp_pm_suspend to save the VFP context whenever
> vfp_current_hw_state is set.
>
> Signed-off-by: Colin Cross <ccross@android.com>
> Cc: Binghua Duan <binghua.duan@csr.com>
> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> Signed-off-by: Barry Song <21cnbao@gmail.com>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm/vfp/vfpmodule.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> Fixes a real-life issue found on OLPC laptops:
> http://lists.arm.linux.org.uk/lurker/message/20120715.044946.c30fff2b.en.html

we also found this kind of issue too, sometimes suspend/resume will
fail with this bug.
so we refined this patch before. but i am really wondering why it has
not been committed yet.

>
> v4->v5: last_VFP_context has been renamed to vfp_current_hw_state.
>
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index 58696192..c86fc52 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -457,6 +457,10 @@ static int vfp_pm_suspend(void)
>
>                 /* disable, just in case */
>                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
> +       } else if (vfp_current_hw_state[ti->cpu]) {
> +               fmxr(FPEXC, fpexc | FPEXC_EN);
> +               vfp_save_state(vfp_current_hw_state[ti->cpu], fpexc);
> +               fmxr(FPEXC, fpexc);
>         }
>
>         /* clear any information we had about last context state */
> --
> 1.7.10.4
>

-barry

[PATCH v5] ARM: vfp: Always save VFP state in vfp_pm_suspend

[Will Deacon]

On Mon, Jul 16, 2012 at 02:40:06AM +0100, Barry Song wrote:
> 2012/7/16 Daniel Drake <dsd@laptop.org>:
> > From: Colin Cross <ccross@android.com>
> >
> > vfp_pm_suspend should save the VFP state any time there is
> > a vfp_current_hw_state.  If it only saves when the VFP is enabled,
> > the state can get lost when, on a UP system:
> >    Thread 1 uses the VFP
> >    Context switch occurs to thread 2, VFP is disabled but the
> >       VFP context is not saved to allow lazy save and restore
> >    Thread 2 initiates suspend
> >    vfp_pm_suspend is called with the VFP disabled, but the
> >       context has not been saved.
> >
> > Modify vfp_pm_suspend to save the VFP context whenever
> > vfp_current_hw_state is set.
> >
> > Signed-off-by: Colin Cross <ccross@android.com>
> > Cc: Binghua Duan <binghua.duan@csr.com>
> > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> > Signed-off-by: Barry Song <21cnbao@gmail.com>
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> we also found this kind of issue too, sometimes suspend/resume will
> fail with this bug.
> so we refined this patch before. but i am really wondering why it has
> not been committed yet.

At a guess, it hasn't been applied because nobody has submitted it to the
patch system.

> > diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> > index 58696192..c86fc52 100644
> > --- a/arch/arm/vfp/vfpmodule.c
> > +++ b/arch/arm/vfp/vfpmodule.c
> > @@ -457,6 +457,10 @@ static int vfp_pm_suspend(void)
> >
> >                 /* disable, just in case */
> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
> > +       } else if (vfp_current_hw_state[ti->cpu]) {
> > +               fmxr(FPEXC, fpexc | FPEXC_EN);
> > +               vfp_save_state(vfp_current_hw_state[ti->cpu], fpexc);
> > +               fmxr(FPEXC, fpexc);

Given that we don't do lazy saving on SMP systems, can we make this
conditional on !SMP?

Will

[PATCH v5] ARM: vfp: Always save VFP state in vfp_pm_suspend

[Daniel Drake]

On Mon, Jul 16, 2012 at 5:33 AM, Will Deacon <will.deacon@arm.com> wrote:
> Given that we don't do lazy saving on SMP systems, can we make this
> conditional on !SMP?

Colin pointed out in private mail that there are some other patches in
the android tree that fix this patch on SMP.

Colin, are you planning to roll them into one patch and posted a v6,
or send the SMP fixes separately?

Thanks
Daniel

[PATCH v5] ARM: vfp: Always save VFP state in vfp_pm_suspend

[Colin Cross]

On Mon, Jul 16, 2012 at 4:33 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Jul 16, 2012 at 02:40:06AM +0100, Barry Song wrote:
>> 2012/7/16 Daniel Drake <dsd@laptop.org>:
>> > From: Colin Cross <ccross@android.com>
>> >
>> > vfp_pm_suspend should save the VFP state any time there is
>> > a vfp_current_hw_state.  If it only saves when the VFP is enabled,
>> > the state can get lost when, on a UP system:
>> >    Thread 1 uses the VFP
>> >    Context switch occurs to thread 2, VFP is disabled but the
>> >       VFP context is not saved to allow lazy save and restore
>> >    Thread 2 initiates suspend
>> >    vfp_pm_suspend is called with the VFP disabled, but the
>> >       context has not been saved.
>> >
>> > Modify vfp_pm_suspend to save the VFP context whenever
>> > vfp_current_hw_state is set.
>> >
>> > Signed-off-by: Colin Cross <ccross@android.com>
>> > Cc: Binghua Duan <binghua.duan@csr.com>
>> > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
>> > Signed-off-by: Barry Song <21cnbao@gmail.com>
>> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>>
>> we also found this kind of issue too, sometimes suspend/resume will
>> fail with this bug.
>> so we refined this patch before. but i am really wondering why it has
>> not been committed yet.
>
> At a guess, it hasn't been applied because nobody has submitted it to the
> patch system.
>
>> > diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
>> > index 58696192..c86fc52 100644
>> > --- a/arch/arm/vfp/vfpmodule.c
>> > +++ b/arch/arm/vfp/vfpmodule.c
>> > @@ -457,6 +457,10 @@ static int vfp_pm_suspend(void)
>> >
>> >                 /* disable, just in case */
>> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>> > +       } else if (vfp_current_hw_state[ti->cpu]) {
>> > +               fmxr(FPEXC, fpexc | FPEXC_EN);
>> > +               vfp_save_state(vfp_current_hw_state[ti->cpu], fpexc);
>> > +               fmxr(FPEXC, fpexc);
>
> Given that we don't do lazy saving on SMP systems, can we make this
> conditional on !SMP?

Ido Yariv pointed out that this is actually unsafe on SMP systems,
because vfp_current_hw_state can be a dangling pointer if a task exits
on another cpu.  I'll squash his fix into this one (#ifdef CONFIG_SMP
around the contents of the new if clause) and post v6.