ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

PATCH New James Morse 6 participants 7 messages Apr 26, 2016 – May 4, 2016 CPU idle time management framework

Share & tools

James Morse

author

2016-04-26 11:15 UTC #0 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.

Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")

Signed-off-by James Morse <[email protected]>

---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpuidle/cpuidle-arm.c +1 -1

index 545069d5fdfb..e342565e8715 100644
@@ -50,7 +50,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 		 * call the CPU ops suspend protocol with idle index as a
 		 * parameter.
 		 */
-		arm_cpuidle_suspend(idx);
+		ret = arm_cpuidle_suspend(idx);
 
 		cpu_pm_exit();
 	}

Signature · 1 line

2.8.0.rc3

Lorenzo Pieralisi

2016-04-26 11:31 UTC #1 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

Quoted 11 lines · James Morse

arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.
Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")
Signed-off-by James Morse <[email protected]>
---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by Lorenzo Pieralisi <[email protected]>

Quoted 17 lines

drivers/cpuidle/cpuidle-arm.c +1 -1

index 545069d5fdfb..e342565e8715 100644
@@ -50,7 +50,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 		 * call the CPU ops suspend protocol with idle index as a
 		 * parameter.
 		 */
-		arm_cpuidle_suspend(idx);
+		ret = arm_cpuidle_suspend(idx);
 
 		cpu_pm_exit();
 	}

Signature · 1 line

2.8.0.rc3

Rafael J. Wysocki

maintainer

2016-04-26 19:05 UTC #2 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

Quoted 27 lines · Lorenzo Pieralisi

Quoted 11 lines · James Morse
arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.
Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")
Signed-off-by James Morse <[email protected]>
---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by Lorenzo Pieralisi <[email protected]>
Quoted 12 lines
drivers/cpuidle/cpuidle-arm.c +1 -1
index 545069d5fdfb..e342565e8715 100644
@@ -50,7 +50,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
               * call the CPU ops suspend protocol with idle index as a
               * parameter.
               */
-             arm_cpuidle_suspend(idx);
+             ret = arm_cpuidle_suspend(idx);
 

Quoted 3 lines

drivers/cpuidle/cpuidle-arm.c static int arm_enter_idle_state(struct cpuidle_device *dev,
Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c +0 -1
              cpu_pm_exit();
      }
--

Should that go into -stable?

Lorenzo Pieralisi

2016-04-27 09:14 UTC #3 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

[+ Lina]

Quoted 29 lines · Rafael J. Wysocki

Quoted 27 lines · Lorenzo Pieralisi
Quoted 11 lines · James Morse
arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.
Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")
Signed-off-by James Morse <[email protected]>
---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by Lorenzo Pieralisi <[email protected]>
Quoted 12 lines
drivers/cpuidle/cpuidle-arm.c +1 -1
index 545069d5fdfb..e342565e8715 100644
@@ -50,7 +50,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
               * call the CPU ops suspend protocol with idle index as a
               * parameter.
               */
-             arm_cpuidle_suspend(idx);
+             ret = arm_cpuidle_suspend(idx);
 

Quoted 7 lines

Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c static int arm_enter_idle_state(struct cpuidle_device *dev,
Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c +0 -1
              cpu_pm_exit();
      }
--
OK
Should that go into -stable?

Yes for arm64, but I would ask Daniel and Lina please to chime in if they see any issue with this change (in particular in relation to the Qualcomm back-end), it looks like it just slipped through the cracks but let's check with them before merging it and send it to stable kernels to prevent any issue.

Thanks !
Lorenzo

Lina Iyer

2016-04-27 15:04 UTC #4 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

Quoted 38 lines · Lorenzo Pieralisi

[+ Lina]
Quoted 35 lines · Rafael J. Wysocki
Quoted 27 lines · Lorenzo Pieralisi
Quoted 11 lines · James Morse
arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.
Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")
Signed-off-by James Morse <[email protected]>
---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by Lorenzo Pieralisi <[email protected]>
Quoted 12 lines
drivers/cpuidle/cpuidle-arm.c +1 -1
index 545069d5fdfb..e342565e8715 100644
@@ -50,7 +50,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
               * call the CPU ops suspend protocol with idle index as a
               * parameter.
               */
-             arm_cpuidle_suspend(idx);
+             ret = arm_cpuidle_suspend(idx);
 
Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c static int arm_enter_idle_state(struct cpuidle_device *dev,
Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c +0 -1
              cpu_pm_exit();
      }
--
OK

Quoted 8 lines

Quoted 1 line
Should that go into -stable?
Yes for arm64, but I would ask Daniel and Lina please to chime in if they see any issue with this change (in particular in relation to the Qualcomm back-end), it looks like it just slipped through the cracks but let's check with them before merging it and send it to stable kernels to prevent any issue.

This patch looks okay to me. In the QCOM backend driver, wee return -1 if the state was not achieved because of a pending interrupt. This was previously ignored, but a quick check doesnt yield an issue returning that backend specific value. Its not a real error per-se and therefore a -1.

Thanks,
Lina

Daniel Lezcano

2016-04-28 08:20 UTC #5 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

In reply to Lorenzo Pieralisi

Quoted 19 lines · Lorenzo Pieralisi

[+ Lina]
Quoted 16 lines · Rafael J. Wysocki
Quoted 14 lines · Lorenzo Pieralisi
Quoted 11 lines · James Morse
arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.
Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")
Signed-off-by James Morse <[email protected]>
---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by Lorenzo Pieralisi <[email protected]>

Acked-by Daniel Lezcano <[email protected]>

Rafael J. Wysocki

2016-05-04 21:17 UTC #6 lore raw

PATCH 1/1 ARM: cpuidle: Pass on arm_cpuidle_suspend()'s return value

Quoted 19 lines · Daniel Lezcano

Quoted 18 lines · Lorenzo Pieralisi
[+ Lina]
Quoted 15 lines · Rafael J. Wysocki
Quoted 13 lines · Lorenzo Pieralisi
Quoted 11 lines · James Morse
arm_cpuidle_suspend() may return -EOPNOTSUPP, or any value returned by the cpu_ops/cpuidle_ops suspend call. arm_enter_idle_state() doesn't update 'ret' with this value, meaning we always signal success to cpuidle_enter_state(), causing it to update the usage counters as if we succeeded.
Fixes 191de17aa3c1 ("ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function")
Signed-off-by James Morse <[email protected]>
---
 drivers/cpuidle/cpuidle-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 3 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Acked-by Lorenzo Pieralisi <[email protected]>
Acked-by Daniel Lezcano <[email protected]>

OK, applied, tagged for -stable (4.1+).

Thanks everybody!