cpuidle: mvebu: Fix the CPU PM notifier usage

PATCH New Gregory CLEMENT 4 participants 15 messages Feb 26, 2015 – Mar 4, 2015 CPU idle time management framework

Share & tools

Gregory CLEMENT

author

2015-02-26 17:20 UTC #0 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.

This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.

Reported-by Fulvio Benini <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/cpuidle/cpuidle-mvebu-v7.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-mvebu-v7.c +2 -2

index 38e68618513a..cefa07438ae1 100644
@@ -37,11 +37,11 @@ static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 		deepidle = true;
 
 	ret = mvebu_v7_cpu_suspend(deepidle);
+	cpu_pm_exit();
+
 	if (ret)
 		return ret;
 
-	cpu_pm_exit();
-
 	return index;
 }

Signature · 1 line

2.1.0

Rafael J. Wysocki

2015-02-26 21:31 UTC #1 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 11 lines · Gregory CLEMENT

As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>

Should that go to "stable" too? Which "stable" series it should go to if so?

Quoted 23 lines

---
 drivers/cpuidle/cpuidle-mvebu-v7.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-mvebu-v7.c +2 -2

index 38e68618513a..cefa07438ae1 100644
@@ -37,11 +37,11 @@ static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 		deepidle = true;
 
 	ret = mvebu_v7_cpu_suspend(deepidle);
+	cpu_pm_exit();
+
 	if (ret)
 		return ret;
 
-	cpu_pm_exit();
-
 	return index;
 }

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Gregory CLEMENT

author

2015-02-27 09:39 UTC #2 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Hi Rafael,

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.

Quoted 7 lines

Quoted 5 lines
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?

Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.

Thanks,

Gregory

Quoted 25 lines

Quoted 24 lines

---
 drivers/cpuidle/cpuidle-mvebu-v7.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-mvebu-v7.c +2 -2

index 38e68618513a..cefa07438ae1 100644
@@ -37,11 +37,11 @@ static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
 		deepidle = true;
 
 	ret = mvebu_v7_cpu_suspend(deepidle);
+	cpu_pm_exit();
+
 	if (ret)
 		return ret;
 
-	cpu_pm_exit();
-
 	return index;
 }

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Daniel Lezcano

2015-03-03 10:30 UTC #3 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 13 lines · Gregory CLEMENT

Hi Rafael,
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.

Hi Gregory,

actually the 'stable' rules state clearly:

"- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)."

You say "it fixes a potential issue", so no bug has been raise yet, right ?

Quoted 26 lines

Quoted 24 lines

Quoted 23 lines

---
  drivers/cpuidle/cpuidle-mvebu-v7.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-mvebu-v7.c +2 -2

index 38e68618513a..cefa07438ae1 100644
@@ -37,11 +37,11 @@ static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
  		deepidle = true;
 
  	ret = mvebu_v7_cpu_suspend(deepidle);
+	cpu_pm_exit();
+
  	if (ret)
  		return ret;
 
-	cpu_pm_exit();
-
  	return index;
  }

Signature · 5 lines

  <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

Gregory CLEMENT

author

2015-03-03 10:34 UTC #4 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Hi Rafael,

Quoted 23 lines · Daniel Lezcano

Quoted 13 lines · Gregory CLEMENT
Hi Rafael,
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.

Quoted 8 lines

Hi Gregory,
actually the 'stable' rules state clearly:
"- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)."
You say "it fixes a potential issue", so no bug has been raise yet, right ?

Indeed nobody claimed yet having a bug related to this issue.

Gregory

Quoted 29 lines

Quoted 27 lines

Quoted 25 lines

Quoted 24 lines

---
  drivers/cpuidle/cpuidle-mvebu-v7.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-mvebu-v7.c +2 -2

index 38e68618513a..cefa07438ae1 100644
@@ -37,11 +37,11 @@ static int mvebu_v7_enter_idle(struct cpuidle_device *dev,
  		deepidle = true;
 
  	ret = mvebu_v7_cpu_suspend(deepidle);
+	cpu_pm_exit();
+
  	if (ret)
  		return ret;
 
-	cpu_pm_exit();
-
  	return index;
  }

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Fulvio

2015-03-03 10:52 UTC #5 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 39 lines · Gregory CLEMENT

Hi Rafael,
Quoted 33 lines · Daniel Lezcano
Quoted 15 lines · Gregory CLEMENT
Hi Rafael,
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.
Hi Gregory,
actually the 'stable' rules state clearly:
"- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)."

Quoted 8 lines

Quoted 2 lines
You say "it fixes a potential issue", so no bug has been raise yet, right ?
Indeed nobody claimed yet having a bug related to this issue.
Gregory

I reported the issue, but i cannot say if it's a real bug.
I had random kernel panics with a Netgear ReadyNAS RN102 (armada 370 cpu):
http://www.readynas.com/forum/viewtopic.php?f=20&t=78697&sid=14747617286d55ac27296cdee7a3f420&start=210#p452214

All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.
I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered. Bye,
Fulvio

Daniel Lezcano

2015-03-03 11:12 UTC #6 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 41 lines · Fulvio

Quoted 33 lines · Gregory CLEMENT
Hi Rafael,
Quoted 28 lines · Daniel Lezcano
Quoted 13 lines · Gregory CLEMENT
Hi Rafael,
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.
Hi Gregory,
actually the 'stable' rules state clearly:
"- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)."
Quoted 7 lines
Quoted 2 lines
You say "it fixes a potential issue", so no bug has been raise yet, right ?
Indeed nobody claimed yet having a bug related to this issue.
Gregory

Quoted 7 lines

I reported the issue, but i cannot say if it's a real bug.
I had random kernel panics with a Netgear ReadyNAS RN102 (armada 370 cpu):
http://www.readynas.com/forum/viewtopic.php?f=20&t=78697&sid=14747617286d55ac27296cdee7a3f420&start=210#p452214
All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.

Hi Fulvio,

so IIUC, you suggest the stress test prevent the system to use cpuidle because of the busy cycles, right ?

Is it possible to have the kernel panic stack ?

Are you able to reproduce the kernel panic ? and test a new kernel ?

Thanks

-- Daniel

Quoted 4 lines

I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered. Bye,
Fulvio

Signature · 5 lines

  <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

Gregory CLEMENT

author

2015-03-03 12:51 UTC #7 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

In reply to Fulvio

Hi Fulvio,

Quoted 43 lines · Fulvio

Quoted 42 lines · Gregory CLEMENT
Hi Rafael,
Quoted 35 lines · Daniel Lezcano
Quoted 15 lines · Gregory CLEMENT
Hi Rafael,
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.
Hi Gregory,
actually the 'stable' rules state clearly:
"- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)."
You say "it fixes a potential issue", so no bug has been raise yet, right ?

Quoted 8 lines

Quoted 5 lines
Indeed nobody claimed yet having a bug related to this issue.
Gregory
I reported the issue, but i cannot say if it's a real bug.
I had random kernel panics with a Netgear ReadyNAS RN102 (armada 370 cpu):
http://www.readynas.com/forum/viewtopic.php?f=20&t=78697&sid=14747617286d55ac27296cdee7a3f420&start=210#p452214

I didn't know you experimented random kernel panics and that you thought it was related to the CPU Idle driver.

Quoted 5 lines

All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.
I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered.

I think that if you disable all the state using by doing an

echo 1 > /sys/devices/system/cpu/cpu0/cpuidle/stateNUM/disable

where NUM is the nuymbert of the state. Then it should disable the cpuidle on the fly.

Daniel, is it correct?

Or if you can modify your bootargs, then just add cpuidle.off=1 to the kernel command line.

Thanks,

Gregory

Quoted 3 lines

Bye,
Fulvio

Signature · 4 lines

Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Daniel Lezcano

2015-03-03 13:00 UTC #8 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 64 lines · Gregory CLEMENT

Hi Fulvio,
Quoted 43 lines · Fulvio
Quoted 42 lines · Gregory CLEMENT
Hi Rafael,
Quoted 35 lines · Daniel Lezcano
Quoted 15 lines · Gregory CLEMENT
Hi Rafael,
Quoted 11 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Yes as it fixes a potential issue, you're right it should go to "stable". The bug was here since the introduction of the driver in 3.16.
Hi Gregory,
actually the 'stable' rules state clearly:
"- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)."
You say "it fixes a potential issue", so no bug has been raise yet, right ?
Quoted 8 lines
Quoted 5 lines
Indeed nobody claimed yet having a bug related to this issue.
Gregory
I reported the issue, but i cannot say if it's a real bug.
I had random kernel panics with a Netgear ReadyNAS RN102 (armada 370 cpu):
http://www.readynas.com/forum/viewtopic.php?f=20&t=78697&sid=14747617286d55ac27296cdee7a3f420&start=210#p452214
I didn't know you experimented random kernel panics and that you thought it was related to the CPU Idle driver.
Quoted 5 lines
All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.
I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered.

Quoted 8 lines

I think that if you disable all the state using by doing an
echo 1 > /sys/devices/system/cpu/cpu0/cpuidle/stateNUM/disable
where NUM is the nuymbert of the state. Then it should disable the cpuidle on the fly.
Daniel, is it correct?

Yes, it is correct.

Make sure it is done on all cpus.

Quoted 12 lines

Or if you can modify your bootargs, then just add cpuidle.off=1 to the kernel command line.
Thanks,
Gregory
Quoted 3 lines
Bye,
Fulvio

Signature · 5 lines

  <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

Fulvio

2015-03-03 14:58 UTC #9 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

In reply to Gregory CLEMENT

Quoted 11 lines

I didn't know you experimented random kernel panics and that you thought it was related to the CPU Idle driver.
Quoted 5 lines
All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.
I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered.

Quoted 7 lines

I think that if you disable all the state using by doing an
echo 1 > /sys/devices/system/cpu/cpu0/cpuidle/stateNUM/disable
where NUM is the nuymbert of the state. Then it should disable the cpuidle on the fly.

Thanks, i'll try that as soon as possible (i gave back the unit to my client) and report back.

However, the description of the cpu_pm_enter function state:
"Must be called on the affected CPU with interrupts disabled. Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called. Notified drivers can include VFP co-processor, interrupt controller and its PM extensions, local CPU timers context save/restore which shouldn't be interrupted. Hence it must be called with interrupts disabled."

and the point is: it that an invariant? Do current code and future code safely assume that cpu_pm_enter is not called twice?
For example if cpu_pm_enter do "context save" and cpu_pm_exit do "context restore", calling twice cpu_pm_enter will overwrite the previous saved context: is that safe in all circumstances?

I assume the rule " It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)." is to avoid committing useless changes that may introduce new bugs, but i do not think that apply to this case: a bug report from an unknown user (me) should change nothing.

Bye,
Fulvio

Daniel Lezcano

2015-03-03 15:20 UTC #10 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 26 lines · Fulvio

Quoted 9 lines
I didn't know you experimented random kernel panics and that you thought it was related to the CPU Idle driver.
Quoted 4 lines
All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.
I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered.
Quoted 6 lines
I think that if you disable all the state using by doing an
echo 1 > /sys/devices/system/cpu/cpu0/cpuidle/stateNUM/disable
where NUM is the nuymbert of the state. Then it should disable the cpuidle on the fly.
Thanks, i'll try that as soon as possible (i gave back the unit to my client) and report back.
However, the description of the cpu_pm_enter function state:
"Must be called on the affected CPU with interrupts disabled. Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called. Notified drivers can include VFP co-processor, interrupt controller and its PM extensions, local CPU timers context save/restore which shouldn't be interrupted. Hence it must be called with interrupts disabled."

Quoted 2 lines

and the point is: it that an invariant? Do current code and future code safely assume that cpu_pm_enter is not called twice?

The fix is correct. The cpu_pm_enter/exit symmetry must be kept because we don't know what the notifier clients are doing.

The point is : can we send it to stable@ as a bug fix or not ?

Quoted 3 lines

For example if cpu_pm_enter do "context save" and cpu_pm_exit do "context restore", calling twice cpu_pm_enter will overwrite the previous saved context: is that safe in all circumstances?

That is the drawback of the notifiers: the kernel provides a service and everyone plug something on it. The cpu_pm notifier are very low level functions, so the answer of your question is not obvious. I already checked all the cpuidle drivers if the potential bug you reported is there or not but apparently everything else is fine, cpu_pm_exit is always called after cpu_pm_enter.

As you stated, the API description implies cpu_pm_exit must be called after cpu_pm_enter. So the fix is right.

Quoted 5 lines

I assume the rule " It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)." is to avoid committing useless changes that may introduce new bugs, but i do not think that apply to this case: a bug report from an unknown user (me) should change nothing.

It would be perfect if we can succeed to reproduce the bug you are facing and check the patch fixes it. In this case, it goes to stable@ automatically.

Signature · 5 lines

  <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

Rafael J. Wysocki

2015-03-04 14:29 UTC #11 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 27 lines · Daniel Lezcano

Quoted 26 lines · Fulvio
Quoted 9 lines
I didn't know you experimented random kernel panics and that you thought it was related to the CPU Idle driver.
Quoted 4 lines
All i can say is that the system use the "armadaxp_idle" driver and works fine when running "stress --cpu 8" in background.
I asked Netgear to provide a firmware without the idle driver to confirm if it's the cause of the problem, but they did not answered.
Quoted 6 lines
I think that if you disable all the state using by doing an
echo 1 > /sys/devices/system/cpu/cpu0/cpuidle/stateNUM/disable
where NUM is the nuymbert of the state. Then it should disable the cpuidle on the fly.
Thanks, i'll try that as soon as possible (i gave back the unit to my client) and report back.
However, the description of the cpu_pm_enter function state:
"Must be called on the affected CPU with interrupts disabled. Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called. Notified drivers can include VFP co-processor, interrupt controller and its PM extensions, local CPU timers context save/restore which shouldn't be interrupted. Hence it must be called with interrupts disabled."

Quoted 7 lines

Quoted 2 lines
and the point is: it that an invariant? Do current code and future code safely assume that cpu_pm_enter is not called twice?
The fix is correct. The cpu_pm_enter/exit symmetry must be kept because we don't know what the notifier clients are doing.
The point is : can we send it to stable@ as a bug fix or not ?

Yes, we can, unless there's a risk of breaking things that cannot be ignored.

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Daniel Lezcano

2015-03-04 14:34 UTC #12 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Daniel Lezcano
On 03/03/2015 03:58 PM, Fulvio wrote:

[ ... ]

Quoted 7 lines

Quoted 4 lines
The fix is correct. The cpu_pm_enter/exit symmetry must be kept because we don't know what the notifier clients are doing.
The point is : can we send it to stable@ as a bug fix or not ?
Yes, we can, unless there's a risk of breaking things that cannot be ignored.

Ok, I added the stable@ tag.

Thanks
-- Daniel

Signature · 5 lines

  <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

Daniel Lezcano

2015-02-27 16:50 UTC #13 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

In reply to Rafael J. Wysocki

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.

Quoted 7 lines

Quoted 5 lines
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?

Hi Rafael,

do you mind if I take this fix in my tree ? There is the same issue for cpuidle-arm64.c I fixed.

Signature · 5 lines

  <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

Rafael J. Wysocki

2015-02-27 21:55 UTC #14 lore raw

PATCH 1/1 cpuidle: mvebu: Fix the CPU PM notifier usage

Quoted 11 lines · Daniel Lezcano

Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · Gregory CLEMENT
As stated in kernel/cpu_pm.c, "Platform is responsible for ensuring that cpu_pm_enter is not called twice on the same CPU before cpu_pm_exit is called.". In the current code in case of failure when calling mvebu_v7_cpu_suspend, the function cpu_pm_exit() is never called whereas cpu_pm_enter() was called just before.
This patch moves the cpu_pm_exit() in order to balance the cpu_pm_enter() calls.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Reported-by Fulvio Benini <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
Should that go to "stable" too? Which "stable" series it should go to if so?
Hi Rafael,
do you mind if I take this fix in my tree ?

Not at all, please go ahead.

Quoted 1 line

There is the same issue for cpuidle-arm64.c I fixed.

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.