lkml.org 
[lkml]   [2019]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] clk: Don't cache errors from clk_ops::get_phase()
Hi,

On Tue, Oct 1, 2019 at 10:44 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> We don't check for errors from clk_ops::get_phase() before storing away
> the result into the clk_core::phase member. This can lead to some fairly
> confusing debugfs information if these ops do return an error. Let's
> skip the store when this op fails to fix this. While we're here, move
> the locking outside of clk_core_get_phase() to simplify callers from
> the debugfs side.
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>
> Resending because I couldn't find this anywhere.

It was at:

https://lore.kernel.org/r/155692148370.12939.291938595926908281@swboyd.mtv.corp.google.com


> @@ -2640,14 +2640,14 @@ EXPORT_SYMBOL_GPL(clk_set_phase);
>
> static int clk_core_get_phase(struct clk_core *core)
> {
> - int ret;
> + int ret = 0;
>
> - clk_prepare_lock();
> + lockdep_assert_held(&prepare_lock);
> /* Always try to update cached phase if possible */
> if (core->ops->get_phase)
> - core->phase = core->ops->get_phase(core->hw);
> - ret = core->phase;
> - clk_prepare_unlock();
> + ret = core->ops->get_phase(core->hw);
> + if (ret >= 0)
> + core->phase = ret;

It doesn't matter much, but if it were me I'd add this under the "if
(core->ops->get_phase)" statement. Then we don't keep doing a memory
write of 0 to "core->phase" all the time when "core->ops->get_phase"
isn't there. ...plus (to me) it makes more logical sense.

I'd guess you were trying to make sure that core->phase got set to 0
like the old code did in __clk_core_init(). ...but that really
shouldn't be needed since the clk_core is initted with kzalloc().


> @@ -2661,10 +2661,16 @@ static int clk_core_get_phase(struct clk_core *core)
> */
> int clk_get_phase(struct clk *clk)
> {
> + int ret;
> +
> if (!clk)
> return 0;
>
> - return clk_core_get_phase(clk->core);
> + clk_prepare_unlock();
> + ret = clk_core_get_phase(clk->core);
> + clk_prepare_unlock();

Probably the first of these two should be clk_prepare_lock() unless
you really really wanted the clock to be unlocked.


> @@ -2878,13 +2884,21 @@ static struct hlist_head *orphan_list[] = {
> static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
> int level)
> {
> - seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu %5d %6d\n",
> + int phase;
> +
> + seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu ",
> level * 3 + 1, "",
> 30 - level * 3, c->name,
> c->enable_count, c->prepare_count, c->protect_count,
> - clk_core_get_rate(c), clk_core_get_accuracy(c),
> - clk_core_get_phase(c),
> - clk_core_get_scaled_duty_cycle(c, 100000));
> + clk_core_get_rate(c), clk_core_get_accuracy(c));
> +
> + phase = clk_core_get_phase(c);

Don't you need a clk_prepare_lock() / clk_prepare_unlock() around this now?


> @@ -3349,10 +3366,7 @@ static int __clk_core_init(struct clk_core *core)
> * Since a phase is by definition relative to its parent, just
> * query the current clock phase, or just assume it's in phase.

Maybe update the comment to something like "clk_core_get_phase() will
cache the phase for us".


> */
> - if (core->ops->get_phase)
> - core->phase = core->ops->get_phase(core->hw);
> - else
> - core->phase = 0;
> + clk_core_get_phase(core);

\
 
 \ /
  Last update: 2019-10-01 23:22    [W:0.076 / U:1.940 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site