Re: [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components
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 0034
> pgd = c0004000
> [0034] *pgd=
> 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-2-g1fb7f0e1150d #423
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> task: ee48 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 : []lr : []psr: a113
> sp : ee489ce8 ip : ee489d84 fp : ee489d84
> r10: 005c r9 : 0001 r8 : 016e3600
> r7 : r6 : r5 : ee442200 r4 : ee440c98
> r3 : r2 : r1 : 016e3600 r0 : ee440c98
> Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel
> Control: 10c5387d Table: 4000406a DAC: 0015
> Process swapper/0 (pid: 1, stack limit = 0xee488210)
> Stack: (0xee489ce8 to 0xee48a000)
> 9ce0: 6113 ee440c98 ee442200
> 9d00: 016e3600 0001 005c ee489d84 c03a3734 ee489d80 ee489d84
> 9d20: c048b130 0400 c03a5798 ee489d80 ee489d84 c0607f60 ffea
> 9d40: 0001 0001 ee489d5c c003f844 c06e3340 ee402680 ee440d0c ed935000
> 9d60: 016e3600 0003 0001 005c eded3700 c03a11a0 ee489d80 ee489d84
> 9d80: 016e3600 ee402680 c05b413a eddc9900 016e3600 c03a1228
> 9da0: eddc9900 016e3600 c03a1c1c 016e3600 ed8c6710 c03d6ce4
> 9dc0: eded3400 c03c797c 0001 005c eded3700 eded3700
> 9de0: 05e0 0001 005c c03db8ac c06e7e54 c03c8f08 c06e7e64
> 9e00: c06b6e74 c06e7f64 05e0 c06e7df8 c06e5100 c06e7e6c c06e7f54
> 9e20: eebd9550 c06e7da0 c06e7e54 ee7b5010 c06e7da0
> 9e40: eddc9690 c06e7db4 c06b6e74 0097 c03d4398 ee7b5010
> 9e60: eebd9550 c06e7da0 c03db824 ee7b5010 fffe c06e7db4 c0299c7c
> 9e80: ee7b5010 c072a05c c0298858 ee7b5010 c06e7db4 ee7b5044
> 9ea0: eddc9580 c0298a04 c06e7db4 c0298978 c02971d4 ee405c78 ee732b40
> 9ec0: c06e7db4 eded3800 c06d6738 c0298044 c0608300 c06e7db4 c06e7db4
> 9ee0: c06beb58 c06beb58 c0299024 c068dd00 c0008944
> 9f00: 0038 c049013c ee462200 c0711920 ee48 6113 c06c2cb0
> 9f20: c06c2cb0 6113 ef7fcafc c0640194 c00389ec
> 9f40: c05ec3a8 c063f824 0006 0006 c06c2c50 c0696444 0006 c0696424
> 9f60: c06ee1c0 c066b588 c06b6e74 0097 c066bd44 0006 0006
> 9f80: c066b588 c003d684 c0481938
> 9fa0: c0481940 c000e680
> 9fc0:
> 9fe0: 0013
> [] (clk_mux_determine_rate_flags) from []
> (__clk_mux_determine_rate+0x24/0x2c)
> [] (__clk_mux_determine_rate) from []
> (clk_composite_determine_rate+0xbc/0x238)
> [] (clk_composite_determine_rate) from []
> (clk_core_round_rate_nolock+0x5c/0x9c)
> [] (clk_core_round_rate_nolock) from []
> (__clk_round_rate+0x38/0x40)
> [] (__clk_round_rate) from [] (clk_round_rate+0x20/0x38)
> [] (clk_round_rate) from []
> (max98090_dai_set_sysclk+0x34/0x118)
> [] (max98090_dai_set_sysclk) from []
> (snd_soc_dai_set_sysclk+0x38/0x80)
> [] (snd_soc_dai_set_sysclk) from []
> (snow_late_probe+0x24/0x48)
> [] (snow_late_probe) from []
> (snd_soc_register_card+0xf04/0x1070)
> [] (snd_soc_register_card) from []
> (devm_snd_soc_register_card+0x30/0x64)
> [] (devm_snd_soc_register_card) from []
> (snow_probe+0x68/0xcc)
> [] (snow_probe) from [] (platform_drv_probe+0x48/0x98)
> [] (platform_drv_probe) from []
> (driver_probe_device+0x114/0x234)
> [] (driver_probe_device) from []
> (__driver_attach+0x8c/0x90)
> [] (__driver_attach) from [] (bus_for_each_dev+0x54/0x88)
> [] (bus_for_each_dev) from [] (bus_add_driver+0xd8/0x1cc)
> [] (bus_add_driver) from [] (driver_register+0x78/0xf4)
> [] (driver_register) from [] (do_one_initcall+0x80/0x1d0)
> [] (do_one_initcall) from []
> (kernel_init_freeable+0x10c/0x1d8)
> [] (kernel_init_freeable) from [] (kernel_init+0x8/0xe4)
> [] (kernel_init) from [] (ret_from_fork+0x14/0x34)
> Code: e24dd00c e5907000 e1a08001 e88d000c (e5970034)
> ---[ end trace 825e9bbb0898d421 ]---
>
> Fixes: 035a61c314eb3 ("c
[PATCH 2/2] clk: composite: Set clk_core to composite rate and mux components
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 0034
pgd = c0004000
[0034] *pgd=
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-2-g1fb7f0e1150d #423
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
task: ee48 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 : []lr : []psr: a113
sp : ee489ce8 ip : ee489d84 fp : ee489d84
r10: 005c r9 : 0001 r8 : 016e3600
r7 : r6 : r5 : ee442200 r4 : ee440c98
r3 : r2 : r1 : 016e3600 r0 : ee440c98
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c5387d Table: 4000406a DAC: 0015
Process swapper/0 (pid: 1, stack limit = 0xee488210)
Stack: (0xee489ce8 to 0xee48a000)
9ce0: 6113 ee440c98 ee442200
9d00: 016e3600 0001 005c ee489d84 c03a3734 ee489d80 ee489d84
9d20: c048b130 0400 c03a5798 ee489d80 ee489d84 c0607f60 ffea
9d40: 0001 0001 ee489d5c c003f844 c06e3340 ee402680 ee440d0c ed935000
9d60: 016e3600 0003 0001 005c eded3700 c03a11a0 ee489d80 ee489d84
9d80: 016e3600 ee402680 c05b413a eddc9900 016e3600 c03a1228
9da0: eddc9900 016e3600 c03a1c1c 016e3600 ed8c6710 c03d6ce4
9dc0: eded3400 c03c797c 0001 005c eded3700 eded3700
9de0: 05e0 0001 005c c03db8ac c06e7e54 c03c8f08 c06e7e64
9e00: c06b6e74 c06e7f64 05e0 c06e7df8 c06e5100 c06e7e6c c06e7f54
9e20: eebd9550 c06e7da0 c06e7e54 ee7b5010 c06e7da0
9e40: eddc9690 c06e7db4 c06b6e74 0097 c03d4398 ee7b5010
9e60: eebd9550 c06e7da0 c03db824 ee7b5010 fffe c06e7db4 c0299c7c
9e80: ee7b5010 c072a05c c0298858 ee7b5010 c06e7db4 ee7b5044
9ea0: eddc9580 c0298a04 c06e7db4 c0298978 c02971d4 ee405c78 ee732b40
9ec0: c06e7db4 eded3800 c06d6738 c0298044 c0608300 c06e7db4 c06e7db4
9ee0: c06beb58 c06beb58 c0299024 c068dd00 c0008944
9f00: 0038 c049013c ee462200 c0711920 ee48 6113 c06c2cb0
9f20: c06c2cb0 6113 ef7fcafc c0640194 c00389ec
9f40: c05ec3a8 c063f824 0006 0006 c06c2c50 c0696444 0006 c0696424
9f60: c06ee1c0 c066b588 c06b6e74 0097 c066bd44 0006 0006
9f80: c066b588 c003d684 c0481938
9fa0: c0481940 c000e680
9fc0:
9fe0: 0013
[] (clk_mux_determine_rate_flags) from []
(__clk_mux_determine_rate+0x24/0x2c)
[] (__clk_mux_determine_rate) from []
(clk_composite_determine_rate+0xbc/0x238)
[] (clk_composite_determine_rate) from []
(clk_core_round_rate_nolock+0x5c/0x9c)
[] (clk_core_round_rate_nolock) from []
(__clk_round_rate+0x38/0x40)
[] (__clk_round_rate) from [] (clk_round_rate+0x20/0x38)
[] (clk_round_rate) from []
(max98090_dai_set_sysclk+0x34/0x118)
[] (max98090_dai_set_sysclk) from []
(snd_soc_dai_set_sysclk+0x38/0x80)
[] (snd_soc_dai_set_sysclk) from []
(snow_late_probe+0x24/0x48)
[] (snow_late_probe) from []
(snd_soc_register_card+0xf04/0x1070)
[] (snd_soc_register_card) from []
(devm_snd_soc_register_card+0x30/0x64)
[] (devm_snd_soc_register_card) from []
(snow_probe+0x68/0xcc)
[] (snow_probe) from [] (platform_drv_probe+0x48/0x98)
[] (platform_drv_probe) from []
(driver_probe_device+0x114/0x234)
[] (driver_probe_device) from [] (__driver_attach+0x8c/0x90)
[] (__driver_attach) from [] (bus_for_each_dev+0x54/0x88)
[] (bus_for_each_dev) from [] (bus_add_driver+0xd8/0x1cc)
[] (bus_add_driver) from [] (driver_register+0x78/0xf4)
[] (driver_register) from [] (do_one_initcall+0x80/0x1d0)
[] (do_one_initcall) from []
(kernel_init_freeable+0x10c/0x1d8)
[] (kernel_init_freeable) from [] (kernel_init+0x8/0xe4)
[] (kernel_init) from [] (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
---
Hello,
I set the rate and mux components' .core in clk_composite_determine_rate()
because that is the least intrusive change and where
