ARM: ux500:mach-ux500/cpuidle.c spinlock dis-matching

Hi Daniel,

I think we must unlock the master spinlock even
prcmu_gic_decouple function now always return 0.
Could you give some infos about this?
Thanks.

arch/arm/mach-ux500/cpuidle.c +3 -0

index b54884bd..b0759ce 100644
@@ -29,6 +29,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
 {
        int this_cpu = smp_processor_id();
        bool recouple = false;
+       bool locked = false;
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);

@@ -39,6 +40,8 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                if (!spin_trylock(&master_lock))
                        goto wfi;
 
+               locked = true;
+
                /* decouple the gic from the A9 cores */
                if (prcmu_gic_decouple())

                        goto out;

arch/arm/mach-ux500/cpuidle.c +1 -1

@@ -76,7 +79,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                /* When we switch to retention, the prcmu is in charge
                 * of recoupling the gic automatically */
                recouple = false;
-
+               locked = false;
                spin_unlock(&master_lock);
        }

wfi:

arch/arm/mach-ux500/cpuidle.c +2 -1

@@ -86,7 +89,8 @@ out:
 
        if (recouple) {
                prcmu_gic_recouple();
-               spin_unlock(&master_lock);
+               if (locked)
+                       spin_unlock(&master_lock);
        }
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

Steve Zhan

Hi Daniel,

I think we must unlock the master spinlock even
prcmu_gic_decouple function now always return 0.
Could you give some infos about this?
Thanks.

arch/arm/mach-ux500/cpuidle.c +3 -0

index b54884bd..b0759ce 100644
@@ -29,6 +29,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
 {
        int this_cpu = smp_processor_id();
        bool recouple = false;
+       bool locked = false;
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);

@@ -39,6 +40,8 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                if (!spin_trylock(&master_lock))
                        goto wfi;
 
+               locked = true;
+
                /* decouple the gic from the A9 cores */
                if (prcmu_gic_decouple())

                        goto out;

arch/arm/mach-ux500/cpuidle.c +1 -1

@@ -76,7 +79,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                /* When we switch to retention, the prcmu is in charge
                 * of recoupling the gic automatically */
                recouple = false;
-
+               locked = false;
                spin_unlock(&master_lock);
        }

wfi:

arch/arm/mach-ux500/cpuidle.c +2 -1

@@ -86,7 +89,8 @@ out:
 
        if (recouple) {
                prcmu_gic_recouple();
-               spin_unlock(&master_lock);
+               if (locked)
+                       spin_unlock(&master_lock);
        }
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

Steve Zhan

Hi Daniel,

I think we must unlock the master spinlock even
prcmu_gic_decouple function now always return 0.
Could you give some infos about this?

Thanks.

arch/arm/mach-ux500/cpuidle.c +6 -2

index b54884bd..b0759ce 100644
@@ -29,6 +29,7 @@ static inline int ux500_enter_idle(struct cpuidle_device *dev,
 {
        int this_cpu = smp_processor_id();
        bool recouple = false;
+       bool locked = false;
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);
 

@@ -39,6 +40,8 @@ static inline int ux500_enter_idle(struct cpuidle_device *dev,
                if (!spin_trylock(&master_lock))
                        goto wfi;
 
+               locked = true;
+
                /* decouple the gic from the A9 cores */
                if (prcmu_gic_decouple())
                        goto out;

@@ -76,7 +79,7 @@ static inline int ux500_enter_idle(struct cpuidle_device *dev,
                /* When we switch to retention, the prcmu is in charge
                 * of recoupling the gic automatically */
                recouple = false;
-
+               locked = false;
                spin_unlock(&master_lock);
        }
 wfi:

@@ -86,7 +89,8 @@ out:
 
        if (recouple) {
                prcmu_gic_recouple();
-               spin_unlock(&master_lock);
+               if (locked)
+                       spin_unlock(&master_lock);
        }
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

Steve Zhan

Quoted 1 line

Hi Daniel,

Hi Steve,

sorry I missed your email.

Quoted 4 lines

I think we must unlock the master spinlock even
prcmu_gic_decouple function now always return 0.
Could you give some infos about this?

I agree, that would be cleaner.

AFAICS, your patch does not solve the problem because 'recouple' will be false if prcmu_gic_decouple fails, so the lock will never be release.

That will be simpler to do:

if (prcmu_gic_decouple()) {
	spin_unlock(&master);
	goto out;
}

no ?

Quoted 49 lines

Thanks.

arch/arm/mach-ux500/cpuidle.c +3 -0

index b54884bd..b0759ce 100644
@@ -29,6 +29,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
 {
        int this_cpu = smp_processor_id();
        bool recouple = false;
+       bool locked = false;
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);

@@ -39,6 +40,8 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                if (!spin_trylock(&master_lock))
                        goto wfi;
 
+               locked = true;
+
                /* decouple the gic from the A9 cores */
                if (prcmu_gic_decouple())

                        goto out;

arch/arm/mach-ux500/cpuidle.c +1 -1

@@ -76,7 +79,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                /* When we switch to retention, the prcmu is in charge
                 * of recoupling the gic automatically */
                recouple = false;
-
+               locked = false;
                spin_unlock(&master_lock);
        }

wfi:

arch/arm/mach-ux500/cpuidle.c +2 -1

@@ -86,7 +89,8 @@ out:
 
        if (recouple) {
                prcmu_gic_recouple();
-               spin_unlock(&master_lock);
+               if (locked)
+                       spin_unlock(&master_lock);
        }
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

Quoted 2 lines

Steve Zhan

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Hi Daniel,

Happy new year, Thank you for reply.
Sorry for that i have refer the old patch email.
I have updated the patch, Pls check the URL:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/138939.html
Now i am using gmail GUI tools to send mail, i will switch email tool to MUTT to commit the other patchs in the future.

--------------
steve.zhan

2013/1/7, Daniel Lezcano <[email protected]>:

Quoted 87 lines · steve.zhan

Quoted 1 line

Hi Daniel,

Hi Steve,

sorry I missed your email.

Quoted 4 lines

I think we must unlock the master spinlock even
prcmu_gic_decouple function now always return 0.
Could you give some infos about this?

I agree, that would be cleaner.

AFAICS, your patch does not solve the problem because 'recouple' will be false if prcmu_gic_decouple fails, so the lock will never be release.

That will be simpler to do:

if (prcmu_gic_decouple()) {
	spin_unlock(&master);
	goto out;
}

no ?

Quoted 49 lines

Thanks.

arch/arm/mach-ux500/cpuidle.c +3 -0

index b54884bd..b0759ce 100644
@@ -29,6 +29,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
 {
        int this_cpu = smp_processor_id();
        bool recouple = false;
+       bool locked = false;
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);

@@ -39,6 +40,8 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                if (!spin_trylock(&master_lock))
                        goto wfi;
 
+               locked = true;
+
                /* decouple the gic from the A9 cores */
                if (prcmu_gic_decouple())

                        goto out;

arch/arm/mach-ux500/cpuidle.c +1 -1

@@ -76,7 +79,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                /* When we switch to retention, the prcmu is in charge
                 * of recoupling the gic automatically */
                recouple = false;
-
+               locked = false;
                spin_unlock(&master_lock);
        }

wfi:

arch/arm/mach-ux500/cpuidle.c +2 -1

@@ -86,7 +89,8 @@ out:
 
        if (recouple) {
                prcmu_gic_recouple();
-               spin_unlock(&master_lock);
+               if (locked)
+                       spin_unlock(&master_lock);
        }
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

Quoted 2 lines

Steve Zhan

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Quoted 8 lines

Hi Daniel,

Happy new year, Thank you for reply.
Sorry for that i have refer the old patch email.
I have updated the patch, Pls check the URL:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/138939.html
Now i am using gmail GUI tools to send mail, i will switch email tool to MUTT to commit the other patchs in the future.

Ok.

Please do not introduce a new variable which is at the end pointless and add more complexity.

You can simply remove the if statement for prcmu_gic_decouple(), or unlock if it fails.

Thanks
-- Daniel

Quoted 93 lines

--------------
steve.zhan

2013/1/7, Daniel Lezcano <[email protected]>:

Quoted 87 lines · steve.zhan

Quoted 1 line

Hi Daniel,

Hi Steve,

sorry I missed your email.

Quoted 4 lines

I think we must unlock the master spinlock even
prcmu_gic_decouple function now always return 0.
Could you give some infos about this?

I agree, that would be cleaner.

AFAICS, your patch does not solve the problem because 'recouple' will be false if prcmu_gic_decouple fails, so the lock will never be release.

That will be simpler to do:

if (prcmu_gic_decouple()) {
     spin_unlock(&master);
     goto out;
}

no ?

Quoted 49 lines

Thanks.

arch/arm/mach-ux500/cpuidle.c +3 -0

index b54884bd..b0759ce 100644
@@ -29,6 +29,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
 {
        int this_cpu = smp_processor_id();
        bool recouple = false;
+       bool locked = false;
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu);

@@ -39,6 +40,8 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                if (!spin_trylock(&master_lock))
                        goto wfi;
 
+               locked = true;
+
                /* decouple the gic from the A9 cores */
                if (prcmu_gic_decouple())

                        goto out;

arch/arm/mach-ux500/cpuidle.c +1 -1

@@ -76,7 +79,7 @@ static inline int ux500_enter_idle(struct cpuidle_device
 *dev,
                /* When we switch to retention, the prcmu is in charge
                 * of recoupling the gic automatically */
                recouple = false;
-
+               locked = false;
                spin_unlock(&master_lock);
        }

wfi:

arch/arm/mach-ux500/cpuidle.c +2 -1

@@ -86,7 +89,8 @@ out:
 
        if (recouple) {
                prcmu_gic_recouple();
-               spin_unlock(&master_lock);
+               if (locked)
+                       spin_unlock(&master_lock);
        }
 
        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu);

Quoted 2 lines

Steve Zhan

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog