ARM: vfp: Always save VFP state in vfp_pm_suspend

PATCH Superseded Colin Cross 3 participants 9 messages Feb 13, 2011 – Feb 20, 2011

Subsystems (2)

Arm (32-bit) Arm/vfp support

lore

Share & tools

lore

Colin Cross

author

2011-02-13 23:13 UTC #0 lore raw

PATCH 1/1 ARM: vfp: Always save VFP state in vfp_pm_suspend

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 <[email protected]>

Signed-off-by Colin Cross <[email protected]>

---
 arch/arm/vfp/vfpmodule.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

arch/arm/vfp/vfpmodule.c +5 -6

index 66bf8d1..7aea616 100644
@@ -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 */

Signature · 1 line

1.7.3.1

Catalin Marinas

2011-02-14 11:42 UTC #1 lore raw

Quoted 9 lines · 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.

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

Quoted 9 lines

arch/arm/vfp/vfpmodule.c +1 -2

@@ -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 */

arch/arm/vfp/vfpmodule.c static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state) · line 424

Quoted 10 lines

arch/arm/vfp/vfpmodule.c +4 -4

+       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.

Signature · 1 line

Catalin

Colin Cross

author

2011-02-14 18:35 UTC #2 lore raw

Quoted 11 lines · Catalin Marinas

Quoted 9 lines · 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.

Quoted 2 lines

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.

Quoted 16 lines

arch/arm/vfp/vfpmodule.c +3 -6

@@ -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);

Quoted 8 lines

arch/arm/vfp/vfpmodule.c static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state) · line 424
Quoted 3 lines
arch/arm/vfp/vfpmodule.c +2 -0
+ ? ? ? ? ? ? ? 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.

Colin Cross

author

2011-02-14 22:55 UTC #3 lore raw

PATCH v2 1/1 ARM: vfp: Always save VFP state in vfp_pm_suspend

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

Cc Catalin Marinas <[email protected]>

Signed-off-by Colin Cross <[email protected]>

---
 arch/arm/vfp/vfpmodule.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

arch/arm/vfp/vfpmodule.c +6 -6

index 66bf8d1..7231d18 100644
@@ -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 */

Signature · 1 line

1.7.3.1

Catalin Marinas

Acked-by on v2 1/1

2011-02-15 16:51 UTC #4 lore raw

Quoted 10 lines · 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.

Quoted 5 lines

Modify vfp_pm_suspend to save the VFP context whenever last_VFP_context is set.
Cc Catalin Marinas <[email protected]>
Signed-off-by Colin Cross <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Russell King - ARM Linux

2011-02-15 17:03 UTC #5 lore raw

In reply to Colin Cross

arch/arm/vfp/vfpmodule.c static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state) · line 419

Quoted 12 lines · Colin Cross

arch/arm/vfp/vfpmodule.c +2 -2

index 66bf8d1..7231d18 100644
@@ -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) {
		...

Colin Cross

author

2011-02-16 19:36 UTC #6 lore raw

Quoted 25 lines · Russell King - ARM Linux

arch/arm/vfp/vfpmodule.c static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state) · line 419
Quoted 12 lines · Colin Cross
arch/arm/vfp/vfpmodule.c +2 -2
index 66bf8d1..7231d18 100644
@@ -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.

Quoted 9 lines

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.

Russell King - ARM Linux

2011-02-20 12:57 UTC #7 lore raw

Quoted 37 lines · Colin Cross

Quoted 25 lines · Russell King - ARM Linux
arch/arm/vfp/vfpmodule.c static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state) · line 419
Quoted 12 lines · Colin Cross
arch/arm/vfp/vfpmodule.c +2 -2
index 66bf8d1..7231d18 100644
@@ -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.
Quoted 9 lines
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) {
? ? ? ? ? ? ? ?...

Quoted 6 lines

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.

Quoted 6 lines

		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.

Colin Cross

author

2011-02-20 18:43 UTC #8 lore raw

Quoted 47 lines · Russell King - ARM Linux

Quoted 37 lines · Colin Cross
Quoted 25 lines · Russell King - ARM Linux
arch/arm/vfp/vfpmodule.c static int vfp_pm_suspend(struct sys_device *dev, pm_message_t state) · line 419
Quoted 12 lines · Colin Cross
arch/arm/vfp/vfpmodule.c +2 -2
index 66bf8d1..7231d18 100644
@@ -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.
Quoted 9 lines
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) {
? ? ? ? ? ? ? ?...
Quoted 6 lines
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.

Quoted 9 lines

Quoted 6 lines
? ? ? ? ? ? ? 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.