LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] ARM: vfp: Always save VFP state in vfp_pm_suspend
@ 2011-02-13 23:13 Colin Cross
  2011-02-14 11:42 ` Catalin Marinas
  0 siblings, 1 reply; 9+ messages in thread
From: Colin Cross @ 2011-02-13 23:13 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: Colin Cross, Catalin Marinas, Russell King, linux-kernel

vfp_pm_suspend should save the VFP state any time there is
a last_VFP_context.  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
last_VFP_context is set.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Colin Cross <ccross@android.com>
---
 arch/arm/vfp/vfpmodule.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 66bf8d1..7aea616 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -415,13 +415,12 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
 	struct thread_info *ti = current_thread_info();
 	u32 fpexc = fmrx(FPEXC);
 
-	/* if vfp is on, then save state for resumption */
-	if (fpexc & FPEXC_EN) {
+	/* save state for resume */
+	if (last_VFP_context[ti->cpu]) {
 		printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
-		vfp_save_state(&ti->vfpstate, fpexc);
-
-		/* disable, just in case */
-		fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+		fmxr(FPEXC, fpexc | FPEXC_EN);
+		vfp_save_state(last_VFP_context[ti->cpu], fpexc);
+		fmxr(FPEXC, fpexc & ~FPEXC_EN);
 	}
 
 	/* clear any information we had about last context state */
-- 
1.7.3.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-13 23:13 [PATCH] ARM: vfp: Always save VFP state in vfp_pm_suspend Colin Cross
@ 2011-02-14 11:42 ` Catalin Marinas
  2011-02-14 18:35   ` Colin Cross
  0 siblings, 1 reply; 9+ messages in thread
From: Catalin Marinas @ 2011-02-14 11:42 UTC (permalink / raw)
  To: Colin Cross; +Cc: linux-arm-kernel, Russell King, linux-kernel

On Sun, 2011-02-13 at 23:13 +0000, Colin Cross wrote:
> vfp_pm_suspend should save the VFP state any time there is
> a last_VFP_context.  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.

At this point is it guaranteed that the thread won't migrate to another
CPU? If not, we should use get/put_cpu.

> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -415,13 +415,12 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
>         struct thread_info *ti = current_thread_info();
>         u32 fpexc = fmrx(FPEXC);
> 
> -       /* if vfp is on, then save state for resumption */
> -       if (fpexc & FPEXC_EN) {
> +       /* save state for resume */
> +       if (last_VFP_context[ti->cpu]) {
>                 printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
> -               vfp_save_state(&ti->vfpstate, fpexc);
> -
> -               /* disable, just in case */
> -               fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
> +               fmxr(FPEXC, fpexc | FPEXC_EN);
> +               vfp_save_state(last_VFP_context[ti->cpu], fpexc);
> +               fmxr(FPEXC, fpexc & ~FPEXC_EN);
>         }

We may want to set the last_VFP_context to NULL so that after resuming
(to the same thread) we force the VFP reload from the vfpstate
structure. The vfp_support_entry code ignores the reloading if the
last_VFP_context is the same as vfpstate.

-- 
Catalin



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-14 11:42 ` Catalin Marinas
@ 2011-02-14 18:35   ` Colin Cross
  2011-02-14 22:55     ` [PATCH v2] " Colin Cross
  0 siblings, 1 reply; 9+ messages in thread
From: Colin Cross @ 2011-02-14 18:35 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: linux-arm-kernel, Russell King, linux-kernel

On Mon, Feb 14, 2011 at 3:42 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Sun, 2011-02-13 at 23:13 +0000, Colin Cross wrote:
>> vfp_pm_suspend should save the VFP state any time there is
>> a last_VFP_context.  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.
>
> At this point is it guaranteed that the thread won't migrate to another
> CPU? If not, we should use get/put_cpu.

Yes, VFP suspend is implemented with a sysdev, which is suspended
after disable_nonboot_cpus.

>> --- a/arch/arm/vfp/vfpmodule.c
>> +++ b/arch/arm/vfp/vfpmodule.c
>> @@ -415,13 +415,12 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
>>         struct thread_info *ti = current_thread_info();
>>         u32 fpexc = fmrx(FPEXC);
>>
>> -       /* if vfp is on, then save state for resumption */
>> -       if (fpexc & FPEXC_EN) {
>> +       /* save state for resume */
>> +       if (last_VFP_context[ti->cpu]) {
>>                 printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
>> -               vfp_save_state(&ti->vfpstate, fpexc);
>> -
>> -               /* disable, just in case */
>> -               fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>> +               fmxr(FPEXC, fpexc | FPEXC_EN);
>> +               vfp_save_state(last_VFP_context[ti->cpu], fpexc);
>> +               fmxr(FPEXC, fpexc & ~FPEXC_EN);
>>         }
>
> We may want to set the last_VFP_context to NULL so that after resuming
> (to the same thread) we force the VFP reload from the vfpstate
> structure. The vfp_support_entry code ignores the reloading if the
> last_VFP_context is the same as vfpstate.

Right, will fix.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-14 18:35   ` Colin Cross
@ 2011-02-14 22:55     ` Colin Cross
  2011-02-15 16:51       ` Catalin Marinas
  2011-02-15 17:03       ` Russell King - ARM Linux
  0 siblings, 2 replies; 9+ messages in thread
From: Colin Cross @ 2011-02-14 22:55 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: Catalin Marinas, Russell King, linux-kernel, Colin Cross

vfp_pm_suspend should save the VFP state any time there is
a last_VFP_context.  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
last_VFP_context is set.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Colin Cross <ccross@android.com>
---
 arch/arm/vfp/vfpmodule.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 66bf8d1..7231d18 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -415,13 +415,13 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
 	struct thread_info *ti = current_thread_info();
 	u32 fpexc = fmrx(FPEXC);
 
-	/* if vfp is on, then save state for resumption */
-	if (fpexc & FPEXC_EN) {
+	/* save state for resume */
+	if (last_VFP_context[ti->cpu]) {
 		printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
-		vfp_save_state(&ti->vfpstate, fpexc);
-
-		/* disable, just in case */
-		fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
+		fmxr(FPEXC, fpexc | FPEXC_EN);
+		vfp_save_state(last_VFP_context[ti->cpu], fpexc);
+		last_VFP_context[ti->cpu] = NULL;
+		fmxr(FPEXC, fpexc & ~FPEXC_EN);
 	}
 
 	/* clear any information we had about last context state */
-- 
1.7.3.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-14 22:55     ` [PATCH v2] " Colin Cross
@ 2011-02-15 16:51       ` Catalin Marinas
  2011-02-15 17:03       ` Russell King - ARM Linux
  1 sibling, 0 replies; 9+ messages in thread
From: Catalin Marinas @ 2011-02-15 16:51 UTC (permalink / raw)
  To: Colin Cross; +Cc: linux-arm-kernel, Russell King, linux-kernel

On Mon, 2011-02-14 at 22:55 +0000, Colin Cross wrote:
> vfp_pm_suspend should save the VFP state any time there is
> a last_VFP_context.  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
> last_VFP_context is set.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Colin Cross <ccross@android.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-14 22:55     ` [PATCH v2] " Colin Cross
  2011-02-15 16:51       ` Catalin Marinas
@ 2011-02-15 17:03       ` Russell King - ARM Linux
  2011-02-16 19:36         ` Colin Cross
  1 sibling, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2011-02-15 17:03 UTC (permalink / raw)
  To: Colin Cross; +Cc: linux-arm-kernel, Catalin Marinas, linux-kernel

On Mon, Feb 14, 2011 at 02:55:47PM -0800, Colin Cross wrote:
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index 66bf8d1..7231d18 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -415,13 +415,13 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
>  	struct thread_info *ti = current_thread_info();
>  	u32 fpexc = fmrx(FPEXC);
>  
> -	/* if vfp is on, then save state for resumption */
> -	if (fpexc & FPEXC_EN) {
> +	/* save state for resume */
> +	if (last_VFP_context[ti->cpu]) {

I'm not entirely happy with this.

It is true that last_VFP_context[] when non-NULL indicates who owns the
hardware VFP state, so saving it would seem logical.  However, this new
code now saves the state with the saved fpexc indicating that it's disabled.

This will cause a VFP exception to misbehave by reloading the state, and
then disabling the VFP unit.  That will cause another VFP exception which
will find the VFP unit disabled, and re-enable it.  All in all, this is
rather wasteful.

So...
	/* 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) {
		...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-15 17:03       ` Russell King - ARM Linux
@ 2011-02-16 19:36         ` Colin Cross
  2011-02-20 12:57           ` Russell King - ARM Linux
  0 siblings, 1 reply; 9+ messages in thread
From: Colin Cross @ 2011-02-16 19:36 UTC (permalink / raw)
  To: Russell King - ARM Linux; +Cc: linux-arm-kernel, Catalin Marinas, linux-kernel

On Tue, Feb 15, 2011 at 9:03 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Feb 14, 2011 at 02:55:47PM -0800, Colin Cross wrote:
>> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
>> index 66bf8d1..7231d18 100644
>> --- a/arch/arm/vfp/vfpmodule.c
>> +++ b/arch/arm/vfp/vfpmodule.c
>> @@ -415,13 +415,13 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
>>       struct thread_info *ti = current_thread_info();
>>       u32 fpexc = fmrx(FPEXC);
>>
>> -     /* if vfp is on, then save state for resumption */
>> -     if (fpexc & FPEXC_EN) {
>> +     /* save state for resume */
>> +     if (last_VFP_context[ti->cpu]) {
>
> I'm not entirely happy with this.
>
> It is true that last_VFP_context[] when non-NULL indicates who owns the
> hardware VFP state, so saving it would seem logical.  However, this new
> code now saves the state with the saved fpexc indicating that it's disabled.
>
> This will cause a VFP exception to misbehave by reloading the state, and
> then disabling the VFP unit.  That will cause another VFP exception which
> will find the VFP unit disabled, and re-enable it.  All in all, this is
> rather wasteful.
>
> So...
>        /* 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) {
>                ...

I think v2 of the patch handles this case correctly:
	/* save state for resume */
	if (last_VFP_context[ti->cpu]) {
		printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
		fmxr(FPEXC, fpexc | FPEXC_EN);
		vfp_save_state(last_VFP_context[ti->cpu], fpexc);
		last_VFP_context[ti->cpu] = NULL;
		fmxr(FPEXC, fpexc & ~FPEXC_EN);
	}

This version enables the VFP if it was not enabled, but saves the
original fpexc value.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-16 19:36         ` Colin Cross
@ 2011-02-20 12:57           ` Russell King - ARM Linux
  2011-02-20 18:43             ` Colin Cross
  0 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2011-02-20 12:57 UTC (permalink / raw)
  To: Colin Cross; +Cc: linux-arm-kernel, Catalin Marinas, linux-kernel

On Wed, Feb 16, 2011 at 11:36:45AM -0800, Colin Cross wrote:
> On Tue, Feb 15, 2011 at 9:03 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Feb 14, 2011 at 02:55:47PM -0800, Colin Cross wrote:
> >> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> >> index 66bf8d1..7231d18 100644
> >> --- a/arch/arm/vfp/vfpmodule.c
> >> +++ b/arch/arm/vfp/vfpmodule.c
> >> @@ -415,13 +415,13 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
> >>       struct thread_info *ti = current_thread_info();
> >>       u32 fpexc = fmrx(FPEXC);
> >>
> >> -     /* if vfp is on, then save state for resumption */
> >> -     if (fpexc & FPEXC_EN) {
> >> +     /* save state for resume */
> >> +     if (last_VFP_context[ti->cpu]) {
> >
> > I'm not entirely happy with this.
> >
> > It is true that last_VFP_context[] when non-NULL indicates who owns the
> > hardware VFP state, so saving it would seem logical.  However, this new
> > code now saves the state with the saved fpexc indicating that it's disabled.
> >
> > This will cause a VFP exception to misbehave by reloading the state, and
> > then disabling the VFP unit.  That will cause another VFP exception which
> > will find the VFP unit disabled, and re-enable it.  All in all, this is
> > rather wasteful.
> >
> > So...
> >        /* 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) {
> >                ...
> 
> I think v2 of the patch handles this case correctly:
> 	/* save state for resume */
> 	if (last_VFP_context[ti->cpu]) {
> 		printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
> 		fmxr(FPEXC, fpexc | FPEXC_EN);
> 		vfp_save_state(last_VFP_context[ti->cpu], fpexc);

This saves fpexc with the enable flag possibly clear.

> 		last_VFP_context[ti->cpu] = NULL;
> 		fmxr(FPEXC, fpexc & ~FPEXC_EN);
> 	}
> 
> This version enables the VFP if it was not enabled, but saves the
> original fpexc value.

Which is wrong as I said above.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] ARM: vfp: Always save VFP state in vfp_pm_suspend
  2011-02-20 12:57           ` Russell King - ARM Linux
@ 2011-02-20 18:43             ` Colin Cross
  0 siblings, 0 replies; 9+ messages in thread
From: Colin Cross @ 2011-02-20 18:43 UTC (permalink / raw)
  To: Russell King - ARM Linux; +Cc: linux-arm-kernel, Catalin Marinas, linux-kernel

On Sun, Feb 20, 2011 at 4:57 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Feb 16, 2011 at 11:36:45AM -0800, Colin Cross wrote:
>> On Tue, Feb 15, 2011 at 9:03 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Feb 14, 2011 at 02:55:47PM -0800, Colin Cross wrote:
>> >> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
>> >> index 66bf8d1..7231d18 100644
>> >> --- a/arch/arm/vfp/vfpmodule.c
>> >> +++ b/arch/arm/vfp/vfpmodule.c
>> >> @@ -415,13 +415,13 @@ static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state)
>> >>       struct thread_info *ti = current_thread_info();
>> >>       u32 fpexc = fmrx(FPEXC);
>> >>
>> >> -     /* if vfp is on, then save state for resumption */
>> >> -     if (fpexc & FPEXC_EN) {
>> >> +     /* save state for resume */
>> >> +     if (last_VFP_context[ti->cpu]) {
>> >
>> > I'm not entirely happy with this.
>> >
>> > It is true that last_VFP_context[] when non-NULL indicates who owns the
>> > hardware VFP state, so saving it would seem logical.  However, this new
>> > code now saves the state with the saved fpexc indicating that it's disabled.
>> >
>> > This will cause a VFP exception to misbehave by reloading the state, and
>> > then disabling the VFP unit.  That will cause another VFP exception which
>> > will find the VFP unit disabled, and re-enable it.  All in all, this is
>> > rather wasteful.
>> >
>> > So...
>> >        /* 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) {
>> >                ...
>>
>> I think v2 of the patch handles this case correctly:
>>       /* save state for resume */
>>       if (last_VFP_context[ti->cpu]) {
>>               printk(KERN_DEBUG "%s: saving vfp state\n", __func__);
>>               fmxr(FPEXC, fpexc | FPEXC_EN);
>>               vfp_save_state(last_VFP_context[ti->cpu], fpexc);
>
> This saves fpexc with the enable flag possibly clear.
>
>>               last_VFP_context[ti->cpu] = NULL;
>>               fmxr(FPEXC, fpexc & ~FPEXC_EN);
>>       }
>>
>> This version enables the VFP if it was not enabled, but saves the
>> original fpexc value.
>
> Which is wrong as I said above.
>

Sorry, I misunderstood.  I'll repost with your changes after I get a
chance to test it, or you can commit it yourself.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-02-20 18:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-13 23:13 [PATCH] ARM: vfp: Always save VFP state in vfp_pm_suspend Colin Cross
2011-02-14 11:42 ` Catalin Marinas
2011-02-14 18:35   ` Colin Cross
2011-02-14 22:55     ` [PATCH v2] " Colin Cross
2011-02-15 16:51       ` Catalin Marinas
2011-02-15 17:03       ` Russell King - ARM Linux
2011-02-16 19:36         ` Colin Cross
2011-02-20 12:57           ` Russell King - ARM Linux
2011-02-20 18:43             ` Colin Cross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).