All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
       [not found] <CGME20180115115231eucas1p1499f46ca64b65598d03a182ed5670f28@eucas1p1.samsung.com>
@ 2018-01-15 11:52   ` Marek Szyprowski
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-15 11:52 UTC (permalink / raw)
  To: linux-clk, linux-arm-kernel
  Cc: Marek Szyprowski, Stephen Boyd, Michael Turquette, Shawn Guo,
	Dong Aisheng, Bartlomiej Zolnierkiewicz

If orphaned clock has been already prepared/enabled (for example if it or
one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
counters of the newly assigned parent are not updated correctly. This
might later cause warnings during changing clock parents.

This patch fixes following warning on Exynos5422-based boards:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811 clk_core_disable_lock+0x18/0x24
Modules linked in:
CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115 #106
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: pm genpd_power_off_work_fn
[<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
[<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
[<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
[<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
[<c01250cc>] (warn_slowpath_null) from [<c048d584>] (clk_core_disable_lock+0x18/0x24)
[<c048d584>] (clk_core_disable_lock) from [<c048e490>] (clk_core_disable_unprepare+0xc/0x20)
[<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
[<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
[<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
[<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
[<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
[<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
[<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
[<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
[<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
[<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xeeb01fb0 to 0xeeb01ff8)
1fa0:                                     00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 503c239fb760f17a ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684 clk_core_disable_unprepare+0x18/0x20
Modules linked in:
CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-20180115 #106
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: pm genpd_power_off_work_fn
[<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
[<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
[<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
[<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
[<c01250cc>] (warn_slowpath_null) from [<c048e49c>] (clk_core_disable_unprepare+0x18/0x20)
[<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
[<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
[<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
[<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
[<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
[<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
[<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
[<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
[<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
[<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xeeb01fb0 to 0xeeb01ff8)
1fa0:                                     00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 503c239fb760f17b ]---
------------[ cut here ]------------

Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0f686a9dac3e..d33c087d7868 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
 		if (parent) {
 			/* update the clk tree topology */
 			flags = clk_enable_lock();
+			if (orphan->prepare_count)
+				clk_core_prepare(parent);
+			if (orphan->enable_count)
+				clk_core_enable(parent);
 			clk_reparent(orphan, parent);
 			clk_enable_unlock(flags);
 			__clk_recalc_accuracies(orphan);
-- 
2.15.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-15 11:52   ` Marek Szyprowski
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-15 11:52 UTC (permalink / raw)
  To: linux-arm-kernel

If orphaned clock has been already prepared/enabled (for example if it or
one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
counters of the newly assigned parent are not updated correctly. This
might later cause warnings during changing clock parents.

This patch fixes following warning on Exynos5422-based boards:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811 clk_core_disable_lock+0x18/0x24
Modules linked in:
CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115 #106
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: pm genpd_power_off_work_fn
[<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
[<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
[<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
[<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
[<c01250cc>] (warn_slowpath_null) from [<c048d584>] (clk_core_disable_lock+0x18/0x24)
[<c048d584>] (clk_core_disable_lock) from [<c048e490>] (clk_core_disable_unprepare+0xc/0x20)
[<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
[<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
[<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
[<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
[<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
[<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
[<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
[<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
[<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
[<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xeeb01fb0 to 0xeeb01ff8)
1fa0:                                     00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 503c239fb760f17a ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684 clk_core_disable_unprepare+0x18/0x20
Modules linked in:
CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-20180115 #106
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
Workqueue: pm genpd_power_off_work_fn
[<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
[<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
[<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
[<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
[<c01250cc>] (warn_slowpath_null) from [<c048e49c>] (clk_core_disable_unprepare+0x18/0x20)
[<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
[<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
[<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
[<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
[<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
[<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
[<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
[<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
[<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
[<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xeeb01fb0 to 0xeeb01ff8)
1fa0:                                     00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 503c239fb760f17b ]---
------------[ cut here ]------------

Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/clk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0f686a9dac3e..d33c087d7868 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
 		if (parent) {
 			/* update the clk tree topology */
 			flags = clk_enable_lock();
+			if (orphan->prepare_count)
+				clk_core_prepare(parent);
+			if (orphan->enable_count)
+				clk_core_enable(parent);
 			clk_reparent(orphan, parent);
 			clk_enable_unlock(flags);
 			__clk_recalc_accuracies(orphan);
-- 
2.15.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
  2018-01-15 11:52   ` Marek Szyprowski
@ 2018-01-17  8:05     ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2018-01-17  8:05 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-clk, linux-arm-kernel, Stephen Boyd, Michael Turquette,
	Shawn Guo, Dong Aisheng, Bartlomiej Zolnierkiewicz

On Mon, Jan 15, 2018 at 12:52 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> If orphaned clock has been already prepared/enabled (for example if it or
> one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
> counters of the newly assigned parent are not updated correctly. This
> might later cause warnings during changing clock parents.
>

Tested-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-17  8:05     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2018-01-17  8:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jan 15, 2018 at 12:52 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> If orphaned clock has been already prepared/enabled (for example if it or
> one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
> counters of the newly assigned parent are not updated correctly. This
> might later cause warnings during changing clock parents.
>

Tested-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
  2018-01-15 11:52   ` Marek Szyprowski
@ 2018-01-25 22:19     ` Stephen Boyd
  -1 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2018-01-25 22:19 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-clk, linux-arm-kernel, Michael Turquette, Shawn Guo,
	Dong Aisheng, Bartlomiej Zolnierkiewicz

On 01/15, Marek Szyprowski wrote:
> If orphaned clock has been already prepared/enabled (for example if it or
> one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
> counters of the newly assigned parent are not updated correctly. This
> might later cause warnings during changing clock parents.
> 
> This patch fixes following warning on Exynos5422-based boards:
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811 clk_core_disable_lock+0x18/0x24
> Modules linked in:
> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115 #106
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: pm genpd_power_off_work_fn
> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
> [<c01250cc>] (warn_slowpath_null) from [<c048d584>] (clk_core_disable_lock+0x18/0x24)
> [<c048d584>] (clk_core_disable_lock) from [<c048e490>] (clk_core_disable_unprepare+0xc/0x20)
> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
> 1fa0:                                     00000000 00000000 00000000 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 503c239fb760f17a ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684 clk_core_disable_unprepare+0x18/0x20
> Modules linked in:
> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-20180115 #106
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: pm genpd_power_off_work_fn
> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
> [<c01250cc>] (warn_slowpath_null) from [<c048e49c>] (clk_core_disable_unprepare+0x18/0x20)
> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
> 1fa0:                                     00000000 00000000 00000000 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 503c239fb760f17b ]---
> ------------[ cut here ]------------
> 
> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/clk.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0f686a9dac3e..d33c087d7868 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
>  		if (parent) {
>  			/* update the clk tree topology */
>  			flags = clk_enable_lock();

This is the spinlock?

> +			if (orphan->prepare_count)
> +				clk_core_prepare(parent);

And this is prepare mutex? Doesn't sound like it will work.

> +			if (orphan->enable_count)
> +				clk_core_enable(parent);

This would be OK, but then we might touch the hardware again?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-25 22:19     ` Stephen Boyd
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2018-01-25 22:19 UTC (permalink / raw)
  To: linux-arm-kernel

On 01/15, Marek Szyprowski wrote:
> If orphaned clock has been already prepared/enabled (for example if it or
> one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
> counters of the newly assigned parent are not updated correctly. This
> might later cause warnings during changing clock parents.
> 
> This patch fixes following warning on Exynos5422-based boards:
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811 clk_core_disable_lock+0x18/0x24
> Modules linked in:
> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115 #106
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: pm genpd_power_off_work_fn
> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
> [<c01250cc>] (warn_slowpath_null) from [<c048d584>] (clk_core_disable_lock+0x18/0x24)
> [<c048d584>] (clk_core_disable_lock) from [<c048e490>] (clk_core_disable_unprepare+0xc/0x20)
> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
> 1fa0:                                     00000000 00000000 00000000 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 503c239fb760f17a ]---
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684 clk_core_disable_unprepare+0x18/0x20
> Modules linked in:
> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-20180115 #106
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: pm genpd_power_off_work_fn
> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
> [<c01250cc>] (warn_slowpath_null) from [<c048e49c>] (clk_core_disable_unprepare+0x18/0x20)
> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
> 1fa0:                                     00000000 00000000 00000000 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 503c239fb760f17b ]---
> ------------[ cut here ]------------
> 
> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/clk.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0f686a9dac3e..d33c087d7868 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
>  		if (parent) {
>  			/* update the clk tree topology */
>  			flags = clk_enable_lock();

This is the spinlock?

> +			if (orphan->prepare_count)
> +				clk_core_prepare(parent);

And this is prepare mutex? Doesn't sound like it will work.

> +			if (orphan->enable_count)
> +				clk_core_enable(parent);

This would be OK, but then we might touch the hardware again?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
  2018-01-25 22:19     ` Stephen Boyd
@ 2018-01-26  7:31       ` Marek Szyprowski
  -1 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-26  7:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-clk, linux-arm-kernel, Michael Turquette, Shawn Guo,
	Dong Aisheng, Bartlomiej Zolnierkiewicz

Hi Stephen,

On 2018-01-25 23:19, Stephen Boyd wrote:
> On 01/15, Marek Szyprowski wrote:
>> If orphaned clock has been already prepared/enabled (for example if it or
>> one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
>> counters of the newly assigned parent are not updated correctly. This
>> might later cause warnings during changing clock parents.
>>
>> This patch fixes following warning on Exynos5422-based boards:
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811 clk_core_disable_lock+0x18/0x24
>> Modules linked in:
>> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115 #106
>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> Workqueue: pm genpd_power_off_work_fn
>> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
>> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
>> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
>> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
>> [<c01250cc>] (warn_slowpath_null) from [<c048d584>] (clk_core_disable_lock+0x18/0x24)
>> [<c048d584>] (clk_core_disable_lock) from [<c048e490>] (clk_core_disable_unprepare+0xc/0x20)
>> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
>> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
>> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
>> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
>> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
>> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
>> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
>> 1fa0:                                     00000000 00000000 00000000 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> ---[ end trace 503c239fb760f17a ]---
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684 clk_core_disable_unprepare+0x18/0x20
>> Modules linked in:
>> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-20180115 #106
>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> Workqueue: pm genpd_power_off_work_fn
>> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
>> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
>> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
>> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
>> [<c01250cc>] (warn_slowpath_null) from [<c048e49c>] (clk_core_disable_unprepare+0x18/0x20)
>> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
>> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
>> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
>> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
>> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
>> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
>> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
>> 1fa0:                                     00000000 00000000 00000000 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> ---[ end trace 503c239fb760f17b ]---
>> ------------[ cut here ]------------
>>
>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/clk/clk.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 0f686a9dac3e..d33c087d7868 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
>>   		if (parent) {
>>   			/* update the clk tree topology */
>>   			flags = clk_enable_lock();
> This is the spinlock?

yes

>
>> +			if (orphan->prepare_count)
>> +				clk_core_prepare(parent);
> And this is prepare mutex? Doesn't sound like it will work.

Prepare mutex is taken at the beginning of __clk_core_init() function, so
everything is fine here.

>> +			if (orphan->enable_count)
>> +				clk_core_enable(parent);
> This would be OK, but then we might touch the hardware again?

Well, it will touch hardware in one case: if orphaned clock is already
prepared and enabled (for example due to having CLK_IS_CRITICAL flag)
and the newly registered parent is not (yet?) enabled. Then it will
prepare and enable this new parent clock, what is a desired behavior.

This code will not temporarily disable any clocks, what I assume was the
main reason for the patch mentioned in the commit message.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-26  7:31       ` Marek Szyprowski
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-26  7:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Stephen,

On 2018-01-25 23:19, Stephen Boyd wrote:
> On 01/15, Marek Szyprowski wrote:
>> If orphaned clock has been already prepared/enabled (for example if it or
>> one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
>> counters of the newly assigned parent are not updated correctly. This
>> might later cause warnings during changing clock parents.
>>
>> This patch fixes following warning on Exynos5422-based boards:
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811 clk_core_disable_lock+0x18/0x24
>> Modules linked in:
>> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115 #106
>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> Workqueue: pm genpd_power_off_work_fn
>> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
>> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
>> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
>> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
>> [<c01250cc>] (warn_slowpath_null) from [<c048d584>] (clk_core_disable_lock+0x18/0x24)
>> [<c048d584>] (clk_core_disable_lock) from [<c048e490>] (clk_core_disable_unprepare+0xc/0x20)
>> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
>> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
>> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
>> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
>> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
>> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
>> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
>> 1fa0:                                     00000000 00000000 00000000 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> ---[ end trace 503c239fb760f17a ]---
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684 clk_core_disable_unprepare+0x18/0x20
>> Modules linked in:
>> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-20180115 #106
>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> Workqueue: pm genpd_power_off_work_fn
>> [<c0111504>] (unwind_backtrace) from [<c010dbec>] (show_stack+0x10/0x14)
>> [<c010dbec>] (show_stack) from [<c09b3f34>] (dump_stack+0x90/0xc8)
>> [<c09b3f34>] (dump_stack) from [<c0125060>] (__warn+0xe4/0x110)
>> [<c0125060>] (__warn) from [<c01250cc>] (warn_slowpath_null+0x40/0x48)
>> [<c01250cc>] (warn_slowpath_null) from [<c048e49c>] (clk_core_disable_unprepare+0x18/0x20)
>> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>] (__clk_set_parent_after+0x48/0x4c)
>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>] (clk_core_set_parent_nolock+0x224/0x5b4)
>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>] (clk_set_parent+0x38/0x6c)
>> [<c048f23c>] (clk_set_parent) from [<c049c41c>] (exynos_pd_power+0x74/0x1cc)
>> [<c049c41c>] (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>> [<c0550768>] (genpd_power_off) from [<c0550b34>] (genpd_power_off_work_fn+0x2c/0x40)
>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>] (process_one_work+0x1d0/0x7bc)
>> [<c01432e4>] (process_one_work) from [<c014393c>] (worker_thread+0x34/0x4dc)
>> [<c014393c>] (worker_thread) from [<c014a0d0>] (kthread+0x128/0x164)
>> [<c014a0d0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
>> Exception stack(0xeeb01fb0 to 0xeeb01ff8)
>> 1fa0:                                     00000000 00000000 00000000 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> ---[ end trace 503c239fb760f17b ]---
>> ------------[ cut here ]------------
>>
>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting during registration")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/clk/clk.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 0f686a9dac3e..d33c087d7868 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
>>   		if (parent) {
>>   			/* update the clk tree topology */
>>   			flags = clk_enable_lock();
> This is the spinlock?

yes

>
>> +			if (orphan->prepare_count)
>> +				clk_core_prepare(parent);
> And this is prepare mutex? Doesn't sound like it will work.

Prepare mutex is taken at the beginning of __clk_core_init() function, so
everything is fine here.

>> +			if (orphan->enable_count)
>> +				clk_core_enable(parent);
> This would be OK, but then we might touch the hardware again?

Well, it will touch hardware in one case: if orphaned clock is already
prepared and enabled (for example due to having CLK_IS_CRITICAL flag)
and the newly registered parent is not (yet?) enabled. Then it will
prepare and enable this new parent clock, what is a desired behavior.

This code will not temporarily disable any clocks, what I assume was the
main reason for the patch mentioned in the commit message.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 14+ messages in thread
* RE: [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
  2018-01-26  7:31       ` Marek Szyprowski
@ 2018-01-26  8:36         ` A.s. Dong
  -1 siblings, 0 replies; 14+ messages in thread
From: A.s. Dong @ 2018-01-26  8:36 UTC (permalink / raw)
  To: Marek Szyprowski, Stephen Boyd
  Cc: linux-clk, linux-arm-kernel, Michael Turquette, Shawn Guo,
	Bartlomiej Zolnierkiewicz, dl-linux-imx

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBNYXJlayBTenlwcm93c2tpIFtt
YWlsdG86bS5zenlwcm93c2tpQHNhbXN1bmcuY29tXQ0KPiBTZW50OiBGcmlkYXksIEphbnVhcnkg
MjYsIDIwMTggMzozMSBQTQ0KPiBUbzogU3RlcGhlbiBCb3lkIDxzYm95ZEBjb2RlYXVyb3JhLm9y
Zz4NCj4gQ2M6IGxpbnV4LWNsa0B2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWFybS1rZXJuZWxAbGlz
dHMuaW5mcmFkZWFkLm9yZzsgTWljaGFlbA0KPiBUdXJxdWV0dGUgPG10dXJxdWV0dGVAYmF5bGli
cmUuY29tPjsgU2hhd24gR3VvIDxzaGF3bmd1b0BrZXJuZWwub3JnPjsNCj4gQS5zLiBEb25nIDxh
aXNoZW5nLmRvbmdAbnhwLmNvbT47IEJhcnRsb21pZWogWm9sbmllcmtpZXdpY3oNCj4gPGIuem9s
bmllcmtpZUBzYW1zdW5nLmNvbT4NCj4gU3ViamVjdDogUmU6IFtQQVRDSF0gY2xrOiBQcm9wZXJs
eSB1cGRhdGUgcHJlcGFyZS9lbmFibGUgY291bnQgb24gb3JwaGFuDQo+IGNsb2NrIHJlcGFyZW50
DQo+IA0KPiBIaSBTdGVwaGVuLA0KPiANCj4gT24gMjAxOC0wMS0yNSAyMzoxOSwgU3RlcGhlbiBC
b3lkIHdyb3RlOg0KPiA+IE9uIDAxLzE1LCBNYXJlayBTenlwcm93c2tpIHdyb3RlOg0KPiA+PiBJ
ZiBvcnBoYW5lZCBjbG9jayBoYXMgYmVlbiBhbHJlYWR5IHByZXBhcmVkL2VuYWJsZWQgKGZvciBl
eGFtcGxlIGlmDQo+ID4+IGl0IG9yIG9uZSBvZiBpdHMgY2hpbGRyZW4gaGFzIENMS19JU19DUklU
SUNBTCBmbGFnKSwgdGhlbiB0aGUNCj4gPj4gcHJlcGFyZS9lbmFibGUgY291bnRlcnMgb2YgdGhl
IG5ld2x5IGFzc2lnbmVkIHBhcmVudCBhcmUgbm90IHVwZGF0ZWQNCj4gPj4gY29ycmVjdGx5LiBU
aGlzIG1pZ2h0IGxhdGVyIGNhdXNlIHdhcm5pbmdzIGR1cmluZyBjaGFuZ2luZyBjbG9jayBwYXJl
bnRzLg0KPiA+Pg0KPiA+PiBUaGlzIHBhdGNoIGZpeGVzIGZvbGxvd2luZyB3YXJuaW5nIG9uIEV4
eW5vczU0MjItYmFzZWQgYm9hcmRzOg0KPiA+PiAtLS0tLS0tLS0tLS1bIGN1dCBoZXJlIF0tLS0t
LS0tLS0tLS0NCj4gPj4gV0FSTklORzogQ1BVOiAwIFBJRDogNTkgYXQgZHJpdmVycy9jbGsvY2xr
LmM6ODExDQo+ID4+IGNsa19jb3JlX2Rpc2FibGVfbG9jaysweDE4LzB4MjQgTW9kdWxlcyBsaW5r
ZWQgaW46DQo+ID4+IENQVTogMCBQSUQ6IDU5IENvbW06IGt3b3JrZXIvMDoxIE5vdCB0YWludGVk
IDQuMTUuMC1yYzctbmV4dC0yMDE4MDExNQ0KPiA+PiAjMTA2IEhhcmR3YXJlIG5hbWU6IFNBTVNV
TkcgRVhZTk9TIChGbGF0dGVuZWQgRGV2aWNlIFRyZWUpDQo+ID4+IFdvcmtxdWV1ZTogcG0gZ2Vu
cGRfcG93ZXJfb2ZmX3dvcmtfZm4gWzxjMDExMTUwND5dDQo+ICh1bndpbmRfYmFja3RyYWNlKQ0K
PiA+PiBmcm9tIFs8YzAxMGRiZWM+XSAoc2hvd19zdGFjaysweDEwLzB4MTQpIFs8YzAxMGRiZWM+
XSAoc2hvd19zdGFjaykNCj4gPj4gZnJvbSBbPGMwOWIzZjM0Pl0gKGR1bXBfc3RhY2srMHg5MC8w
eGM4KSBbPGMwOWIzZjM0Pl0gKGR1bXBfc3RhY2spDQo+ID4+IGZyb20gWzxjMDEyNTA2MD5dIChf
X3dhcm4rMHhlNC8weDExMCkgWzxjMDEyNTA2MD5dIChfX3dhcm4pIGZyb20NCj4gPj4gWzxjMDEy
NTBjYz5dICh3YXJuX3Nsb3dwYXRoX251bGwrMHg0MC8weDQ4KSBbPGMwMTI1MGNjPl0NCj4gPj4g
KHdhcm5fc2xvd3BhdGhfbnVsbCkgZnJvbSBbPGMwNDhkNTg0Pl0NCj4gPj4gKGNsa19jb3JlX2Rp
c2FibGVfbG9jaysweDE4LzB4MjQpIFs8YzA0OGQ1ODQ+XQ0KPiA+PiAoY2xrX2NvcmVfZGlzYWJs
ZV9sb2NrKSBmcm9tIFs8YzA0OGU0OTA+XQ0KPiA+PiAoY2xrX2NvcmVfZGlzYWJsZV91bnByZXBh
cmUrMHhjLzB4MjApDQo+ID4+IFs8YzA0OGU0OTA+XSAoY2xrX2NvcmVfZGlzYWJsZV91bnByZXBh
cmUpIGZyb20gWzxjMDQ4ZWE2MD5dDQo+ID4+IChfX2Nsa19zZXRfcGFyZW50X2FmdGVyKzB4NDgv
MHg0YykNCj4gPj4gWzxjMDQ4ZWE2MD5dIChfX2Nsa19zZXRfcGFyZW50X2FmdGVyKSBmcm9tIFs8
YzA0OGVjODg+XQ0KPiA+PiAoY2xrX2NvcmVfc2V0X3BhcmVudF9ub2xvY2srMHgyMjQvMHg1YjQp
DQo+ID4+IFs8YzA0OGVjODg+XSAoY2xrX2NvcmVfc2V0X3BhcmVudF9ub2xvY2spIGZyb20gWzxj
MDQ4ZjIzYz5dDQo+ID4+IChjbGtfc2V0X3BhcmVudCsweDM4LzB4NmMpIFs8YzA0OGYyM2M+XSAo
Y2xrX3NldF9wYXJlbnQpIGZyb20NCj4gPj4gWzxjMDQ5YzQxYz5dIChleHlub3NfcGRfcG93ZXIr
MHg3NC8weDFjYykgWzxjMDQ5YzQxYz5dDQo+ID4+IChleHlub3NfcGRfcG93ZXIpIGZyb20gWzxj
MDU1MDc2OD5dIChnZW5wZF9wb3dlcl9vZmYrMHgxNjQvMHgyNjQpDQo+ID4+IFs8YzA1NTA3Njg+
XSAoZ2VucGRfcG93ZXJfb2ZmKSBmcm9tIFs8YzA1NTBiMzQ+XQ0KPiA+PiAoZ2VucGRfcG93ZXJf
b2ZmX3dvcmtfZm4rMHgyYy8weDQwKQ0KPiA+PiBbPGMwNTUwYjM0Pl0gKGdlbnBkX3Bvd2VyX29m
Zl93b3JrX2ZuKSBmcm9tIFs8YzAxNDMyZTQ+XQ0KPiA+PiAocHJvY2Vzc19vbmVfd29yaysweDFk
MC8weDdiYykgWzxjMDE0MzJlND5dIChwcm9jZXNzX29uZV93b3JrKSBmcm9tDQo+ID4+IFs8YzAx
NDM5M2M+XSAod29ya2VyX3RocmVhZCsweDM0LzB4NGRjKSBbPGMwMTQzOTNjPl0gKHdvcmtlcl90
aHJlYWQpDQo+ID4+IGZyb20gWzxjMDE0YTBkMD5dIChrdGhyZWFkKzB4MTI4LzB4MTY0KSBbPGMw
MTRhMGQwPl0gKGt0aHJlYWQpIGZyb20NCj4gPj4gWzxjMDEwMTBiND5dIChyZXRfZnJvbV9mb3Jr
KzB4MTQvMHgyMCkgRXhjZXB0aW9uIHN0YWNrKDB4ZWViMDFmYjAgdG8NCj4gMHhlZWIwMWZmOCkN
Cj4gPj4gMWZhMDogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgMDAwMDAwMDAg
MDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAwMDANCj4gPj4gMWZjMDogMDAwMDAwMDAgMDAwMDAwMDAg
MDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAwMDANCj4gPj4gMDAwMDAw
MDANCj4gPj4gMWZlMDogMDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAwMDAgMDAwMDAw
MTMgMDAwMDAwMDAgLS0tWyBlbmQNCj4gPj4gdHJhY2UgNTAzYzIzOWZiNzYwZjE3YSBdLS0tIC0t
LS0tLS0tLS0tLVsgY3V0IGhlcmUgXS0tLS0tLS0tLS0tLQ0KPiA+PiBXQVJOSU5HOiBDUFU6IDAg
UElEOiA1OSBhdCBkcml2ZXJzL2Nsay9jbGsuYzo2ODQNCj4gPj4gY2xrX2NvcmVfZGlzYWJsZV91
bnByZXBhcmUrMHgxOC8weDIwDQo+ID4+IE1vZHVsZXMgbGlua2VkIGluOg0KPiA+PiBDUFU6IDAg
UElEOiA1OSBDb21tOiBrd29ya2VyLzA6MSBUYWludGVkOiBHICAgICAgICBXICAgICAgICA0LjE1
LjAtcmM3LW5leHQtDQo+IDIwMTgwMTE1ICMxMDYNCj4gPj4gSGFyZHdhcmUgbmFtZTogU0FNU1VO
RyBFWFlOT1MgKEZsYXR0ZW5lZCBEZXZpY2UgVHJlZSkNCj4gPj4gV29ya3F1ZXVlOiBwbSBnZW5w
ZF9wb3dlcl9vZmZfd29ya19mbiBbPGMwMTExNTA0Pl0NCj4gKHVud2luZF9iYWNrdHJhY2UpDQo+
ID4+IGZyb20gWzxjMDEwZGJlYz5dIChzaG93X3N0YWNrKzB4MTAvMHgxNCkgWzxjMDEwZGJlYz5d
IChzaG93X3N0YWNrKQ0KPiA+PiBmcm9tIFs8YzA5YjNmMzQ+XSAoZHVtcF9zdGFjaysweDkwLzB4
YzgpIFs8YzA5YjNmMzQ+XSAoZHVtcF9zdGFjaykNCj4gPj4gZnJvbSBbPGMwMTI1MDYwPl0gKF9f
d2FybisweGU0LzB4MTEwKSBbPGMwMTI1MDYwPl0gKF9fd2FybikgZnJvbQ0KPiA+PiBbPGMwMTI1
MGNjPl0gKHdhcm5fc2xvd3BhdGhfbnVsbCsweDQwLzB4NDgpIFs8YzAxMjUwY2M+XQ0KPiA+PiAo
d2Fybl9zbG93cGF0aF9udWxsKSBmcm9tIFs8YzA0OGU0OWM+XQ0KPiA+PiAoY2xrX2NvcmVfZGlz
YWJsZV91bnByZXBhcmUrMHgxOC8weDIwKQ0KPiA+PiBbPGMwNDhlNDljPl0gKGNsa19jb3JlX2Rp
c2FibGVfdW5wcmVwYXJlKSBmcm9tIFs8YzA0OGVhNjA+XQ0KPiA+PiAoX19jbGtfc2V0X3BhcmVu
dF9hZnRlcisweDQ4LzB4NGMpDQo+ID4+IFs8YzA0OGVhNjA+XSAoX19jbGtfc2V0X3BhcmVudF9h
ZnRlcikgZnJvbSBbPGMwNDhlYzg4Pl0NCj4gPj4gKGNsa19jb3JlX3NldF9wYXJlbnRfbm9sb2Nr
KzB4MjI0LzB4NWI0KQ0KPiA+PiBbPGMwNDhlYzg4Pl0gKGNsa19jb3JlX3NldF9wYXJlbnRfbm9s
b2NrKSBmcm9tIFs8YzA0OGYyM2M+XQ0KPiA+PiAoY2xrX3NldF9wYXJlbnQrMHgzOC8weDZjKSBb
PGMwNDhmMjNjPl0gKGNsa19zZXRfcGFyZW50KSBmcm9tDQo+ID4+IFs8YzA0OWM0MWM+XSAoZXh5
bm9zX3BkX3Bvd2VyKzB4NzQvMHgxY2MpIFs8YzA0OWM0MWM+XQ0KPiA+PiAoZXh5bm9zX3BkX3Bv
d2VyKSBmcm9tIFs8YzA1NTA3Njg+XSAoZ2VucGRfcG93ZXJfb2ZmKzB4MTY0LzB4MjY0KQ0KPiA+
PiBbPGMwNTUwNzY4Pl0gKGdlbnBkX3Bvd2VyX29mZikgZnJvbSBbPGMwNTUwYjM0Pl0NCj4gPj4g
KGdlbnBkX3Bvd2VyX29mZl93b3JrX2ZuKzB4MmMvMHg0MCkNCj4gPj4gWzxjMDU1MGIzND5dIChn
ZW5wZF9wb3dlcl9vZmZfd29ya19mbikgZnJvbSBbPGMwMTQzMmU0Pl0NCj4gPj4gKHByb2Nlc3Nf
b25lX3dvcmsrMHgxZDAvMHg3YmMpIFs8YzAxNDMyZTQ+XSAocHJvY2Vzc19vbmVfd29yaykgZnJv
bQ0KPiA+PiBbPGMwMTQzOTNjPl0gKHdvcmtlcl90aHJlYWQrMHgzNC8weDRkYykgWzxjMDE0Mzkz
Yz5dICh3b3JrZXJfdGhyZWFkKQ0KPiA+PiBmcm9tIFs8YzAxNGEwZDA+XSAoa3RocmVhZCsweDEy
OC8weDE2NCkgWzxjMDE0YTBkMD5dIChrdGhyZWFkKSBmcm9tDQo+ID4+IFs8YzAxMDEwYjQ+XSAo
cmV0X2Zyb21fZm9yaysweDE0LzB4MjApIEV4Y2VwdGlvbiBzdGFjaygweGVlYjAxZmIwIHRvDQo+
IDB4ZWViMDFmZjgpDQo+ID4+IDFmYTA6ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgIDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAwDQo+ID4+IDFmYzA6IDAwMDAw
MDAwIDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAw
DQo+ID4+IDAwMDAwMDAwDQo+ID4+IDFmZTA6IDAwMDAwMDAwIDAwMDAwMDAwIDAwMDAwMDAwIDAw
MDAwMDAwIDAwMDAwMDEzIDAwMDAwMDAwIC0tLVsgZW5kDQo+ID4+IHRyYWNlIDUwM2MyMzlmYjc2
MGYxN2IgXS0tLSAtLS0tLS0tLS0tLS1bIGN1dCBoZXJlIF0tLS0tLS0tLS0tLS0NCj4gPj4NCj4g
Pj4gRml4ZXM6IGY4ZjhmMWQwNDQ5NCAoImNsazogRG9uJ3QgdG91Y2ggaGFyZHdhcmUgd2hlbiBy
ZXBhcmVudGluZw0KPiA+PiBkdXJpbmcgcmVnaXN0cmF0aW9uIikNCj4gPj4gU2lnbmVkLW9mZi1i
eTogTWFyZWsgU3p5cHJvd3NraSA8bS5zenlwcm93c2tpQHNhbXN1bmcuY29tPg0KPiA+PiAtLS0N
Cj4gPj4gICBkcml2ZXJzL2Nsay9jbGsuYyB8IDQgKysrKw0KPiA+PiAgIDEgZmlsZSBjaGFuZ2Vk
LCA0IGluc2VydGlvbnMoKykNCj4gPj4NCj4gPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvY2xrL2Ns
ay5jIGIvZHJpdmVycy9jbGsvY2xrLmMgaW5kZXgNCj4gPj4gMGY2ODZhOWRhYzNlLi5kMzNjMDg3
ZDc4NjggMTAwNjQ0DQo+ID4+IC0tLSBhL2RyaXZlcnMvY2xrL2Nsay5jDQo+ID4+ICsrKyBiL2Ry
aXZlcnMvY2xrL2Nsay5jDQo+ID4+IEBAIC0yOTgyLDYgKzI5ODIsMTAgQEAgc3RhdGljIGludCBf
X2Nsa19jb3JlX2luaXQoc3RydWN0IGNsa19jb3JlICpjb3JlKQ0KPiA+PiAgIAkJaWYgKHBhcmVu
dCkgew0KPiA+PiAgIAkJCS8qIHVwZGF0ZSB0aGUgY2xrIHRyZWUgdG9wb2xvZ3kgKi8NCj4gPj4g
ICAJCQlmbGFncyA9IGNsa19lbmFibGVfbG9jaygpOw0KPiA+IFRoaXMgaXMgdGhlIHNwaW5sb2Nr
Pw0KPiANCj4geWVzDQo+IA0KPiA+DQo+ID4+ICsJCQlpZiAob3JwaGFuLT5wcmVwYXJlX2NvdW50
KQ0KPiA+PiArCQkJCWNsa19jb3JlX3ByZXBhcmUocGFyZW50KTsNCj4gPiBBbmQgdGhpcyBpcyBw
cmVwYXJlIG11dGV4PyBEb2Vzbid0IHNvdW5kIGxpa2UgaXQgd2lsbCB3b3JrLg0KPiANCj4gUHJl
cGFyZSBtdXRleCBpcyB0YWtlbiBhdCB0aGUgYmVnaW5uaW5nIG9mIF9fY2xrX2NvcmVfaW5pdCgp
IGZ1bmN0aW9uLCBzbw0KPiBldmVyeXRoaW5nIGlzIGZpbmUgaGVyZS4NCj4gDQoNClRoZW9yZXRp
Y2FsbHkgbm8gaXNzdWUgbWF5IGhhcHBlbiwganVzdCBsb29rcyBzdHJhbmdlIHRvIGNsYWltIG11
dGV4IHdpdGhpbg0Kc3BpbmxvY2suDQoNCj4gPj4gKwkJCWlmIChvcnBoYW4tPmVuYWJsZV9jb3Vu
dCkNCj4gPj4gKwkJCQljbGtfY29yZV9lbmFibGUocGFyZW50KTsNCj4gPiBUaGlzIHdvdWxkIGJl
IE9LLCBidXQgdGhlbiB3ZSBtaWdodCB0b3VjaCB0aGUgaGFyZHdhcmUgYWdhaW4/DQo+IA0KPiBX
ZWxsLCBpdCB3aWxsIHRvdWNoIGhhcmR3YXJlIGluIG9uZSBjYXNlOiBpZiBvcnBoYW5lZCBjbG9j
ayBpcyBhbHJlYWR5IHByZXBhcmVkDQo+IGFuZCBlbmFibGVkIChmb3IgZXhhbXBsZSBkdWUgdG8g
aGF2aW5nIENMS19JU19DUklUSUNBTCBmbGFnKSBhbmQgdGhlIG5ld2x5DQo+IHJlZ2lzdGVyZWQg
cGFyZW50IGlzIG5vdCAoeWV0PykgZW5hYmxlZC4gVGhlbiBpdCB3aWxsIHByZXBhcmUgYW5kIGVu
YWJsZSB0aGlzDQo+IG5ldyBwYXJlbnQgY2xvY2ssIHdoYXQgaXMgYSBkZXNpcmVkIGJlaGF2aW9y
Lg0KPiANCj4gVGhpcyBjb2RlIHdpbGwgbm90IHRlbXBvcmFyaWx5IGRpc2FibGUgYW55IGNsb2Nr
cywgd2hhdCBJIGFzc3VtZSB3YXMgdGhlIG1haW4NCj4gcmVhc29uIGZvciB0aGUgcGF0Y2ggbWVu
dGlvbmVkIGluIHRoZSBjb21taXQgbWVzc2FnZS4NCj4gDQoNClRoZW4gdGhpcyBjb2RlIHNlZW1z
IGJ5cGFzcyB0aGUgQ0xLX09QU19QQVJFTlRfRU5BQkxFIGZlYXR1cmUuDQoNClJlZ2FyZHMNCkRv
bmcgQWlzaGVuZw0KDQo+IEJlc3QgcmVnYXJkcw0KPiAtLQ0KPiBNYXJlayBTenlwcm93c2tpLCBQ
aEQNCj4gU2Ftc3VuZyBSJkQgSW5zdGl0dXRlIFBvbGFuZA0KDQo=

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-26  8:36         ` A.s. Dong
  0 siblings, 0 replies; 14+ messages in thread
From: A.s. Dong @ 2018-01-26  8:36 UTC (permalink / raw)
  To: linux-arm-kernel

> -----Original Message-----
> From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
> Sent: Friday, January 26, 2018 3:31 PM
> To: Stephen Boyd <sboyd@codeaurora.org>
> Cc: linux-clk at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Michael
> Turquette <mturquette@baylibre.com>; Shawn Guo <shawnguo@kernel.org>;
> A.s. Dong <aisheng.dong@nxp.com>; Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com>
> Subject: Re: [PATCH] clk: Properly update prepare/enable count on orphan
> clock reparent
> 
> Hi Stephen,
> 
> On 2018-01-25 23:19, Stephen Boyd wrote:
> > On 01/15, Marek Szyprowski wrote:
> >> If orphaned clock has been already prepared/enabled (for example if
> >> it or one of its children has CLK_IS_CRITICAL flag), then the
> >> prepare/enable counters of the newly assigned parent are not updated
> >> correctly. This might later cause warnings during changing clock parents.
> >>
> >> This patch fixes following warning on Exynos5422-based boards:
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811
> >> clk_core_disable_lock+0x18/0x24 Modules linked in:
> >> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115
> >> #106 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
> (unwind_backtrace)
> >> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
> >> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
> >> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
> >> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
> >> (warn_slowpath_null) from [<c048d584>]
> >> (clk_core_disable_lock+0x18/0x24) [<c048d584>]
> >> (clk_core_disable_lock) from [<c048e490>]
> >> (clk_core_disable_unprepare+0xc/0x20)
> >> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>]
> >> (__clk_set_parent_after+0x48/0x4c)
> >> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
> >> (clk_core_set_parent_nolock+0x224/0x5b4)
> >> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
> >> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
> >> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
> >> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
> >> [<c0550768>] (genpd_power_off) from [<c0550b34>]
> >> (genpd_power_off_work_fn+0x2c/0x40)
> >> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
> >> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
> >> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
> >> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
> >> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
> 0xeeb01ff8)
> >> 1fa0:                                     00000000 00000000 00000000 00000000
> >> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> >> 00000000
> >> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
> >> trace 503c239fb760f17a ]--- ------------[ cut here ]------------
> >> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684
> >> clk_core_disable_unprepare+0x18/0x20
> >> Modules linked in:
> >> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-
> 20180115 #106
> >> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
> (unwind_backtrace)
> >> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
> >> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
> >> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
> >> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
> >> (warn_slowpath_null) from [<c048e49c>]
> >> (clk_core_disable_unprepare+0x18/0x20)
> >> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>]
> >> (__clk_set_parent_after+0x48/0x4c)
> >> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
> >> (clk_core_set_parent_nolock+0x224/0x5b4)
> >> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
> >> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
> >> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
> >> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
> >> [<c0550768>] (genpd_power_off) from [<c0550b34>]
> >> (genpd_power_off_work_fn+0x2c/0x40)
> >> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
> >> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
> >> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
> >> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
> >> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
> 0xeeb01ff8)
> >> 1fa0:                                     00000000 00000000 00000000 00000000
> >> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> >> 00000000
> >> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
> >> trace 503c239fb760f17b ]--- ------------[ cut here ]------------
> >>
> >> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting
> >> during registration")
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>   drivers/clk/clk.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> >> 0f686a9dac3e..d33c087d7868 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
> >>   		if (parent) {
> >>   			/* update the clk tree topology */
> >>   			flags = clk_enable_lock();
> > This is the spinlock?
> 
> yes
> 
> >
> >> +			if (orphan->prepare_count)
> >> +				clk_core_prepare(parent);
> > And this is prepare mutex? Doesn't sound like it will work.
> 
> Prepare mutex is taken at the beginning of __clk_core_init() function, so
> everything is fine here.
> 

Theoretically no issue may happen, just looks strange to claim mutex within
spinlock.

> >> +			if (orphan->enable_count)
> >> +				clk_core_enable(parent);
> > This would be OK, but then we might touch the hardware again?
> 
> Well, it will touch hardware in one case: if orphaned clock is already prepared
> and enabled (for example due to having CLK_IS_CRITICAL flag) and the newly
> registered parent is not (yet?) enabled. Then it will prepare and enable this
> new parent clock, what is a desired behavior.
> 
> This code will not temporarily disable any clocks, what I assume was the main
> reason for the patch mentioned in the commit message.
> 

Then this code seems bypass the CLK_OPS_PARENT_ENABLE feature.

Regards
Dong Aisheng

> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
  2018-01-26  8:36         ` A.s. Dong
@ 2018-01-26  9:06           ` Marek Szyprowski
  -1 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-26  9:06 UTC (permalink / raw)
  To: A.s. Dong, Stephen Boyd
  Cc: linux-clk, linux-arm-kernel, Michael Turquette, Shawn Guo,
	Bartlomiej Zolnierkiewicz, dl-linux-imx

Hi Aisheng,

On 2018-01-26 09:36, A.s. Dong wrote:
>> -----Original Message-----
>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>> Sent: Friday, January 26, 2018 3:31 PM
>> To: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Michael
>> Turquette <mturquette@baylibre.com>; Shawn Guo <shawnguo@kernel.org>;
>> A.s. Dong <aisheng.dong@nxp.com>; Bartlomiej Zolnierkiewicz
>> <b.zolnierkie@samsung.com>
>> Subject: Re: [PATCH] clk: Properly update prepare/enable count on orphan
>> clock reparent
>>
>> Hi Stephen,
>>
>> On 2018-01-25 23:19, Stephen Boyd wrote:
>>> On 01/15, Marek Szyprowski wrote:
>>>> If orphaned clock has been already prepared/enabled (for example if
>>>> it or one of its children has CLK_IS_CRITICAL flag), then the
>>>> prepare/enable counters of the newly assigned parent are not updated
>>>> correctly. This might later cause warnings during changing clock parents.
>>>>
>>>> This patch fixes following warning on Exynos5422-based boards:
>>>> ------------[ cut here ]------------
>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811
>>>> clk_core_disable_lock+0x18/0x24 Modules linked in:
>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115
>>>> #106 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>> (unwind_backtrace)
>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>> (warn_slowpath_null) from [<c048d584>]
>>>> (clk_core_disable_lock+0x18/0x24) [<c048d584>]
>>>> (clk_core_disable_lock) from [<c048e490>]
>>>> (clk_core_disable_unprepare+0xc/0x20)
>>>> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>> (__clk_set_parent_after+0x48/0x4c)
>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>> 0xeeb01ff8)
>>>> 1fa0:                                     00000000 00000000 00000000 00000000
>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>> 00000000
>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>> trace 503c239fb760f17a ]--- ------------[ cut here ]------------
>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684
>>>> clk_core_disable_unprepare+0x18/0x20
>>>> Modules linked in:
>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-
>> 20180115 #106
>>>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>> (unwind_backtrace)
>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>> (warn_slowpath_null) from [<c048e49c>]
>>>> (clk_core_disable_unprepare+0x18/0x20)
>>>> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>> (__clk_set_parent_after+0x48/0x4c)
>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>> 0xeeb01ff8)
>>>> 1fa0:                                     00000000 00000000 00000000 00000000
>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>> 00000000
>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>> trace 503c239fb760f17b ]--- ------------[ cut here ]------------
>>>>
>>>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting
>>>> during registration")
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>    drivers/clk/clk.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
>>>> 0f686a9dac3e..d33c087d7868 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
>>>>    		if (parent) {
>>>>    			/* update the clk tree topology */
>>>>    			flags = clk_enable_lock();
>>> This is the spinlock?
>> yes
>>
>>>> +			if (orphan->prepare_count)
>>>> +				clk_core_prepare(parent);
>>> And this is prepare mutex? Doesn't sound like it will work.
>> Prepare mutex is taken at the beginning of __clk_core_init() function, so
>> everything is fine here.
>>
> Theoretically no issue may happen, just looks strange to claim mutex within
> spinlock.

Nope, the mutex is taken before entering this block of code. What is strange
then? clk_core_prepare() doesn't touch prepare mutex, there is also
a clk_core_prepare_lock() function, which takes it.

>>>> +			if (orphan->enable_count)
>>>> +				clk_core_enable(parent);
>>> This would be OK, but then we might touch the hardware again?
>> Well, it will touch hardware in one case: if orphaned clock is already prepared
>> and enabled (for example due to having CLK_IS_CRITICAL flag) and the newly
>> registered parent is not (yet?) enabled. Then it will prepare and enable this
>> new parent clock, what is a desired behavior.
>>
>> This code will not temporarily disable any clocks, what I assume was the main
>> reason for the patch mentioned in the commit message.
>>
> Then this code seems bypass the CLK_OPS_PARENT_ENABLE feature.

Well, CLK_OPS_PARENT_ENABLE doesn't affect preparing/enabling parent 
clock if
child clock has been already prepared/enabled. That's all that this fix 
do, so
I don't know what do you mean by bypassing the CLK_OPS_PARENT_ENABLE 
feature.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-26  9:06           ` Marek Szyprowski
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-26  9:06 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Aisheng,

On 2018-01-26 09:36, A.s. Dong wrote:
>> -----Original Message-----
>> From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
>> Sent: Friday, January 26, 2018 3:31 PM
>> To: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: linux-clk at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Michael
>> Turquette <mturquette@baylibre.com>; Shawn Guo <shawnguo@kernel.org>;
>> A.s. Dong <aisheng.dong@nxp.com>; Bartlomiej Zolnierkiewicz
>> <b.zolnierkie@samsung.com>
>> Subject: Re: [PATCH] clk: Properly update prepare/enable count on orphan
>> clock reparent
>>
>> Hi Stephen,
>>
>> On 2018-01-25 23:19, Stephen Boyd wrote:
>>> On 01/15, Marek Szyprowski wrote:
>>>> If orphaned clock has been already prepared/enabled (for example if
>>>> it or one of its children has CLK_IS_CRITICAL flag), then the
>>>> prepare/enable counters of the newly assigned parent are not updated
>>>> correctly. This might later cause warnings during changing clock parents.
>>>>
>>>> This patch fixes following warning on Exynos5422-based boards:
>>>> ------------[ cut here ]------------
>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811
>>>> clk_core_disable_lock+0x18/0x24 Modules linked in:
>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115
>>>> #106 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>> (unwind_backtrace)
>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>> (warn_slowpath_null) from [<c048d584>]
>>>> (clk_core_disable_lock+0x18/0x24) [<c048d584>]
>>>> (clk_core_disable_lock) from [<c048e490>]
>>>> (clk_core_disable_unprepare+0xc/0x20)
>>>> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>> (__clk_set_parent_after+0x48/0x4c)
>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>> 0xeeb01ff8)
>>>> 1fa0:                                     00000000 00000000 00000000 00000000
>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>> 00000000
>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>> trace 503c239fb760f17a ]--- ------------[ cut here ]------------
>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684
>>>> clk_core_disable_unprepare+0x18/0x20
>>>> Modules linked in:
>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G        W        4.15.0-rc7-next-
>> 20180115 #106
>>>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>> (unwind_backtrace)
>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>> (warn_slowpath_null) from [<c048e49c>]
>>>> (clk_core_disable_unprepare+0x18/0x20)
>>>> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>> (__clk_set_parent_after+0x48/0x4c)
>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>> 0xeeb01ff8)
>>>> 1fa0:                                     00000000 00000000 00000000 00000000
>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>> 00000000
>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>> trace 503c239fb760f17b ]--- ------------[ cut here ]------------
>>>>
>>>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting
>>>> during registration")
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>    drivers/clk/clk.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
>>>> 0f686a9dac3e..d33c087d7868 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core *core)
>>>>    		if (parent) {
>>>>    			/* update the clk tree topology */
>>>>    			flags = clk_enable_lock();
>>> This is the spinlock?
>> yes
>>
>>>> +			if (orphan->prepare_count)
>>>> +				clk_core_prepare(parent);
>>> And this is prepare mutex? Doesn't sound like it will work.
>> Prepare mutex is taken at the beginning of __clk_core_init() function, so
>> everything is fine here.
>>
> Theoretically no issue may happen, just looks strange to claim mutex within
> spinlock.

Nope, the mutex is taken before entering this block of code. What is strange
then? clk_core_prepare() doesn't touch prepare mutex, there is also
a clk_core_prepare_lock() function, which takes it.

>>>> +			if (orphan->enable_count)
>>>> +				clk_core_enable(parent);
>>> This would be OK, but then we might touch the hardware again?
>> Well, it will touch hardware in one case: if orphaned clock is already prepared
>> and enabled (for example due to having CLK_IS_CRITICAL flag) and the newly
>> registered parent is not (yet?) enabled. Then it will prepare and enable this
>> new parent clock, what is a desired behavior.
>>
>> This code will not temporarily disable any clocks, what I assume was the main
>> reason for the patch mentioned in the commit message.
>>
> Then this code seems bypass the CLK_OPS_PARENT_ENABLE feature.

Well, CLK_OPS_PARENT_ENABLE doesn't affect preparing/enabling parent 
clock if
child clock has been already prepared/enabled. That's all that this fix 
do, so
I don't know what do you mean by bypassing the CLK_OPS_PARENT_ENABLE 
feature.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
  2018-01-26  9:06           ` Marek Szyprowski
@ 2018-01-26  9:14             ` Marek Szyprowski
  -1 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-26  9:14 UTC (permalink / raw)
  To: A.s. Dong, Stephen Boyd
  Cc: linux-clk, linux-arm-kernel, Michael Turquette, Shawn Guo,
	Bartlomiej Zolnierkiewicz, dl-linux-imx

Hi Aisheng and Stephen,

On 2018-01-26 10:06, Marek Szyprowski wrote:
> Hi Aisheng,
>
> On 2018-01-26 09:36, A.s. Dong wrote:
>>> -----Original Message-----
>>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>>> Sent: Friday, January 26, 2018 3:31 PM
>>> To: Stephen Boyd <sboyd@codeaurora.org>
>>> Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; 
>>> Michael
>>> Turquette <mturquette@baylibre.com>; Shawn Guo <shawnguo@kernel.org>;
>>> A.s. Dong <aisheng.dong@nxp.com>; Bartlomiej Zolnierkiewicz
>>> <b.zolnierkie@samsung.com>
>>> Subject: Re: [PATCH] clk: Properly update prepare/enable count on 
>>> orphan
>>> clock reparent
>>>
>>> Hi Stephen,
>>>
>>> On 2018-01-25 23:19, Stephen Boyd wrote:
>>>> On 01/15, Marek Szyprowski wrote:
>>>>> If orphaned clock has been already prepared/enabled (for example if
>>>>> it or one of its children has CLK_IS_CRITICAL flag), then the
>>>>> prepare/enable counters of the newly assigned parent are not updated
>>>>> correctly. This might later cause warnings during changing clock 
>>>>> parents.
>>>>>
>>>>> This patch fixes following warning on Exynos5422-based boards:
>>>>> ------------[ cut here ]------------
>>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811
>>>>> clk_core_disable_lock+0x18/0x24 Modules linked in:
>>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115
>>>>> #106 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>>> (unwind_backtrace)
>>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>>> (warn_slowpath_null) from [<c048d584>]
>>>>> (clk_core_disable_lock+0x18/0x24) [<c048d584>]
>>>>> (clk_core_disable_lock) from [<c048e490>]
>>>>> (clk_core_disable_unprepare+0xc/0x20)
>>>>> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>>> (__clk_set_parent_after+0x48/0x4c)
>>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>>> 0xeeb01ff8)
>>>>> 1fa0: 00000000 00000000 00000000 00000000
>>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>>> 00000000
>>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>>> trace 503c239fb760f17a ]--- ------------[ cut here ]------------
>>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684
>>>>> clk_core_disable_unprepare+0x18/0x20
>>>>> Modules linked in:
>>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G W        4.15.0-rc7-next-
>>> 20180115 #106
>>>>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>>> (unwind_backtrace)
>>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>>> (warn_slowpath_null) from [<c048e49c>]
>>>>> (clk_core_disable_unprepare+0x18/0x20)
>>>>> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>>> (__clk_set_parent_after+0x48/0x4c)
>>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>>> 0xeeb01ff8)
>>>>> 1fa0: 00000000 00000000 00000000 00000000
>>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>>> 00000000
>>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>>> trace 503c239fb760f17b ]--- ------------[ cut here ]------------
>>>>>
>>>>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting
>>>>> during registration")
>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> ---
>>>>>    drivers/clk/clk.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
>>>>> 0f686a9dac3e..d33c087d7868 100644
>>>>> --- a/drivers/clk/clk.c
>>>>> +++ b/drivers/clk/clk.c
>>>>> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core 
>>>>> *core)
>>>>>            if (parent) {
>>>>>                /* update the clk tree topology */
>>>>>                flags = clk_enable_lock();
>>>> This is the spinlock?
>>> yes
>>>
>>>>> +            if (orphan->prepare_count)
>>>>> +                clk_core_prepare(parent);
>>>> And this is prepare mutex? Doesn't sound like it will work.
>>> Prepare mutex is taken at the beginning of __clk_core_init() 
>>> function, so
>>> everything is fine here.
>>>
>> Theoretically no issue may happen, just looks strange to claim mutex 
>> within
>> spinlock.
>
> Nope, the mutex is taken before entering this block of code. What is 
> strange
> then? clk_core_prepare() doesn't touch prepare mutex, there is also
> a clk_core_prepare_lock() function, which takes it.

Okay, I got your concerns. To make this part a bit easier to understand, 
I will
move "if (orphan->prepare_count) clk_core_prepare(parent);" to be called 
before
"flags = clk_enable_lock();".

>>>>> +            if (orphan->enable_count)
>>>>> +                clk_core_enable(parent);
>>>> This would be OK, but then we might touch the hardware again?
>>> Well, it will touch hardware in one case: if orphaned clock is 
>>> already prepared
>>> and enabled (for example due to having CLK_IS_CRITICAL flag) and the 
>>> newly
>>> registered parent is not (yet?) enabled. Then it will prepare and 
>>> enable this
>>> new parent clock, what is a desired behavior.
>>>
>>> This code will not temporarily disable any clocks, what I assume was 
>>> the main
>>> reason for the patch mentioned in the commit message.
>>>
>> Then this code seems bypass the CLK_OPS_PARENT_ENABLE feature.
>
> Well, CLK_OPS_PARENT_ENABLE doesn't affect preparing/enabling parent 
> clock if
> child clock has been already prepared/enabled. That's all that this 
> fix do, so
> I don't know what do you mean by bypassing the CLK_OPS_PARENT_ENABLE 
> feature.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH] clk: Properly update prepare/enable count on orphan clock reparent
@ 2018-01-26  9:14             ` Marek Szyprowski
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Szyprowski @ 2018-01-26  9:14 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Aisheng and Stephen,

On 2018-01-26 10:06, Marek Szyprowski wrote:
> Hi Aisheng,
>
> On 2018-01-26 09:36, A.s. Dong wrote:
>>> -----Original Message-----
>>> From: Marek Szyprowski [mailto:m.szyprowski at samsung.com]
>>> Sent: Friday, January 26, 2018 3:31 PM
>>> To: Stephen Boyd <sboyd@codeaurora.org>
>>> Cc: linux-clk at vger.kernel.org; linux-arm-kernel at lists.infradead.org; 
>>> Michael
>>> Turquette <mturquette@baylibre.com>; Shawn Guo <shawnguo@kernel.org>;
>>> A.s. Dong <aisheng.dong@nxp.com>; Bartlomiej Zolnierkiewicz
>>> <b.zolnierkie@samsung.com>
>>> Subject: Re: [PATCH] clk: Properly update prepare/enable count on 
>>> orphan
>>> clock reparent
>>>
>>> Hi Stephen,
>>>
>>> On 2018-01-25 23:19, Stephen Boyd wrote:
>>>> On 01/15, Marek Szyprowski wrote:
>>>>> If orphaned clock has been already prepared/enabled (for example if
>>>>> it or one of its children has CLK_IS_CRITICAL flag), then the
>>>>> prepare/enable counters of the newly assigned parent are not updated
>>>>> correctly. This might later cause warnings during changing clock 
>>>>> parents.
>>>>>
>>>>> This patch fixes following warning on Exynos5422-based boards:
>>>>> ------------[ cut here ]------------
>>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811
>>>>> clk_core_disable_lock+0x18/0x24 Modules linked in:
>>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted 4.15.0-rc7-next-20180115
>>>>> #106 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>>> (unwind_backtrace)
>>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>>> (warn_slowpath_null) from [<c048d584>]
>>>>> (clk_core_disable_lock+0x18/0x24) [<c048d584>]
>>>>> (clk_core_disable_lock) from [<c048e490>]
>>>>> (clk_core_disable_unprepare+0xc/0x20)
>>>>> [<c048e490>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>>> (__clk_set_parent_after+0x48/0x4c)
>>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>>> 0xeeb01ff8)
>>>>> 1fa0: 00000000 00000000 00000000 00000000
>>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>>> 00000000
>>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>>> trace 503c239fb760f17a ]--- ------------[ cut here ]------------
>>>>> WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684
>>>>> clk_core_disable_unprepare+0x18/0x20
>>>>> Modules linked in:
>>>>> CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G W??????? 4.15.0-rc7-next-
>>> 20180115 #106
>>>>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> Workqueue: pm genpd_power_off_work_fn [<c0111504>]
>>> (unwind_backtrace)
>>>>> from [<c010dbec>] (show_stack+0x10/0x14) [<c010dbec>] (show_stack)
>>>>> from [<c09b3f34>] (dump_stack+0x90/0xc8) [<c09b3f34>] (dump_stack)
>>>>> from [<c0125060>] (__warn+0xe4/0x110) [<c0125060>] (__warn) from
>>>>> [<c01250cc>] (warn_slowpath_null+0x40/0x48) [<c01250cc>]
>>>>> (warn_slowpath_null) from [<c048e49c>]
>>>>> (clk_core_disable_unprepare+0x18/0x20)
>>>>> [<c048e49c>] (clk_core_disable_unprepare) from [<c048ea60>]
>>>>> (__clk_set_parent_after+0x48/0x4c)
>>>>> [<c048ea60>] (__clk_set_parent_after) from [<c048ec88>]
>>>>> (clk_core_set_parent_nolock+0x224/0x5b4)
>>>>> [<c048ec88>] (clk_core_set_parent_nolock) from [<c048f23c>]
>>>>> (clk_set_parent+0x38/0x6c) [<c048f23c>] (clk_set_parent) from
>>>>> [<c049c41c>] (exynos_pd_power+0x74/0x1cc) [<c049c41c>]
>>>>> (exynos_pd_power) from [<c0550768>] (genpd_power_off+0x164/0x264)
>>>>> [<c0550768>] (genpd_power_off) from [<c0550b34>]
>>>>> (genpd_power_off_work_fn+0x2c/0x40)
>>>>> [<c0550b34>] (genpd_power_off_work_fn) from [<c01432e4>]
>>>>> (process_one_work+0x1d0/0x7bc) [<c01432e4>] (process_one_work) from
>>>>> [<c014393c>] (worker_thread+0x34/0x4dc) [<c014393c>] (worker_thread)
>>>>> from [<c014a0d0>] (kthread+0x128/0x164) [<c014a0d0>] (kthread) from
>>>>> [<c01010b4>] (ret_from_fork+0x14/0x20) Exception stack(0xeeb01fb0 to
>>> 0xeeb01ff8)
>>>>> 1fa0: 00000000 00000000 00000000 00000000
>>>>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>>> 00000000
>>>>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end
>>>>> trace 503c239fb760f17b ]--- ------------[ cut here ]------------
>>>>>
>>>>> Fixes: f8f8f1d04494 ("clk: Don't touch hardware when reparenting
>>>>> during registration")
>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> ---
>>>>> ?? drivers/clk/clk.c | 4 ++++
>>>>> ?? 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
>>>>> 0f686a9dac3e..d33c087d7868 100644
>>>>> --- a/drivers/clk/clk.c
>>>>> +++ b/drivers/clk/clk.c
>>>>> @@ -2982,6 +2982,10 @@ static int __clk_core_init(struct clk_core 
>>>>> *core)
>>>>> ?????????? if (parent) {
>>>>> ?????????????? /* update the clk tree topology */
>>>>> ?????????????? flags = clk_enable_lock();
>>>> This is the spinlock?
>>> yes
>>>
>>>>> +??????????? if (orphan->prepare_count)
>>>>> +??????????????? clk_core_prepare(parent);
>>>> And this is prepare mutex? Doesn't sound like it will work.
>>> Prepare mutex is taken at the beginning of __clk_core_init() 
>>> function, so
>>> everything is fine here.
>>>
>> Theoretically no issue may happen, just looks strange to claim mutex 
>> within
>> spinlock.
>
> Nope, the mutex is taken before entering this block of code. What is 
> strange
> then? clk_core_prepare() doesn't touch prepare mutex, there is also
> a clk_core_prepare_lock() function, which takes it.

Okay, I got your concerns. To make this part a bit easier to understand, 
I will
move "if (orphan->prepare_count) clk_core_prepare(parent);" to be called 
before
"flags = clk_enable_lock();".

>>>>> +??????????? if (orphan->enable_count)
>>>>> +??????????????? clk_core_enable(parent);
>>>> This would be OK, but then we might touch the hardware again?
>>> Well, it will touch hardware in one case: if orphaned clock is 
>>> already prepared
>>> and enabled (for example due to having CLK_IS_CRITICAL flag) and the 
>>> newly
>>> registered parent is not (yet?) enabled. Then it will prepare and 
>>> enable this
>>> new parent clock, what is a desired behavior.
>>>
>>> This code will not temporarily disable any clocks, what I assume was 
>>> the main
>>> reason for the patch mentioned in the commit message.
>>>
>> Then this code seems bypass the CLK_OPS_PARENT_ENABLE feature.
>
> Well, CLK_OPS_PARENT_ENABLE doesn't affect preparing/enabling parent 
> clock if
> child clock has been already prepared/enabled. That's all that this 
> fix do, so
> I don't know what do you mean by bypassing the CLK_OPS_PARENT_ENABLE 
> feature.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-01-26  9:14 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20180115115231eucas1p1499f46ca64b65598d03a182ed5670f28@eucas1p1.samsung.com>
2018-01-15 11:52 ` [PATCH] clk: Properly update prepare/enable count on orphan clock reparent Marek Szyprowski
2018-01-15 11:52   ` Marek Szyprowski
2018-01-17  8:05   ` Krzysztof Kozlowski
2018-01-17  8:05     ` Krzysztof Kozlowski
2018-01-25 22:19   ` Stephen Boyd
2018-01-25 22:19     ` Stephen Boyd
2018-01-26  7:31     ` Marek Szyprowski
2018-01-26  7:31       ` Marek Szyprowski
2018-01-26  8:36       ` A.s. Dong
2018-01-26  8:36         ` A.s. Dong
2018-01-26  9:06         ` Marek Szyprowski
2018-01-26  9:06           ` Marek Szyprowski
2018-01-26  9:14           ` Marek Szyprowski
2018-01-26  9:14             ` Marek Szyprowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.