linux-kernel - Re: [PATCH] clk: Don't cache errors from clk_ops::get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 04 Jan 2020 23:53:37 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Michael Turquette <mturquette@...libre.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()

Quoting Doug Anderson (2019-10-01 14:20:50)
> Hi,
> 
> On Tue, Oct 1, 2019 at 10:44 AM Stephen Boyd <sboyd@...nel.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@...omium.org>
> > Cc: Heiko Stuebner <heiko@...ech.de>
> > Cc: Jerome Brunet <jbrunet@...libre.com>
> > Signed-off-by: Stephen Boyd <sboyd@...nel.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.