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:50:49 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Jerome Brunet <jbrunet@...libre.com>
Cc:     Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH] clk: Don't cache errors from clk_ops::get_phase()

(Sorry I'm way behind on emails)

Quoting Jerome Brunet (2019-10-02 01:31:46)
> 
> On Tue 01 Oct 2019 at 19:44, 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.
> 
> Function already under the lock seem to be marked with  "_nolock"
> Maybe one should added for get_phase() ?
> 
> Also the debugfs side calls clk_core_get_rate() and
> clk_core_get_accuracy(). Both are taking the prepare_lock.

Yes both are taking the lock again when we're already holding the lock.
It is wasteful. I'll send another patch with the series to make those
calls in debugfs use the nolock variants. That will open up the question
of how we sometimes recalc rates and other times don't depending on if
the nolock or lock variant of the get_rate() API is used.

> 
> So I don't get why clk_get_phase() should do thing differently from the
> others, and not take the lock ?

Got it.

> 
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 1c677d7f7f53..16add5626dfa 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -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.
> >        */
> > -     if (core->ops->get_phase)
> > -             core->phase = core->ops->get_phase(core->hw);
> > -     else
> > -             core->phase = 0;
> > +     clk_core_get_phase(core);
> 
> Should the error be checked here as well ?

What error?