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

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

[Gregory CLEMENT]

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: <stable@vger.kernel.org>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/platsmp.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-mvebu/platsmp.c b/arch/arm/mach-mvebu/platsmp.c
index f9597b701028..33ef7f9f4ef2 100644
--- a/arch/arm/mach-mvebu/platsmp.c
+++ b/arch/arm/mach-mvebu/platsmp.c
@@ -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
-- 
2.5.0

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

[Thomas Petazzoni]

Greg,

On Tue,  8 Mar 2016 15:18:53 +0100, Gregory CLEMENT wrote:
> 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...

> 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.

> 
> 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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., mars 08 2016, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Greg,
>
> On Tue,  8 Mar 2016 15:18:53 +0100, Gregory CLEMENT wrote:
>> 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!

>
>> 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.

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.


>
> 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.
>
>> 
>> 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


>
> Best regards,
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

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

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

[Thomas Petazzoni]

Hello,

On Tue, 08 Mar 2016 17:19:39 +0100, Gregory CLEMENT wrote:

> 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.

>From the clk_prepare() comment:

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

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., mars 08 2016, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Hello,
>
> On Tue, 08 Mar 2016 17:19:39 +0100, Gregory CLEMENT wrote:
>
>> 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.
>
> 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

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

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

[Thomas Petazzoni]

Hello,

On Tue, 08 Mar 2016 17:38:11 +0100, Gregory CLEMENT wrote:

> 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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Russell King - ARM Linux]

On Tue, Mar 08, 2016 at 05:38:11PM +0100, Gregory CLEMENT wrote:
> 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.

-- 
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.

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

[Russell King - ARM Linux]

On Tue, Mar 08, 2016 at 05:51:47PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 08 Mar 2016 17:38:11 +0100, Gregory CLEMENT wrote:
> 
> > 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.

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.

-- 
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.

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

[Gregory CLEMENT]

Hi Russell King,
 
 On mar., mars 08 2016, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Mar 08, 2016 at 05:38:11PM +0100, Gregory CLEMENT wrote:
>> 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.

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

Gregory

>
> -- 
> 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.

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