Messages in this thread |  | | Subject | Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase() | From | Stephen Boyd <> | Date | Sat, 04 Jan 2020 23:53:37 -0800 |
| |
Quoting Doug Anderson (2019-10-01 14:20:50) > 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().
Ok. I bail out early with return 0 now.
> > > > @@ -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.
Thanks.
> > > > @@ -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?
Not really, we already hold the lock when this function is called so locking it again is not useful.
> > > > @@ -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". >
Ok.
|  |