[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[Barry Song]

From: Rongjun Ying <rongjun.ying@csr.com>

Current vfp pm suspend entry only saves the vfp state of running proceed if it is using vfp. If current proceed doesn't use vfp,
the state of last process will be lost after resume. In pressure tests, we can see old vfp processes crash after resume.

In order that schedule can be faster, scheduler doesn't save vfp state if we schedule from proceeds using vfp to proceeds which
don't use vfp. If system suspend happens just at proceeds which don't use vfp, we have no any chance to save old vfp state.

Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
Cc: Binghua Duan <binghua.duan@csr.com>
Signed-off-by: Barry Song <21cnbao@gmail.com>
---
 arch/arm/vfp/vfpmodule.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bbf3da0..a980967 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -384,6 +384,7 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
 {
 	struct thread_info *ti = current_thread_info();
 	u32 fpexc = fmrx(FPEXC);
+	unsigned int cpu = get_cpu();
 
 	/* if vfp is on, then save state for resumption */
 	if (fpexc & FPEXC_EN) {
@@ -392,8 +393,16 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
 
 		/* disable, just in case */
 		fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+	} else {
+		if (last_VFP_context[cpu]) {
+			fmxr(FPEXC, fpexc | FPEXC_EN);
+			vfp_save_state(last_VFP_context[cpu], fpexc);
+			fmxr(FPEXC, fpexc);
+		}
 	}
 
+	put_cpu();
+
 	/* clear any information we had about last context state */
 	memset(last_VFP_context, 0, sizeof(last_VFP_context));
 
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[Catalin Marinas]

On 27 April 2011 07:30, Barry Song <bs14@csr.com> wrote:
> From: Rongjun Ying <rongjun.ying@csr.com>
>
> Current vfp pm suspend entry only saves the vfp state of running proceed if it is using vfp. If current proceed doesn't use vfp,
> the state of last process will be lost after resume. In pressure tests, we can see old vfp processes crash after resume.
>
> In order that schedule can be faster, scheduler doesn't save vfp state if we schedule from proceeds using vfp to proceeds which
> don't use vfp. If system suspend happens just at proceeds which don't use vfp, we have no any chance to save old vfp state.
>
> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> Cc: Binghua Duan <binghua.duan@csr.com>
> Signed-off-by: Barry Song <21cnbao@gmail.com>

There was a similar patch some time ago by Colin Cross. I don't know
what happened to it but please have a look at that discussion first:

http://thread.gmane.org/gmane.linux.kernel/1099558

-- 
Catalin

[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[Barry Song]

Hi Catalin,
Thanks.
2011/4/29 Catalin Marinas <catalin.marinas@arm.com>:
> On 27 April 2011 07:30, Barry Song <bs14@csr.com> wrote:
>> From: Rongjun Ying <rongjun.ying@csr.com>
>>
>> Current vfp pm suspend entry only saves the vfp state of running proceed if it is using vfp. If current proceed doesn't use vfp,
>> the state of last process will be lost after resume. In pressure tests, we can see old vfp processes crash after resume.
>>
>> In order that schedule can be faster, scheduler doesn't save vfp state if we schedule from proceeds using vfp to proceeds which
>> don't use vfp. If system suspend happens just at proceeds which don't use vfp, we have no any chance to save old vfp state.
>>
>> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
>> Cc: Binghua Duan <binghua.duan@csr.com>
>> Signed-off-by: Barry Song <21cnbao@gmail.com>
>
> There was a similar patch some time ago by Colin Cross. I don't know
> what happened to it but please have a look at that discussion first:
>
> http://thread.gmane.org/gmane.linux.kernel/1099558

sorry, i didn't see any patch committed and lost the thread in lkml.

Seems like Rongjun's codes have handled Russel's last change but in
the "else". Russel handles it before the "if (fpexc & FPEXC_EN) {".
We will take over the test in
http://thread.gmane.org/gmane.linux.kernel/1099558 and continue to
send patch v3.

>
> --
> Catalin
>

[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[Barry Song]

2011/4/29 Barry Song <21cnbao@gmail.com>:
> Hi Catalin,
> Thanks.
> 2011/4/29 Catalin Marinas <catalin.marinas@arm.com>:
>> On 27 April 2011 07:30, Barry Song <bs14@csr.com> wrote:
>>> From: Rongjun Ying <rongjun.ying@csr.com>
>>>
>>> Current vfp pm suspend entry only saves the vfp state of running proceed if it is using vfp. If current proceed doesn't use vfp,
>>> the state of last process will be lost after resume. In pressure tests, we can see old vfp processes crash after resume.
>>>
>>> In order that schedule can be faster, scheduler doesn't save vfp state if we schedule from proceeds using vfp to proceeds which
>>> don't use vfp. If system suspend happens just at proceeds which don't use vfp, we have no any chance to save old vfp state.
>>>
>>> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
>>> Cc: Binghua Duan <binghua.duan@csr.com>
>>> Signed-off-by: Barry Song <21cnbao@gmail.com>
>>
>> There was a similar patch some time ago by Colin Cross. I don't know
>> what happened to it but please have a look at that discussion first:
>>
>> http://thread.gmane.org/gmane.linux.kernel/1099558
>
> sorry, i didn't see any patch committed and lost the thread in lkml.
>
> Seems like Rongjun's codes have handled Russel's last change but in
> the "else". Russel handles it before the "if (fpexc & FPEXC_EN) {".
> We will take over the test in
> http://thread.gmane.org/gmane.linux.kernel/1099558 and continue to
> send patch v3.

but we don't think Russel's last change is complelety right by:
/* If lazy disable, re-enable the VFP ready for it to be saved */
	if (last_VFP_context[ti->cpu] != &ti->vfpstate) {
		fpexc |= FPEXC_EN;
		fmxr(FPEXC, fpexc);
	}
	/* If VFP is on, then save state for resumption */
	if (fpexc & FPEXC_EN) {
		...

there are still risk.  For example, if process p1/p2 switch like this:
P1:     use vfp
       swith to -> P2: don't use vfp
                    switch to  -> P1(use vfp), but it didn't begin to
use vfp, then  FPEXC_EN is not set, but suspend happen at that moment

At the last time, last_VFP_context[ti->cpu] will be &ti->vfpstate,
fpexc & FPEXC_EN will be false. it loses the chance to save status.
So looks like Rongjun's codes can avoid this kind of risk:
       /* if vfp is on, then save state for resumption */
       if (fpexc & FPEXC_EN) {
@@ -392,8 +393,16 @@ static int vfp_pm_suspend(struct sys_device *dev,
pm_message_t state)

               /* disable, just in case */
               fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+       } else {
+               if (last_VFP_context[cpu]) {
+                       fmxr(FPEXC, fpexc | FPEXC_EN);
+                       vfp_save_state(last_VFP_context[cpu], fpexc);
+                       fmxr(FPEXC, fpexc);
+               }
       }


>
>>
>> --
>> Catalin
>>
>

[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[Catalin Marinas]

On Fri, 2011-04-29 at 04:26 +0100, Barry Song wrote:
> 2011/4/29 Barry Song <21cnbao@gmail.com>:
> > Seems like Rongjun's codes have handled Russel's last change but in
> > the "else". Russel handles it before the "if (fpexc & FPEXC_EN) {".
> > We will take over the test in
> > http://thread.gmane.org/gmane.linux.kernel/1099558 and continue to
> > send patch v3.
> 
> but we don't think Russel's last change is complelety right by:
> /* If lazy disable, re-enable the VFP ready for it to be saved */
>         if (last_VFP_context[ti->cpu] != &ti->vfpstate) {
>                 fpexc |= FPEXC_EN;
>                 fmxr(FPEXC, fpexc);
>         }
>         /* If VFP is on, then save state for resumption */
>         if (fpexc & FPEXC_EN) {
>                 ...
> 
> there are still risk.  For example, if process p1/p2 switch like this:
> P1:     use vfp
>        swith to -> P2: don't use vfp
>                     switch to  -> P1(use vfp), but it didn't begin to
> use vfp, then  FPEXC_EN is not set, but suspend happen at that moment
> 
> At the last time, last_VFP_context[ti->cpu] will be &ti->vfpstate,
> fpexc & FPEXC_EN will be false. it loses the chance to save status.

I think your scenario is possible on UP systems. On SMP we save the VFP
context during thread switching anyway.

> So looks like Rongjun's codes can avoid this kind of risk:
>        /* if vfp is on, then save state for resumption */
>        if (fpexc & FPEXC_EN) {
> @@ -392,8 +393,16 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
> 
>                /* disable, just in case */
>                fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
> +       } else {
> +               if (last_VFP_context[cpu]) {
> +                       fmxr(FPEXC, fpexc | FPEXC_EN);
> +                       vfp_save_state(last_VFP_context[cpu], fpexc);
> +                       fmxr(FPEXC, fpexc);
> +               }
>        }

I think we need to set last_VFP_context[cpu] to NULL as well so that the
context is reloaded.

-- 
Catalin

[PATCH] arm:pm: save the vfp state of last scheduled-out proceed while suspending

[Barry Song]

Hi Catalin,

2011/5/3 Catalin Marinas <catalin.marinas@arm.com>:
> On Fri, 2011-04-29 at 04:26 +0100, Barry Song wrote:
>> 2011/4/29 Barry Song <21cnbao@gmail.com>:
>> > Seems like Rongjun's codes have handled Russel's last change but in
>> > the "else". Russel handles it before the "if (fpexc & FPEXC_EN) {".
>> > We will take over the test in
>> > http://thread.gmane.org/gmane.linux.kernel/1099558 and continue to
>> > send patch v3.
>>
>> but we don't think Russel's last change is complelety right by:
>> /* If lazy disable, re-enable the VFP ready for it to be saved */
>> ? ? ? ? if (last_VFP_context[ti->cpu] != &ti->vfpstate) {
>> ? ? ? ? ? ? ? ? fpexc |= FPEXC_EN;
>> ? ? ? ? ? ? ? ? fmxr(FPEXC, fpexc);
>> ? ? ? ? }
>> ? ? ? ? /* If VFP is on, then save state for resumption */
>> ? ? ? ? if (fpexc & FPEXC_EN) {
>> ? ? ? ? ? ? ? ? ...
>>
>> there are still risk. ?For example, if process p1/p2 switch like this:
>> P1: ? ? use vfp
>> ? ? ? ?swith to -> P2: don't use vfp
>> ? ? ? ? ? ? ? ? ? ? switch to ?-> P1(use vfp), but it didn't begin to
>> use vfp, then ?FPEXC_EN is not set, but suspend happen at that moment
>>
>> At the last time, last_VFP_context[ti->cpu] will be &ti->vfpstate,
>> fpexc & FPEXC_EN will be false. it loses the chance to save status.
>
> I think your scenario is possible on UP systems. On SMP we save the VFP
> context during thread switching anyway.
>
>> So looks like Rongjun's codes can avoid this kind of risk:
>> ? ? ? ?/* if vfp is on, then save state for resumption */
>> ? ? ? ?if (fpexc & FPEXC_EN) {
>> @@ -392,8 +393,16 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
>>
>> ? ? ? ? ? ? ? ?/* disable, just in case */
>> ? ? ? ? ? ? ? ?fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? if (last_VFP_context[cpu]) {
>> + ? ? ? ? ? ? ? ? ? ? ? fmxr(FPEXC, fpexc | FPEXC_EN);
>> + ? ? ? ? ? ? ? ? ? ? ? vfp_save_state(last_VFP_context[cpu], fpexc);
>> + ? ? ? ? ? ? ? ? ? ? ? fmxr(FPEXC, fpexc);
>> + ? ? ? ? ? ? ? }
>> ? ? ? ?}
>
> I think we need to set last_VFP_context[cpu] to NULL as well so that the
> context is reloaded.

The original codes have the below:
        /* clear any information we had about last context state */
        memset(last_VFP_context, 0, sizeof(last_VFP_context));


>
> --
> Catalin
>
>
>
Thanks
Barry