[PATCH] clk: exynos: use irqsave version of spin_lock to avoid deadlock with irqs

[PATCH] clk: exynos: use irqsave version of spin_lock to avoid deadlock with irqs

[Marek Szyprowski]

It is allowed to enable/disable clocks from interrupts, so common Exynos
ARM clock management code for CPUfreq should use 'irqsave' version of
spin_lock calls to avoid potential deadlock caused by spin_lock recursion.
The same spin_lock is used by gate/mux clocks during enable/disable calls.

This deadlock, can be reproduced by enabling CPUfreq (ondemand or
userspace) and decoding video with s5p-mfc driver.

Relevant stack trace:
[ 5928.061534] BUG: spinlock recursion on CPU#0, bash/1252
[ 5928.061609]  lock: 0xee80454c, .magic: dead4ead, .owner: bash/1252, .owner_cpu: 0
[ 5928.068586] CPU: 0 PID: 1252 Comm: bash Tainted: G        W       4.4.0-rc4-00001-g447a7fd #678
[ 5928.077260] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 5928.083359] [<c00153dc>] (unwind_backtrace) from [<c00121b4>] (show_stack+0x10/0x14)
[ 5928.091072] [<c00121b4>] (show_stack) from [<c0213e28>] (dump_stack+0x68/0xb8)
[ 5928.098275] [<c0213e28>] (dump_stack) from [<c005c1ec>] (do_raw_spin_lock+0x184/0x1ac)
[ 5928.106177] [<c005c1ec>] (do_raw_spin_lock) from [<c05cfa98>] (_raw_spin_lock_irqsave+0x20/0x28)
[ 5928.114943] [<c05cfa98>] (_raw_spin_lock_irqsave) from [<c0468698>] (clk_gate_endisable+0x24/0x98)
[ 5928.123882] [<c0468698>] (clk_gate_endisable) from [<c0464d40>] (clk_core_disable+0x60/0x84)
[ 5928.132299] [<c0464d40>] (clk_core_disable) from [<c0465e00>] (clk_disable+0x24/0x30)
[ 5928.140117] [<c0465e00>] (clk_disable) from [<c03f3b80>] (s5p_mfc_handle_frame+0x254/0x860)
[ 5928.148445] [<c03f3b80>] (s5p_mfc_handle_frame) from [<c03f4b34>] (s5p_mfc_irq+0x890/0xa24)
[ 5928.156778] [<c03f4b34>] (s5p_mfc_irq) from [<c00627bc>] (handle_irq_event_percpu+0x50/0x14c)
[ 5928.165283] [<c00627bc>] (handle_irq_event_percpu) from [<c00628f0>] (handle_irq_event+0x38/0x5c)
[ 5928.174143] [<c00628f0>] (handle_irq_event) from [<c0065864>] (handle_fasteoi_irq+0xdc/0x1a4)
[ 5928.182645] [<c0065864>] (handle_fasteoi_irq) from [<c0062090>] (generic_handle_irq+0x18/0x28)
[ 5928.191236] [<c0062090>] (generic_handle_irq) from [<c00621a4>] (__handle_domain_irq+0x6c/0xdc)
[ 5928.199917] [<c00621a4>] (__handle_domain_irq) from [<c0009470>] (gic_handle_irq+0x4c/0x98)
[ 5928.208249] [<c0009470>] (gic_handle_irq) from [<c0012c54>] (__irq_svc+0x54/0x90)
[ 5928.215709] Exception stack(0xeddb5cb8 to 0xeddb5d00)
[ 5928.220745] 5ca0:                                                       ee80454c faddfadc
[ 5928.228906] 5cc0: 00000000 01000001 ee831ce0 f8114200 ee807c00 01130520 00000403 eddb5d84
[ 5928.237063] 5ce0: ee807c48 2faf0800 ee807c0c eddb5d08 c046b618 c046b634 20000053 ffffffff
[ 5928.245225] [<c0012c54>] (__irq_svc) from [<c046b634>] (exynos_cpuclk_notifier_cb+0x170/0x270)
[ 5928.253823] [<c046b634>] (exynos_cpuclk_notifier_cb) from [<c003cb58>] (notifier_call_chain+0x44/0x84)
[ 5928.263106] [<c003cb58>] (notifier_call_chain) from [<c003ccd4>] (__srcu_notifier_call_chain+0x6c/0x9c)
[ 5928.272480] [<c003ccd4>] (__srcu_notifier_call_chain) from [<c003cd1c>] (srcu_notifier_call_chain+0x18/0x20)
[ 5928.282288] [<c003cd1c>] (srcu_notifier_call_chain) from [<c0464ed0>] (__clk_notify+0x6c/0x74)
[ 5928.290881] [<c0464ed0>] (__clk_notify) from [<c0465388>] (clk_propagate_rate_change+0xa0/0xac)
[ 5928.299561] [<c0465388>] (clk_propagate_rate_change) from [<c0465378>] (clk_propagate_rate_change+0x90/0xac)
[ 5928.309370] [<c0465378>] (clk_propagate_rate_change) from [<c04666fc>] (clk_core_set_rate_nolock+0x64/0xa8)
[ 5928.319091] [<c04666fc>] (clk_core_set_rate_nolock) from [<c0466760>] (clk_set_rate+0x20/0x30)
[ 5928.327686] [<c0466760>] (clk_set_rate) from [<c0428c70>] (set_target+0xe8/0x23c)
[ 5928.335152] [<c0428c70>] (set_target) from [<c04244d0>] (__cpufreq_driver_target+0x184/0x29c)
[ 5928.343655] [<c04244d0>] (__cpufreq_driver_target) from [<c0427128>] (cpufreq_set+0x44/0x64)
[ 5928.352074] [<c0427128>] (cpufreq_set) from [<c0423948>] (store_scaling_setspeed+0x5c/0x74)
[ 5928.360407] [<c0423948>] (store_scaling_setspeed) from [<c04238d0>] (store+0x7c/0x98)
[ 5928.368221] [<c04238d0>] (store) from [<c0132540>] (sysfs_kf_write+0x44/0x48)
[ 5928.375338] [<c0132540>] (sysfs_kf_write) from [<c0131b9c>] (kernfs_fop_write+0xb8/0x1bc)
[ 5928.383496] [<c0131b9c>] (kernfs_fop_write) from [<c00d71f8>] (__vfs_write+0x2c/0xd4)
[ 5928.391308] [<c00d71f8>] (__vfs_write) from [<c00d7de8>] (vfs_write+0xa0/0x144)
[ 5928.398598] [<c00d7de8>] (vfs_write) from [<c00d8048>] (SyS_write+0x44/0x84)
[ 5928.405631] [<c00d8048>] (SyS_write) from [<c000f540>] (ret_fast_syscall+0x0/0x3c)

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
CC: stable@vger.kernel.org  # v4.2+
---
 drivers/clk/samsung/clk-cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
index 2fe37f708dc7..813003d6ce09 100644
--- a/drivers/clk/samsung/clk-cpu.c
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -148,6 +148,7 @@ static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
 	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
 	unsigned long div0, div1 = 0, mux_reg;
+	unsigned long flags;
 
 	/* find out the divider values to use for clock data */
 	while ((cfg_data->prate * 1000) != ndata->new_rate) {
@@ -156,7 +157,7 @@ static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 		cfg_data++;
 	}
 
-	spin_lock(cpuclk->lock);
+	spin_lock_irqsave(cpuclk->lock, flags);
 
 	/*
 	 * For the selected PLL clock frequency, get the pre-defined divider
@@ -212,7 +213,7 @@ static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 				DIV_MASK_ALL);
 	}
 
-	spin_unlock(cpuclk->lock);
+	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
 }
 
@@ -223,6 +224,7 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
 	unsigned long div = 0, div_mask = DIV_MASK;
 	unsigned long mux_reg;
+	unsigned long flags;
 
 	/* find out the divider values to use for clock data */
 	if (cpuclk->flags & CLK_CPU_NEEDS_DEBUG_ALT_DIV) {
@@ -233,7 +235,7 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 		}
 	}
 
-	spin_lock(cpuclk->lock);
+	spin_lock_irqsave(cpuclk->lock, flags);
 
 	/* select mout_apll as the alternate parent */
 	mux_reg = readl(base + E4210_SRC_CPU);
@@ -246,7 +248,7 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 	}
 
 	exynos_set_safe_div(base, div, div_mask);
-	spin_unlock(cpuclk->lock);
+	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
 }
 
-- 
1.9.2

Re: [PATCH] clk: exynos: use irqsave version of spin_lock to avoid deadlock with irqs

[Tomasz Figa]

Hi Marek,

2015-12-11 23:38 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> It is allowed to enable/disable clocks from interrupts, so common Exynos
> ARM clock management code for CPUfreq should use 'irqsave' version of
> spin_lock calls to avoid potential deadlock caused by spin_lock recursion.
> The same spin_lock is used by gate/mux clocks during enable/disable calls.
>
> This deadlock, can be reproduced by enabling CPUfreq (ondemand or
> userspace) and decoding video with s5p-mfc driver.

Good catch.

Acked-by: Tomasz Figa <tomasz.figa@gmail.com>

Best regards,
Tomasz

Re: [PATCH] clk: exynos: use irqsave version of spin_lock to avoid deadlock with irqs

[Sylwester Nawrocki]

On 11/12/15 15:38, Marek Szyprowski wrote:
> It is allowed to enable/disable clocks from interrupts, so common Exynos
> ARM clock management code for CPUfreq should use 'irqsave' version of
> spin_lock calls to avoid potential deadlock caused by spin_lock recursion.
> The same spin_lock is used by gate/mux clocks during enable/disable calls.
> 
> This deadlock, can be reproduced by enabling CPUfreq (ondemand or
> userspace) and decoding video with s5p-mfc driver.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> CC: stable@vger.kernel.org  # v4.2+

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Mike, Stephen, could you apply this patch directly?
It would be nice to have it in 4.4 as the bug fixed here causes
some of exynos boards in mainline to fail booting with default
config. I could resend the patch directly to you if needed.

Thanks,
Sylwester

> ---
>  drivers/clk/samsung/clk-cpu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> index 2fe37f708dc7..813003d6ce09 100644
> --- a/drivers/clk/samsung/clk-cpu.c
> +++ b/drivers/clk/samsung/clk-cpu.c
> @@ -148,6 +148,7 @@ static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
>  	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
>  	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
>  	unsigned long div0, div1 = 0, mux_reg;
> +	unsigned long flags;
>  
>  	/* find out the divider values to use for clock data */
>  	while ((cfg_data->prate * 1000) != ndata->new_rate) {
> @@ -156,7 +157,7 @@ static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
>  		cfg_data++;
>  	}
>  
> -	spin_lock(cpuclk->lock);
> +	spin_lock_irqsave(cpuclk->lock, flags);
>  
>  	/*
>  	 * For the selected PLL clock frequency, get the pre-defined divider
> @@ -212,7 +213,7 @@ static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
>  				DIV_MASK_ALL);
>  	}
>  
> -	spin_unlock(cpuclk->lock);
> +	spin_unlock_irqrestore(cpuclk->lock, flags);
>  	return 0;
>  }
>  
> @@ -223,6 +224,7 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
>  	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
>  	unsigned long div = 0, div_mask = DIV_MASK;
>  	unsigned long mux_reg;
> +	unsigned long flags;
>  
>  	/* find out the divider values to use for clock data */
>  	if (cpuclk->flags & CLK_CPU_NEEDS_DEBUG_ALT_DIV) {
> @@ -233,7 +235,7 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
>  		}
>  	}
>  
> -	spin_lock(cpuclk->lock);
> +	spin_lock_irqsave(cpuclk->lock, flags);
>  
>  	/* select mout_apll as the alternate parent */
>  	mux_reg = readl(base + E4210_SRC_CPU);
> @@ -246,7 +248,7 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
>  	}
>  
>  	exynos_set_safe_div(base, div, div_mask);
> -	spin_unlock(cpuclk->lock);
> +	spin_unlock_irqrestore(cpuclk->lock, flags);
>  	return 0;
>  }