* [PATCH 0/2] clk: Some fixes for the per-user clk API changes. @ 2015-02-11 10:13 Javier Martinez Canillas 2015-02-11 10:13 ` [PATCH 1/2] clk: Don't dereference parent clock if is NULL Javier Martinez Canillas 2015-02-11 10:13 ` [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components Javier Martinez Canillas 0 siblings, 2 replies; 7+ messages in thread From: Javier Martinez Canillas @ 2015-02-11 10:13 UTC (permalink / raw) To: Mike Turquette Cc: Stephen Boyd, Tomeu Vizoso, Emilio López, linux-samsung-soc, linux-kernel, Javier Martinez Canillas Hello, I found a couple of issues with todays' next (tag next-20150211) when booting an Exynos5420 Peach Pit Chromebook. These were regressions introduced when converting the clk API to return a per-user clk. This series is composed of the following trivial fixes: Javier Martinez Canillas (2): clk: Don't dereference parent clock if is NULL clk: composite: Set clk_core to composite rate and mux components drivers/clk/clk-composite.c | 2 ++ drivers/clk/clk.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) Best regards, Javier ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] clk: Don't dereference parent clock if is NULL 2015-02-11 10:13 [PATCH 0/2] clk: Some fixes for the per-user clk API changes Javier Martinez Canillas @ 2015-02-11 10:13 ` Javier Martinez Canillas 2015-02-11 18:54 ` Stephen Boyd 2015-02-11 10:13 ` [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components Javier Martinez Canillas 1 sibling, 1 reply; 7+ messages in thread From: Javier Martinez Canillas @ 2015-02-11 10:13 UTC (permalink / raw) To: Mike Turquette Cc: Stephen Boyd, Tomeu Vizoso, Emilio López, linux-samsung-soc, linux-kernel, Javier Martinez Canillas The clock passed as an argument to clk_mux_determine_rate_flags() can not have a parent clock if is either a root clock or an orphan. In those cases parent is NULL so parent->hw shouldn't be dereferenced. Fixes: 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/clk/clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 7f53166af5e6..7bd8893c94d6 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -799,7 +799,7 @@ clk_mux_determine_rate_flags(struct clk_hw *hw, unsigned long rate, /* if NO_REPARENT flag set, pass through to current parent */ if (core->flags & CLK_SET_RATE_NO_REPARENT) { parent = core->parent; - if (core->flags & CLK_SET_RATE_PARENT) + if (core->flags & CLK_SET_RATE_PARENT && parent) best = __clk_determine_rate(parent->hw, rate, min_rate, max_rate); else if (parent) -- 2.1.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] clk: Don't dereference parent clock if is NULL 2015-02-11 10:13 ` [PATCH 1/2] clk: Don't dereference parent clock if is NULL Javier Martinez Canillas @ 2015-02-11 18:54 ` Stephen Boyd 2015-02-12 13:35 ` Javier Martinez Canillas 0 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2015-02-11 18:54 UTC (permalink / raw) To: Javier Martinez Canillas Cc: Mike Turquette, Tomeu Vizoso, Emilio L??pez, linux-samsung-soc, linux-kernel On 02/11, Javier Martinez Canillas wrote: > The clock passed as an argument to clk_mux_determine_rate_flags() can > not have a parent clock if is either a root clock or an orphan. > > In those cases parent is NULL so parent->hw shouldn't be dereferenced. > > Fixes: 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > drivers/clk/clk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 7f53166af5e6..7bd8893c94d6 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -799,7 +799,7 @@ clk_mux_determine_rate_flags(struct clk_hw *hw, unsigned long rate, > /* if NO_REPARENT flag set, pass through to current parent */ > if (core->flags & CLK_SET_RATE_NO_REPARENT) { > parent = core->parent; > - if (core->flags & CLK_SET_RATE_PARENT) > + if (core->flags & CLK_SET_RATE_PARENT && parent) > best = __clk_determine_rate(parent->hw, rate, > min_rate, max_rate); > else if (parent) Sorry this doesn't look right. Before all the recent changes to this file we would call __clk_round_rate() which would return 0 if the first argument was NULL. Now we're going to take the else if path and do something different. So we need a parent ? parent->hw : NULL here. Of course, I wonder why a clock has the CLK_SET_RATE_PARENT flag set if it doesn't actually have a parent. That also seems wrong. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] clk: Don't dereference parent clock if is NULL 2015-02-11 18:54 ` Stephen Boyd @ 2015-02-12 13:35 ` Javier Martinez Canillas 0 siblings, 0 replies; 7+ messages in thread From: Javier Martinez Canillas @ 2015-02-12 13:35 UTC (permalink / raw) To: Stephen Boyd Cc: Mike Turquette, Tomeu Vizoso, Emilio L??pez, linux-samsung-soc, linux-kernel Hello Stephen, Thanks a lot for your feedback. On 02/11/2015 07:54 PM, Stephen Boyd wrote: > On 02/11, Javier Martinez Canillas wrote: >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -799,7 +799,7 @@ clk_mux_determine_rate_flags(struct clk_hw *hw, unsigned long rate, >> /* if NO_REPARENT flag set, pass through to current parent */ >> if (core->flags & CLK_SET_RATE_NO_REPARENT) { >> parent = core->parent; >> - if (core->flags & CLK_SET_RATE_PARENT) >> + if (core->flags & CLK_SET_RATE_PARENT && parent) >> best = __clk_determine_rate(parent->hw, rate, >> min_rate, max_rate); >> else if (parent) > > Sorry this doesn't look right. Before all the recent changes to > this file we would call __clk_round_rate() which would return 0 > if the first argument was NULL. Now we're going to take the else > if path and do something different. So we need a parent ? > parent->hw : NULL here. > Right, I'm not that familiar with the common clock framework so I didn't realize I was changing the behavior, sorry about that... > Of course, I wonder why a clock has the CLK_SET_RATE_PARENT flag > set if it doesn't actually have a parent. That also seems wrong. > Yes, I did not face this issue and only patch #2 was enough to fix my problem but the theoretical NULL pointer dereference was found when reading the code. I agree that a clock with that flag set should have at least one parent but afaict there is no sanity check on clock registration. And even if that was the case, I believe that the core should be robust enough to check for NULL before trying to dereference it. I'll post a v2 passing NULL as an argument and parent->hw if parent is not NULL as you suggested. Best regards, Javier ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components 2015-02-11 10:13 [PATCH 0/2] clk: Some fixes for the per-user clk API changes Javier Martinez Canillas 2015-02-11 10:13 ` [PATCH 1/2] clk: Don't dereference parent clock if is NULL Javier Martinez Canillas @ 2015-02-11 10:13 ` Javier Martinez Canillas 2015-02-11 18:50 ` Stephen Boyd 1 sibling, 1 reply; 7+ messages in thread From: Javier Martinez Canillas @ 2015-02-11 10:13 UTC (permalink / raw) To: Mike Turquette Cc: Stephen Boyd, Tomeu Vizoso, Emilio López, linux-samsung-soc, linux-kernel, Javier Martinez Canillas Commit 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") moved the clock state to struct clk_core to allow the clk API returning per user clk instances so now the struct clk_hw .core field is used instead of .clk for most operations. But in case of composite clocks, the rate and mux components didn't have a .core set which leads to a NULL pointer dereference in clk_mux_determine_rate_flags(): Unable to handle kernel NULL pointer dereference at virtual address 00000034 pgd = c0004000 [00000034] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.19.0-next-20150211-00002-g1fb7f0e1150d #423 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) task: ee480000 ti: ee488000 task.ti: ee488000 PC is at clk_mux_determine_rate_flags+0x14/0x19c LR is at __clk_mux_determine_rate+0x24/0x2c pc : [<c03a355c>] lr : [<c03a3734>] psr: a0000113 sp : ee489ce8 ip : ee489d84 fp : ee489d84 r10: 0000005c r9 : 00000001 r8 : 016e3600 r7 : 00000000 r6 : 00000000 r5 : ee442200 r4 : ee440c98 r3 : ffffffff r2 : 00000000 r1 : 016e3600 r0 : ee440c98 Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 10c5387d Table: 4000406a DAC: 00000015 Process swapper/0 (pid: 1, stack limit = 0xee488210) Stack: (0xee489ce8 to 0xee48a000) 9ce0: 00000000 ffffffff 60000113 ee440c98 ee442200 00000000 9d00: 016e3600 ffffffff 00000001 0000005c ee489d84 c03a3734 ee489d80 ee489d84 9d20: 00000000 c048b130 00000400 c03a5798 ee489d80 ee489d84 c0607f60 ffffffea 9d40: 00000001 00000001 ee489d5c c003f844 c06e3340 ee402680 ee440d0c ed935000 9d60: 016e3600 00000003 00000001 0000005c eded3700 c03a11a0 ee489d80 ee489d84 9d80: 016e3600 ee402680 c05b413a eddc9900 016e3600 c03a1228 00000000 ffffffff 9da0: ffffffff eddc9900 016e3600 c03a1c1c ffffffff 016e3600 ed8c6710 c03d6ce4 9dc0: eded3400 00000000 00000000 c03c797c 00000001 0000005c eded3700 eded3700 9de0: 000005e0 00000001 0000005c c03db8ac c06e7e54 c03c8f08 00000000 c06e7e64 9e00: c06b6e74 c06e7f64 000005e0 c06e7df8 c06e5100 00000000 c06e7e6c c06e7f54 9e20: 00000000 00000000 eebd9550 00000000 c06e7da0 c06e7e54 ee7b5010 c06e7da0 9e40: eddc9690 c06e7db4 c06b6e74 00000097 00000000 c03d4398 00000000 ee7b5010 9e60: eebd9550 c06e7da0 00000000 c03db824 ee7b5010 fffffffe c06e7db4 c0299c7c 9e80: ee7b5010 c072a05c 00000000 c0298858 ee7b5010 c06e7db4 ee7b5044 00000000 9ea0: eddc9580 c0298a04 c06e7db4 00000000 c0298978 c02971d4 ee405c78 ee732b40 9ec0: c06e7db4 eded3800 c06d6738 c0298044 c0608300 c06e7db4 00000000 c06e7db4 9ee0: 00000000 c06beb58 c06beb58 c0299024 00000000 c068dd00 00000000 c0008944 9f00: 00000038 c049013c ee462200 c0711920 ee480000 60000113 c06c2cb0 00000000 9f20: 00000000 c06c2cb0 60000113 00000000 ef7fcafc 00000000 c0640194 c00389ec 9f40: c05ec3a8 c063f824 00000006 00000006 c06c2c50 c0696444 00000006 c0696424 9f60: c06ee1c0 c066b588 c06b6e74 00000097 00000000 c066bd44 00000006 00000006 9f80: c066b588 c003d684 00000000 c0481938 00000000 00000000 00000000 00000000 9fa0: 00000000 c0481940 00000000 c000e680 00000000 00000000 00000000 00000000 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [<c03a355c>] (clk_mux_determine_rate_flags) from [<c03a3734>] (__clk_mux_determine_rate+0x24/0x2c) [<c03a3734>] (__clk_mux_determine_rate) from [<c03a5798>] (clk_composite_determine_rate+0xbc/0x238) [<c03a5798>] (clk_composite_determine_rate) from [<c03a11a0>] (clk_core_round_rate_nolock+0x5c/0x9c) [<c03a11a0>] (clk_core_round_rate_nolock) from [<c03a1228>] (__clk_round_rate+0x38/0x40) [<c03a1228>] (__clk_round_rate) from [<c03a1c1c>] (clk_round_rate+0x20/0x38) [<c03a1c1c>] (clk_round_rate) from [<c03d6ce4>] (max98090_dai_set_sysclk+0x34/0x118) [<c03d6ce4>] (max98090_dai_set_sysclk) from [<c03c797c>] (snd_soc_dai_set_sysclk+0x38/0x80) [<c03c797c>] (snd_soc_dai_set_sysclk) from [<c03db8ac>] (snow_late_probe+0x24/0x48) [<c03db8ac>] (snow_late_probe) from [<c03c8f08>] (snd_soc_register_card+0xf04/0x1070) [<c03c8f08>] (snd_soc_register_card) from [<c03d4398>] (devm_snd_soc_register_card+0x30/0x64) [<c03d4398>] (devm_snd_soc_register_card) from [<c03db824>] (snow_probe+0x68/0xcc) [<c03db824>] (snow_probe) from [<c0299c7c>] (platform_drv_probe+0x48/0x98) [<c0299c7c>] (platform_drv_probe) from [<c0298858>] (driver_probe_device+0x114/0x234) [<c0298858>] (driver_probe_device) from [<c0298a04>] (__driver_attach+0x8c/0x90) [<c0298a04>] (__driver_attach) from [<c02971d4>] (bus_for_each_dev+0x54/0x88) [<c02971d4>] (bus_for_each_dev) from [<c0298044>] (bus_add_driver+0xd8/0x1cc) [<c0298044>] (bus_add_driver) from [<c0299024>] (driver_register+0x78/0xf4) [<c0299024>] (driver_register) from [<c0008944>] (do_one_initcall+0x80/0x1d0) [<c0008944>] (do_one_initcall) from [<c066bd44>] (kernel_init_freeable+0x10c/0x1d8) [<c066bd44>] (kernel_init_freeable) from [<c0481940>] (kernel_init+0x8/0xe4) [<c0481940>] (kernel_init) from [<c000e680>] (ret_from_fork+0x14/0x34) Code: e24dd00c e5907000 e1a08001 e88d000c (e5970034) ---[ end trace 825e9bbb0898d421 ]--- Fixes: 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- Hello, I set the rate and mux components' .core in clk_composite_determine_rate() because that is the least intrusive change and where the .clk field is set too but I wonder if there is a reason to change the state of those clocks in that function (which shouldn't have this side effect afaict) instead of doing it in clk_register_composite(). Best regards, Javier --- drivers/clk/clk-composite.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c index dee81b83c4b3..2a53b9580ff7 100644 --- a/drivers/clk/clk-composite.c +++ b/drivers/clk/clk-composite.c @@ -75,6 +75,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, if (rate_hw && rate_ops && rate_ops->determine_rate) { rate_hw->clk = hw->clk; + rate_hw->core = hw->core; return rate_ops->determine_rate(rate_hw, rate, min_rate, max_rate, best_parent_rate, @@ -121,6 +122,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, return best_rate; } else if (mux_hw && mux_ops && mux_ops->determine_rate) { mux_hw->clk = hw->clk; + mux_hw->core = hw->core; return mux_ops->determine_rate(mux_hw, rate, min_rate, max_rate, best_parent_rate, best_parent_p); -- 2.1.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components 2015-02-11 10:13 ` [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components Javier Martinez Canillas @ 2015-02-11 18:50 ` Stephen Boyd 2015-02-12 13:27 ` Javier Martinez Canillas 0 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2015-02-11 18:50 UTC (permalink / raw) To: Javier Martinez Canillas Cc: Mike Turquette, Tomeu Vizoso, Emilio L??pez, linux-samsung-soc, linux-kernel On 02/11, Javier Martinez Canillas wrote: > Commit 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") > moved the clock state to struct clk_core to allow the clk API returning per > user clk instances so now the struct clk_hw .core field is used instead of .clk > for most operations. > > But in case of composite clocks, the rate and mux components didn't have a .core > set which leads to a NULL pointer dereference in clk_mux_determine_rate_flags(): > > Unable to handle kernel NULL pointer dereference at virtual address 00000034 > pgd = c0004000 > [00000034] *pgd=00000000 > Internal error: Oops: 5 [#1] PREEMPT SMP ARM > Modules linked in: > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.19.0-next-20150211-00002-g1fb7f0e1150d #423 > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > task: ee480000 ti: ee488000 task.ti: ee488000 > PC is at clk_mux_determine_rate_flags+0x14/0x19c > LR is at __clk_mux_determine_rate+0x24/0x2c > pc : [<c03a355c>] lr : [<c03a3734>] psr: a0000113 > sp : ee489ce8 ip : ee489d84 fp : ee489d84 > r10: 0000005c r9 : 00000001 r8 : 016e3600 > r7 : 00000000 r6 : 00000000 r5 : ee442200 r4 : ee440c98 > r3 : ffffffff r2 : 00000000 r1 : 016e3600 r0 : ee440c98 > Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 10c5387d Table: 4000406a DAC: 00000015 > Process swapper/0 (pid: 1, stack limit = 0xee488210) > Stack: (0xee489ce8 to 0xee48a000) > 9ce0: 00000000 ffffffff 60000113 ee440c98 ee442200 00000000 > 9d00: 016e3600 ffffffff 00000001 0000005c ee489d84 c03a3734 ee489d80 ee489d84 > 9d20: 00000000 c048b130 00000400 c03a5798 ee489d80 ee489d84 c0607f60 ffffffea > 9d40: 00000001 00000001 ee489d5c c003f844 c06e3340 ee402680 ee440d0c ed935000 > 9d60: 016e3600 00000003 00000001 0000005c eded3700 c03a11a0 ee489d80 ee489d84 > 9d80: 016e3600 ee402680 c05b413a eddc9900 016e3600 c03a1228 00000000 ffffffff > 9da0: ffffffff eddc9900 016e3600 c03a1c1c ffffffff 016e3600 ed8c6710 c03d6ce4 > 9dc0: eded3400 00000000 00000000 c03c797c 00000001 0000005c eded3700 eded3700 > 9de0: 000005e0 00000001 0000005c c03db8ac c06e7e54 c03c8f08 00000000 c06e7e64 > 9e00: c06b6e74 c06e7f64 000005e0 c06e7df8 c06e5100 00000000 c06e7e6c c06e7f54 > 9e20: 00000000 00000000 eebd9550 00000000 c06e7da0 c06e7e54 ee7b5010 c06e7da0 > 9e40: eddc9690 c06e7db4 c06b6e74 00000097 00000000 c03d4398 00000000 ee7b5010 > 9e60: eebd9550 c06e7da0 00000000 c03db824 ee7b5010 fffffffe c06e7db4 c0299c7c > 9e80: ee7b5010 c072a05c 00000000 c0298858 ee7b5010 c06e7db4 ee7b5044 00000000 > 9ea0: eddc9580 c0298a04 c06e7db4 00000000 c0298978 c02971d4 ee405c78 ee732b40 > 9ec0: c06e7db4 eded3800 c06d6738 c0298044 c0608300 c06e7db4 00000000 c06e7db4 > 9ee0: 00000000 c06beb58 c06beb58 c0299024 00000000 c068dd00 00000000 c0008944 > 9f00: 00000038 c049013c ee462200 c0711920 ee480000 60000113 c06c2cb0 00000000 > 9f20: 00000000 c06c2cb0 60000113 00000000 ef7fcafc 00000000 c0640194 c00389ec > 9f40: c05ec3a8 c063f824 00000006 00000006 c06c2c50 c0696444 00000006 c0696424 > 9f60: c06ee1c0 c066b588 c06b6e74 00000097 00000000 c066bd44 00000006 00000006 > 9f80: c066b588 c003d684 00000000 c0481938 00000000 00000000 00000000 00000000 > 9fa0: 00000000 c0481940 00000000 c000e680 00000000 00000000 00000000 00000000 > 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 > [<c03a355c>] (clk_mux_determine_rate_flags) from [<c03a3734>] (__clk_mux_determine_rate+0x24/0x2c) > [<c03a3734>] (__clk_mux_determine_rate) from [<c03a5798>] (clk_composite_determine_rate+0xbc/0x238) > [<c03a5798>] (clk_composite_determine_rate) from [<c03a11a0>] (clk_core_round_rate_nolock+0x5c/0x9c) > [<c03a11a0>] (clk_core_round_rate_nolock) from [<c03a1228>] (__clk_round_rate+0x38/0x40) > [<c03a1228>] (__clk_round_rate) from [<c03a1c1c>] (clk_round_rate+0x20/0x38) > [<c03a1c1c>] (clk_round_rate) from [<c03d6ce4>] (max98090_dai_set_sysclk+0x34/0x118) > [<c03d6ce4>] (max98090_dai_set_sysclk) from [<c03c797c>] (snd_soc_dai_set_sysclk+0x38/0x80) > [<c03c797c>] (snd_soc_dai_set_sysclk) from [<c03db8ac>] (snow_late_probe+0x24/0x48) > [<c03db8ac>] (snow_late_probe) from [<c03c8f08>] (snd_soc_register_card+0xf04/0x1070) > [<c03c8f08>] (snd_soc_register_card) from [<c03d4398>] (devm_snd_soc_register_card+0x30/0x64) > [<c03d4398>] (devm_snd_soc_register_card) from [<c03db824>] (snow_probe+0x68/0xcc) > [<c03db824>] (snow_probe) from [<c0299c7c>] (platform_drv_probe+0x48/0x98) > [<c0299c7c>] (platform_drv_probe) from [<c0298858>] (driver_probe_device+0x114/0x234) > [<c0298858>] (driver_probe_device) from [<c0298a04>] (__driver_attach+0x8c/0x90) > [<c0298a04>] (__driver_attach) from [<c02971d4>] (bus_for_each_dev+0x54/0x88) > [<c02971d4>] (bus_for_each_dev) from [<c0298044>] (bus_add_driver+0xd8/0x1cc) > [<c0298044>] (bus_add_driver) from [<c0299024>] (driver_register+0x78/0xf4) > [<c0299024>] (driver_register) from [<c0008944>] (do_one_initcall+0x80/0x1d0) > [<c0008944>] (do_one_initcall) from [<c066bd44>] (kernel_init_freeable+0x10c/0x1d8) > [<c066bd44>] (kernel_init_freeable) from [<c0481940>] (kernel_init+0x8/0xe4) > [<c0481940>] (kernel_init) from [<c000e680>] (ret_from_fork+0x14/0x34) > Code: e24dd00c e5907000 e1a08001 e88d000c (e5970034) > ---[ end trace 825e9bbb0898d421 ]--- > > Fixes: 035a61c314eb3 ("clk: Make clk API return per-user struct clk instances") > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- Thanks for the patch. > > Hello, > > I set the rate and mux components' .core in clk_composite_determine_rate() > because that is the least intrusive change and where the .clk field is set > too but I wonder if there is a reason to change the state of those clocks > in that function (which shouldn't have this side effect afaict) instead of > doing it in clk_register_composite(). The reason we have to do it in the ops instead of during the registration phase is because some of these ops are called inside the clk_register() function. > > drivers/clk/clk-composite.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c > index dee81b83c4b3..2a53b9580ff7 100644 > --- a/drivers/clk/clk-composite.c > +++ b/drivers/clk/clk-composite.c > @@ -75,6 +75,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, > > if (rate_hw && rate_ops && rate_ops->determine_rate) { > rate_hw->clk = hw->clk; > + rate_hw->core = hw->core; > return rate_ops->determine_rate(rate_hw, rate, min_rate, > max_rate, > best_parent_rate, > @@ -121,6 +122,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, > return best_rate; > } else if (mux_hw && mux_ops && mux_ops->determine_rate) { > mux_hw->clk = hw->clk; > + mux_hw->core = hw->core; > return mux_ops->determine_rate(mux_hw, rate, min_rate, > max_rate, best_parent_rate, > best_parent_p); We need to assign the core pointer wherever we assign the clk pointer. That seems to be quite a few more places than two. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components 2015-02-11 18:50 ` Stephen Boyd @ 2015-02-12 13:27 ` Javier Martinez Canillas 0 siblings, 0 replies; 7+ messages in thread From: Javier Martinez Canillas @ 2015-02-12 13:27 UTC (permalink / raw) To: Stephen Boyd Cc: Mike Turquette, Tomeu Vizoso, Emilio L??pez, linux-samsung-soc, linux-kernel Hello Stephen, On 02/11/2015 07:50 PM, Stephen Boyd wrote: >> --- > > Thanks for the patch. > Thanks a lot for your feedback. >> >> Hello, >> >> I set the rate and mux components' .core in clk_composite_determine_rate() >> because that is the least intrusive change and where the .clk field is set >> too but I wonder if there is a reason to change the state of those clocks >> in that function (which shouldn't have this side effect afaict) instead of >> doing it in clk_register_composite(). > > The reason we have to do it in the ops instead of during the > registration phase is because some of these ops are called inside > the clk_register() function. > Got it. >> >> drivers/clk/clk-composite.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c >> index dee81b83c4b3..2a53b9580ff7 100644 >> --- a/drivers/clk/clk-composite.c >> +++ b/drivers/clk/clk-composite.c >> @@ -75,6 +75,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, >> >> if (rate_hw && rate_ops && rate_ops->determine_rate) { >> rate_hw->clk = hw->clk; >> + rate_hw->core = hw->core; >> return rate_ops->determine_rate(rate_hw, rate, min_rate, >> max_rate, >> best_parent_rate, >> @@ -121,6 +122,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, >> return best_rate; >> } else if (mux_hw && mux_ops && mux_ops->determine_rate) { >> mux_hw->clk = hw->clk; >> + mux_hw->core = hw->core; >> return mux_ops->determine_rate(mux_hw, rate, min_rate, >> max_rate, best_parent_rate, >> best_parent_p); > > We need to assign the core pointer wherever we assign the clk > pointer. That seems to be quite a few more places than two. > Yes, I found more places in other drivers so I wrote a small coccinelle script to replace those using a semantic patch. Also I created a inline function to wrap the assignments since we will have to change it again once the clk_core is dropped. Best regards, Javier ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-02-12 13:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-11 10:13 [PATCH 0/2] clk: Some fixes for the per-user clk API changes Javier Martinez Canillas 2015-02-11 10:13 ` [PATCH 1/2] clk: Don't dereference parent clock if is NULL Javier Martinez Canillas 2015-02-11 18:54 ` Stephen Boyd 2015-02-12 13:35 ` Javier Martinez Canillas 2015-02-11 10:13 ` [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components Javier Martinez Canillas 2015-02-11 18:50 ` Stephen Boyd 2015-02-12 13:27 ` Javier Martinez Canillas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox