From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geert Uytterhoeven Date: Thu, 29 Jan 2015 13:31:12 +0000 Subject: Re: [PATCH v13 4/6] clk: Add rate constraints to clocks Message-Id: List-Id: References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> In-Reply-To: <1422011024-32283-5-git-send-email-tomeu.vizoso@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org Hi Tomeu, Mike, On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso wrote: > Adds a way for clock consumers to set maximum and minimum rates. This > can be used for thermal drivers to set minimum rates, or by misc. > drivers to set maximum rates to assure a minimum performance level. > > Changes the signature of the determine_rate callback by adding the > parameters min_rate and max_rate. > > Signed-off-by: Tomeu Vizoso > Reviewed-by: Stephen Boyd This is now in clk-next, and causes division by zeroes on all shmobile platforms that use renesas,cpg-div6-clock (verified on r8a73a4, r8a7740, r8a7791, sh73a0): Division by zero in kernel. CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.19.0-rc6-koelsch-04360-g48d797d57c5932c8-dirty #792 Hardware name: Generic R8A7791 (Flattened Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) r6:c051b124 r5:00000000 r4:00000000 r3:00200000 [] (show_stack) from [] (dump_stack+0x78/0x94) [] (dump_stack) from [] (__div0+0x18/0x20) r4:2e7ddb00 r3:c05093c8 [] (__div0) from [] (Ldiv0+0x8/0x10) [] (cpg_div6_clock_round_rate) from [] (clk_calc_new_rates+0xc8/0x1d4) r4:eec14e00 r3:c03cb52c [] (clk_calc_new_rates) from [] (clk_core_set_rate_nolock+0x48/0x90) r9:eec02f40 r8:00000001 r7:c051b0b8 r6:c051b124 r5:00000000 r4:eec14e00 [] (clk_core_set_rate_nolock) from [] (__clk_put+0x78/0xdc) r7:c051b0b8 r6:c051b124 r5:eec08100 r4:eec029c0 [] (__clk_put) from [] (clk_put+0x10/0x14) r5:eec08100 r4:00000000 [] (clk_put) from [] (of_clk_init+0x144/0x178) [] (of_clk_init) from [] (rcar_gen2_clocks_init+0x1c/0x24) r10:c04ed098 r9:c05023c0 r8:ffffffff r7:19432124 r6:c0502404 r5:00989680 r4:f0006000 [] (rcar_gen2_clocks_init) from [] (rcar_gen2_timer_init+0x130/0x14c) [] (rcar_gen2_timer_init) from [] (time_init+0x24/0x38) r7:00000000 r6:c0520c80 r5:00000000 r4:ef7fcbc0 [] (time_init) from [] (start_kernel+0x248/0x3bc) [] (start_kernel) from [<40008084>] (0x40008084) r10:00000000 r9:413fc0f2 r8:40007000 r7:c0505870 r6:c04ed094 r5:c0502440 r4:c0521054 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk) > return 1; > } > > -static void clk_core_put(struct clk_core *core) > +void __clk_put(struct clk *clk) > { > struct module *owner; > > - owner = core->owner; > + if (!clk || WARN_ON_ONCE(IS_ERR(clk))) > + return; > > clk_prepare_lock(); > - kref_put(&core->ref, __clk_release); > + > + hlist_del(&clk->child_node); > + clk_core_set_rate_nolock(clk->core, clk->core->req_rate); At this point, clk->core->req_rate is still zero, causing cpg_div6_clock_round_rate() to be called with a zero "rate" parameter, e.g. on r8a7791: cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1 cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1 cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1 and cpg_div6_clock_calc_div() is called to calculate div = DIV_ROUND_CLOSEST(parent_rate, rate); Why was this call to clk_core_set_rate_nolock() in __clk_put() added? Before, there was no rate setting done at this point, and cpg_div6_clock_round_rate() was not called. Have the semantics changed? Should .round_rate() be ready to accept a "zero" rate, and use e.g. the current rate instead? > + owner = clk->core->owner; > + kref_put(&clk->core->ref, __clk_release); > + > clk_prepare_unlock(); > > module_put(owner); > -} > - > -void __clk_put(struct clk *clk) > -{ > - if (!clk || WARN_ON_ONCE(IS_ERR(clk))) > - return; > > - clk_core_put(clk->core); > kfree(clk); > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds