Hi Catalin,

2011/5/3 Catalin Marinas [off-list ref]:

On Fri, 2011-04-29 at 04:26 +0100, Barry Song wrote:

quoted

2011/4/29 Barry Song [off-list ref]:

quoted

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.

quoted

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