ARM/ARM64: arch-timer: fix arch_timer_probed logic

PATCH New Marc Zyngier 5 participants 9 messages Oct 15, 2014 – Oct 27, 2014

Subsystems (2)

Clocksource, clockevent drivers Arm architected timer driver

lore

Share & tools

lore

Marc Zyngier

author

2014-10-15 15:06 UTC #0 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.

There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.

For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).

Just fix the damned logic, and enjoy booting VMs again.

Tested on a Juno system.

Cc Sudeep Holla <[email protected]>

Cc Stephen Boyd <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Christoffer Dall <[email protected]>

Reported-by Riku Voipio <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/clocksource/arm_arch_timer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/clocksource/arm_arch_timer.c +3 -3

index 2133f9d..43005d4 100644
@@ -660,11 +660,11 @@ static bool __init
 arch_timer_probed(int type, const struct of_device_id *matches)
 {
 	struct device_node *dn;
-	bool probed = false;
+	bool probed = true;
 
 	dn = of_find_matching_node(NULL, matches);
-	if (dn && of_device_is_available(dn) && (arch_timers_present & type))
-		probed = true;
+	if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
+		probed = false;
 	of_node_put(dn);
 
 	return probed;

Signature · 1 line

2.0.4

Mark Rutland

maintainer

2014-10-15 15:28 UTC #1 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Quoted 10 lines · Marc Zyngier

Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).

As far as I can tell, the logic is flawed for all cases except two functional nodes that we manage to probe.

Quoted 5 lines

Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.

Quoted 7 lines

Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>

The new logic makes sense to me, so:

Acked-by Mark Rutland <[email protected]>

Thanks,
Mark.

Quoted 27 lines

---
 drivers/clocksource/arm_arch_timer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/clocksource/arm_arch_timer.c +3 -3

index 2133f9d..43005d4 100644
@@ -660,11 +660,11 @@ static bool __init
 arch_timer_probed(int type, const struct of_device_id *matches)
 {
 	struct device_node *dn;
-	bool probed = false;
+	bool probed = true;
 
 	dn = of_find_matching_node(NULL, matches);
-	if (dn && of_device_is_available(dn) && (arch_timers_present & type))
-		probed = true;
+	if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
+		probed = false;
 	of_node_put(dn);
 
 	return probed;

Signature · 1 line

2.0.4

Sudeep Holla

2014-10-15 15:49 UTC #2 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Quoted 9 lines · Mark Rutland

Quoted 8 lines · Marc Zyngier
Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.

Quoted 6 lines

Quoted 2 lines
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
As far as I can tell, the logic is flawed for all cases except two functional nodes that we manage to probe.

Agreed, it's my mistake. I inverted the logic incorrectly when I moved it to a function while adding of_node_put in v2 of the patch.

I think wrong DTB got picked up when I tested this. I am sorry for that.

Quoted 13 lines

Quoted 5 lines
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.
Quoted 7 lines
Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>

Quoted 3 lines

The new logic makes sense to me, so:
Acked-by Mark Rutland <[email protected]>

Acked-by Sudeep Holla <[email protected]>

Tested-by Sudeep Holla <[email protected]>

(This time tested all possible case)

Regards,
Sudeep

Daniel Lezcano

Applied

2014-10-21 10:58 UTC #3 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

In reply to Marc Zyngier

Quoted 15 lines · Marc Zyngier

Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.

Quoted 8 lines

Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---

Applied to my tree. Also for -next.

Thanks !

-- Daniel

Quoted 23 lines

  drivers/clocksource/arm_arch_timer.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

drivers/clocksource/arm_arch_timer.c +3 -3

index 2133f9d..43005d4 100644
@@ -660,11 +660,11 @@ static bool __init
  arch_timer_probed(int type, const struct of_device_id *matches)
  {
  	struct device_node *dn;
-	bool probed = false;
+	bool probed = true;
 
  	dn = of_find_matching_node(NULL, matches);
-	if (dn && of_device_is_available(dn) && (arch_timers_present & type))
-		probed = true;
+	if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
+		probed = false;
  	of_node_put(dn);
 
  	return probed;

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

Mark Rutland

maintainer

2014-10-21 11:07 UTC #4 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Hi Daniel,

Quoted 25 lines · Daniel Lezcano

Quoted 15 lines · Marc Zyngier
Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.
Quoted 8 lines
Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---

Quoted 1 line

Applied to my tree. Also for -next.

Is this going to hit -rc2? This was a regression introduced in -rc1.

Without this fix we've also lost our high precision sched_clock on arm64 platforms.

Thanks,
Mark.

Quoted 38 lines

Thanks !

-- Daniel

Quoted 23 lines

  drivers/clocksource/arm_arch_timer.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

drivers/clocksource/arm_arch_timer.c +3 -3

index 2133f9d..43005d4 100644
@@ -660,11 +660,11 @@ static bool __init
  arch_timer_probed(int type, const struct of_device_id *matches)
  {
  	struct device_node *dn;
-	bool probed = false;
+	bool probed = true;
 
  	dn = of_find_matching_node(NULL, matches);
-	if (dn && of_device_is_available(dn) && (arch_timers_present & type))
-		probed = true;
+	if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
+		probed = false;
  	of_node_put(dn);
 
  	return probed;

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

2014-10-21 11:12 UTC #5 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Quoted 28 lines · Mark Rutland

Hi Daniel,
Quoted 25 lines · Daniel Lezcano
Quoted 23 lines · Marc Zyngier
Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.
Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---

Quoted 7 lines

Quoted 1 line
Applied to my tree. Also for -next.
Is this going to hit -rc2? This was a regression introduced in -rc1.
Without this fix we've also lost our high precision sched_clock on arm64 platforms.

Sure.

Thomas or Ingo,

is it possible to update the tip/urgent branch, so I can send the fixes against 3.18-rc1 ?

Thanks in advance

-- Daniel

Quoted 33 lines

Quoted 33 lines
Quoted 13 lines
   drivers/clocksource/arm_arch_timer.c | 6 +++---
   1 file changed, 3 insertions(+), 3 deletions(-)
drivers/clocksource/arm_arch_timer.c +0 -1
index 2133f9d..43005d4 100644
@@ -660,11 +660,11 @@ static bool __init
   arch_timer_probed(int type, const struct of_device_id *matches)
   {
   	struct device_node *dn;
-	bool probed = false;
drivers/clocksource/arm_arch_timer.c static bool __init · line 670
Quoted 10 lines
drivers/clocksource/arm_arch_timer.c +3 -2
+	bool probed = true;
 
   	dn = of_find_matching_node(NULL, matches);
-	if (dn && of_device_is_available(dn) && (arch_timers_present & type))
-		probed = true;
+	if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
+		probed = false;
   	of_node_put(dn);
 
   	return probed;
--
<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

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

Marc Zyngier

author

2014-10-27 10:33 UTC #6 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Hi Daniel,

Quoted 37 lines · Daniel Lezcano

Quoted 28 lines · Mark Rutland
Hi Daniel,
Quoted 25 lines · Daniel Lezcano
Quoted 23 lines · Marc Zyngier
Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.
Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
Quoted 7 lines
Quoted 1 line
Applied to my tree. Also for -next.
Is this going to hit -rc2? This was a regression introduced in -rc1.
Without this fix we've also lost our high precision sched_clock on arm64 platforms.

Quoted 6 lines

Sure.
Thomas or Ingo,
is it possible to update the tip/urgent branch, so I can send the fixes against 3.18-rc1 ?

Any update on this? -rc2 has landed, but arm/arm64 timers are still in a rather bad shape. Can this please be merged as an urgent fix?

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Thomas Gleixner

2014-10-27 20:30 UTC #7 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Quoted 42 lines · Marc Zyngier

Hi Daniel,
Quoted 39 lines · Daniel Lezcano
Quoted 28 lines · Mark Rutland
Hi Daniel,
Quoted 25 lines · Daniel Lezcano
Quoted 23 lines · Marc Zyngier
Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.
Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
Quoted 7 lines
Quoted 1 line
Applied to my tree. Also for -next.
Is this going to hit -rc2? This was a regression introduced in -rc1.
Without this fix we've also lost our high precision sched_clock on arm64 platforms.
Sure.

Quoted 7 lines

Quoted 4 lines
Thomas or Ingo,
is it possible to update the tip/urgent branch, so I can send the fixes against 3.18-rc1 ?
Any update on this? -rc2 has landed, but arm/arm64 timers are still in a rather bad shape. Can this please be merged as an urgent fix?

Daniel, timers/urgent is on rc1 already. Please send your pull request.

Thanks,

tglx

Daniel Lezcano

2014-10-27 21:52 UTC #8 lore raw

PATCH 1/1 ARM/ARM64: arch-timer: fix arch_timer_probed logic

Quoted 45 lines · Thomas Gleixner

Quoted 44 lines · Marc Zyngier
Hi Daniel,
Quoted 41 lines · Daniel Lezcano
Quoted 28 lines · Mark Rutland
Hi Daniel,
Quoted 25 lines · Daniel Lezcano
Quoted 23 lines · Marc Zyngier
Commit c387f07e6205 (clocksource: arm_arch_timer: Discard unavailable timers correctly) changed the way the driver makes sure both the memory and system-register timers have been probed before finalizing the probing.
There is a interesting flaw in this logic that leads to this final step never to be executed. Things seems to work pretty well until something actually needs the data that is produced during this final stage.
For example, KVM explodes on the first run of a guest when executed on a platform that has both memory and sysreg nodes (Juno, for example).
Just fix the damned logic, and enjoy booting VMs again.
Tested on a Juno system.
Cc Sudeep Holla <[email protected]>
Cc Stephen Boyd <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Daniel Lezcano <[email protected]>
Cc Christoffer Dall <[email protected]>
Reported-by Riku Voipio <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
Quoted 7 lines
Quoted 1 line
Applied to my tree. Also for -next.
Is this going to hit -rc2? This was a regression introduced in -rc1.
Without this fix we've also lost our high precision sched_clock on arm64 platforms.
Sure.
Thomas or Ingo,

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
is it possible to update the tip/urgent branch, so I can send the fixes against 3.18-rc1 ?
Any update on this? -rc2 has landed, but arm/arm64 timers are still in a rather bad shape. Can this please be merged as an urgent fix?
Daniel, timers/urgent is on rc1 already. Please send your pull request.

Hi Thomas,

This pull request for one patch contains a fix for ARM/ARM64 platform:

* Marc Zyngier fixed the probe logic for the ARM architected timers.

Thanks

-- Daniel

The following changes since commit 59aa896db80479dec29f471a7ca2b9eeeeb7d38e:

ARM/ARM64: arch-timer: fix arch_timer_probed logic (2014-10-26
20:50:00 +0100)

are available in the git repository at:

http://git.linaro.org/people/daniel.lezcano/linux.git
clockevents/3.18-fixes

for you to fetch changes up to 59aa896db80479dec29f471a7ca2b9eeeeb7d38e:

ARM/ARM64: arch-timer: fix arch_timer_probed logic (2014-10-26
20:50:00 +0100)

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