Re: [PATCH 22/22] clk: Prevent a clock without a rate to register

Maxime Ripard
2022-04-23T07:50:02Z

On Fri, Apr 22, 2022 at 09:12:33PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:37)
> > A rate of 0 for a clock is considered an error, as evidenced by the
> > documentation of clk_get_rate() and the code of clk_get_rate() and
> > clk_core_get_rate_nolock().
> >
> > The main source of that error is if the clock is supposed to have a
> > parent but is orphan at the moment of the call. This is likely to be
> > transient and solved later in the life of the system as more clocks are
> > registered.
> >
> > The corollary is thus that if a clock is not an orphan, has a parent that
> > has a rate (so is not an orphan itself either) but returns a rate of 0,
> > something is wrong in the driver. Let's return an error in such a case.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> > drivers/clk/clk.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8bbb6adeeead..e8c55678da85 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3773,6 +3773,16 @@ static int __clk_core_init(struct clk_core *core)
> > rate = 0;
> > core->rate = core->req_rate = rate;
> >
> > + /*
> > + * If we're not an orphan clock and our parent has a rate, then
> > + * if our rate is 0, something is badly broken in recalc_rate.
> > + */
> > + if (!core->orphan && (parent && parent->rate) && !core->rate) {
>
> It's possible that it is an orphan at time of registration, so this
> check doesn't even cover the case when it is parented by a later clk
> registration. How would we error out when parenting the clk to the
> parent if recalc_rate then starts returning 0? It doesn't seem possible
> to implement this.

Like I said in my cover letter, this was mostly to spark a discussion :)

Indeed, that case you mentioned wouldn't be covered by this check. I
don't think this patch is reasonable either :)

I mostly wanted to discuss whether you felt like it was something that
was ok or not. If it isn't, I think a good way forward would be to add a
bunch of pr_warn messages to mention that something is fishy and to
clarify the doc.

Hopefully, it will raise both the attention of developers of already
in-tree drivers to fix this, and will prevent new drivers from
introducing more of that behavior.

If the issue persists we could then take stronger measures some time in
the future if needs be?

Maxime