Skip to contentSkip to conversation

ARM: mvebu: Fix of_clk_get() call in a non sleeping context

PATCH New Gregory CLEMENT 3 participants 9 messages Mar 8, 2016 – Mar 10, 2016

Subsystems (4)
Stable branch Arm (32-bit) Arm sub-architectures Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support
lore
Share & tools
lore

Gregory CLEMENT

author
#0 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

of_clk_get() is called armada_xp_smp_prepare_cpus() function. This callback can be called in a context where it should not sleep as pointed when enabling CONFIG_DEBUG_ATOMIC_SLEEP. However of_clk_get() can sleep.

This patch solved this issue by moving the of_clk_get() during initialization by gathering the list of reference on the clock, and only access to this list later.

Fixes f6cec7cd0777 ("ARM: mvebu: remove device tree parsing for cpu

nodes")

Cc <[email protected]>
Signed-off-by Gregory CLEMENT <[email protected]>
---
 arch/arm/mach-mvebu/platsmp.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)
arch/arm/mach-mvebu/platsmp.c +22 -10
index f9597b701028..33ef7f9f4ef2 100644
@@ -35,17 +35,11 @@
 #define AXP_BOOTROM_BASE 0xfff00000
 #define AXP_BOOTROM_SIZE 0x100000
 
+static struct clk *cpu_clks[ARMADA_XP_MAX_CPUS];
+
 static struct clk *get_cpu_clk(int cpu)
 {
-	struct clk *cpu_clk;
-	struct device_node *np = of_get_cpu_node(cpu, NULL);
-
-	if (WARN(!np, "missing cpu node\n"))
-		return NULL;
-	cpu_clk = of_clk_get(np, 0);
-	if (WARN_ON(IS_ERR(cpu_clk)))
-		return NULL;
-	return cpu_clk;
+	return cpu_clks[cpu];
 }
 
 static void set_secondary_cpu_clock(unsigned int cpu)

@@ -126,7 +120,7 @@ static void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 {
 	struct device_node *node;
 	struct resource res;
-	int err;
+	int err, cpu;
 
 	flush_cache_all();
 	set_cpu_coherent();

@@ -146,6 +140,24 @@ static void __init armada_xp_smp_prepare_cpus(unsigned int max_cpus)
 	if (res.start != AXP_BOOTROM_BASE ||
 	    resource_size(&res) != AXP_BOOTROM_SIZE)
 		panic("The address for the BootROM is incorrect");
+
+	/*
+	 * of_clk_get() can sleep, but we need a clk reference in
+	 * armada_xp_smp_prepare_cpus and the smp_prepare_cpus() call
+	 * back must not sleep. So we gather the list of reference on
+	 * the clock now.
+	 */
+	for (cpu = 0; cpu < ARMADA_XP_MAX_CPUS; cpu++) {
+		struct clk *cpu_clk;
+
+		node = of_get_cpu_node(cpu, NULL);
+		if (WARN(!node, "missing cpu node\n"))
+			continue;
+		cpu_clk = of_clk_get(node, 0);
+		if (WARN_ON(IS_ERR(cpu_clk)))
+			cpu_clk = NULL;
+		cpu_clks[cpu] = cpu_clk;
+	}
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
Signature · 1 line
2.5.0
#1 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Greg,

Quoted 1 line · Gregory CLEMENT

of_clk_get() is called armada_xp_smp_prepare_cpus() function.

Hum? Your code is changing things to have of_clk_get() called by armada_xp_smp_prepare_cpus(). I think you wanted to say:

"of_clk_get() is currently called in get_cpu_clk(), which it turns is called by set_secondary_cpu_clock() during armada_xp_boot_secondary(). However, this callback is called in a context...

Quoted 7 lines

This
callback can be called in a context where it should not sleep as pointed when enabling CONFIG_DEBUG_ATOMIC_SLEEP. However of_clk_get() can sleep.

This patch solved this issue by moving the of_clk_get() during initialization by gathering the list of reference on the clock, and only access to this list later.

I think this doesn't solve the problem completely, because set_secondary_cpu_clock() is still calling clk_prepare_enable(), and the prepare step of a clock is theoretically allowed to sleep. In practice, it doesn't sleep in our specific case, but still clk_prepare_enable() is not good in an atomic context.

So I believe the whole approach of setting the CPU clock frequency in armada_xp_boot_secondary() is wrong, and we should basically revert my commit b9b1de0f4da37dac76d812a27d6065eba02dc05b, and instead find a different solution for the resume from suspend case.

Quoted 3 lines
Fixes f6cec7cd0777 ("ARM: mvebu: remove device tree parsing for cpu

nodes")

I don't think it fixes this commit. The one it really fixes is most probably b9b1de0f4da37dac76d812a27d6065eba02dc05b.

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#2 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Hi Thomas,

Quoted 11 lines · Thomas Petazzoni

Greg,

Quoted 1 line · Gregory CLEMENT

of_clk_get() is called armada_xp_smp_prepare_cpus() function.

Hum? Your code is changing things to have of_clk_get() called by armada_xp_smp_prepare_cpus(). I think you wanted to say:

"of_clk_get() is currently called in get_cpu_clk(), which it turns is called by set_secondary_cpu_clock() during armada_xp_boot_secondary(). However, this callback is called in a context...

right!

Quoted 9 lines
Quoted 8 lines

This
callback can be called in a context where it should not sleep as pointed when enabling CONFIG_DEBUG_ATOMIC_SLEEP. However of_clk_get() can sleep.

This patch solved this issue by moving the of_clk_get() during initialization by gathering the list of reference on the clock, and only access to this list later.

Quoted 5 lines

I think this doesn't solve the problem completely, because set_secondary_cpu_clock() is still calling clk_prepare_enable(), and the prepare step of a clock is theoretically allowed to sleep. In practice, it doesn't sleep in our specific case, but still clk_prepare_enable() is not good in an atomic context.

OK, so let's only use clk_enable() which can't sleep. On mvebu SoCs there is no prepare/unprepare operation for the clock, so trying to use clk_prepare_enable() is useless. Especially for this code which is not for a random IP but especially for the Armada XP SoC.

If in the future the same logic can be used for a new mvebu SoC which need the prepare/unprepare feature, then we will see how to deal with it, but I really doubt that it will happen.

Quoted 7 lines

So I believe the whole approach of setting the CPU clock frequency in armada_xp_boot_secondary() is wrong, and we should basically revert my commit b9b1de0f4da37dac76d812a27d6065eba02dc05b, and instead find a different solution for the resume from suspend case.

Quoted 1 line
Quoted 5 lines
Quoted 2 lines
Fixes f6cec7cd0777 ("ARM: mvebu: remove device tree parsing for cpu

nodes")

I don't think it fixes this commit. The one it really fixes is most probably b9b1de0f4da37dac76d812a27d6065eba02dc05b.

Yes to be honest I pick the closer commit on which this fix could apply without any conflict.

Thanks for your feedback, I will send a v2 taking into account your remarks.

Ggregory

Quoted 8 lines

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#3 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Hello,

Quoted 8 lines · Gregory CLEMENT

OK, so let's only use clk_enable() which can't sleep. On mvebu SoCs there is no prepare/unprepare operation for the clock, so trying to use clk_prepare_enable() is useless. Especially for this code which is not for a random IP but especially for the Armada XP SoC.

If in the future the same logic can be used for a new mvebu SoC which need the prepare/unprepare feature, then we will see how to deal with it, but I really doubt that it will happen.

I am not sure doing a clk_enable() without a clk_prepare() before it is legal, and that's why the clk_prepare_enable() helper is widely used.

Quoted 1 line

From the clk_prepare() comment:

""
In fact clk_prepare must be called before clk_enable. ""

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#4 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Hi Thomas,

Quoted 13 lines · Thomas Petazzoni

Hello,

Quoted 8 lines · Gregory CLEMENT

OK, so let's only use clk_enable() which can't sleep. On mvebu SoCs there is no prepare/unprepare operation for the clock, so trying to use clk_prepare_enable() is useless. Especially for this code which is not for a random IP but especially for the Armada XP SoC.

If in the future the same logic can be used for a new mvebu SoC which need the prepare/unprepare feature, then we will see how to deal with it, but I really doubt that it will happen.

Quoted 9 lines

I am not sure doing a clk_enable() without a clk_prepare() before it is legal, and that's why the clk_prepare_enable() helper is widely used.

From the clk_prepare() comment:

""
In fact clk_prepare must be called before clk_enable. ""

And from clk_enable comment we have:
""
clk_enable must not sleep, which differentiates it from clk_prepare. In a simple case, clk_enable can be used instead of clk_prepare to ungate a clk if the operation will never sleep.
""

Moreoever for me the "must" was to insist to the order of the call no to the fact that both must be called.

Gregory

Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#5 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Hello,

Quoted 9 lines · Gregory CLEMENT

And from clk_enable comment we have:
""
clk_enable must not sleep, which differentiates it from clk_prepare. In a simple case, clk_enable can be used instead of clk_prepare to ungate a clk if the operation will never sleep.
""

Moreoever for me the "must" was to insist to the order of the call no to the fact that both must be called.

Right make sense, thanks for correcting me on this. If that's OK from a clock maintainer point of view, I'm fine.

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#6 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Quoted 11 lines · Thomas Petazzoni

Hello,

Quoted 7 lines · Gregory CLEMENT

And from clk_enable comment we have:
""
clk_enable must not sleep, which differentiates it from clk_prepare. In a simple case, clk_enable can be used instead of clk_prepare to ungate a clk if the operation will never sleep.
""

Quoted 5 lines
Quoted 2 lines

Moreoever for me the "must" was to insist to the order of the call no to the fact that both must be called.

Right make sense, thanks for correcting me on this. If that's OK from a clock maintainer point of view, I'm fine.

The comment Gregory quoted is an _implementation_ comment: it's saying that you can implement the gating/ungating in either clk_prepare() or clk_enable().

It isn't giving permission for users of the clk API to omit these calls.

Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#7 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

In reply to Gregory CLEMENT
Quoted 9 lines · Gregory CLEMENT

And from clk_enable comment we have:
""
clk_enable must not sleep, which differentiates it from clk_prepare. In a simple case, clk_enable can be used instead of clk_prepare to ungate a clk if the operation will never sleep.
""

Moreoever for me the "must" was to insist to the order of the call no to the fact that both must be called.

As the author of the clk API, the idea here is that clk_prepare() should always be called _before_ clk_enable() for any clock: in other words, getting a clock and then calling clk_enable() on it is not legal.

CCF presently enforces this - clk_enable() without a preceding clk_prepare() will return -ESHUTDOWN.

Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
author
#8 lore raw

PATCH 1/1 ARM: mvebu: Fix of_clk_get() call in a non sleeping context

Hi Russell King,

Quoted 11 lines · Russell King - ARM Linux
Quoted 9 lines · Gregory CLEMENT

And from clk_enable comment we have:
""
clk_enable must not sleep, which differentiates it from clk_prepare. In a simple case, clk_enable can be used instead of clk_prepare to ungate a clk if the operation will never sleep.
""

Moreoever for me the "must" was to insist to the order of the call no to the fact that both must be called.

Quoted 7 lines

As the author of the clk API, the idea here is that clk_prepare() should always be called _before_ clk_enable() for any clock: in other words, getting a clock and then calling clk_enable() on it is not legal.

CCF presently enforces this - clk_enable() without a preceding clk_prepare() will return -ESHUTDOWN.

Thanks for the clarification, I will work on a alternative solutioin.

Gregory

Quoted 5 lines
Signature · 4 lines
-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com