Re: [PATCH 14/22] clk: Introduce clk_hw_init_rate_request()

Maxime Ripard
2022-04-23T07:17:38Z

Hi Stephen,

Thanks for your reviews :)

On Fri, Apr 22, 2022 at 08:46:21PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-08 02:10:29)
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 399080456e45..83dd5f1df0b9 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1396,6 +1396,26 @@ static void clk_core_init_rate_req(struct clk_core * const core,
> > }
> > }
> >
> > +/**
> > + * clk_hw_init_rate_request - Initializes a clk_rate_request
> > + * @hw: the clk for which we want to submit a rate request
> > + * @req: the clk_rate_request structure we want to initialise
> > + * @rate: the rate which is to be requested
> > + *
> > + * Initializes and fills a clk_rate_request structure to submit to
>
> s/and fills//
>
> > + * __clk_determine_rate or similar functions.
>
> __clk_determine_rate()
>
> > + */
> > +void clk_hw_init_rate_request(struct clk_hw * const hw,
>
> I don't get why it isn't 'const struct clk_hw *hw', but it looks to be
> following clk_core_init_rate_req() so that can be figured out later.

We won't modify either the pointer nor the clk_hw itself, so it made
sense to me?

> Please remove the const from here regardless; it's not doing anything.

Ok, I'll remove it.

> > + struct clk_rate_request *req,
> > + unsigned long rate)
> > +{
> > + if (WARN_ON(!hw || !req))
>
> Why would you call it without those two items? Another copy/paste from
> clk_core_init_rate_req()?

I never know what to do here honestly. It's true that calling it with
NULL as a parameter seems dumb, but I could very well see a NULL pointer
stored in a variable and using that variable.

Preventing a NULL pointer dereference in such a case seems sane, but I
couldn't find a consistent policy in the kernel about what to do. I'll
remove it if you don't like it.

> > + return;
> > +
> > + clk_core_init_rate_req(hw->core, req, rate);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_hw_init_rate_request);
> > +
> > static bool clk_core_can_round(struct clk_core * const core)
> > {
> > return core->ops->determine_rate || core->ops->round_rate;
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index c10dc4c659e2..39e4ed301ec5 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -42,6 +42,8 @@ struct dentry;
> > * struct clk_rate_request - Structure encoding the clk constraints that
> > * a clock user might require.
> > *
> > + * Should be initialized by calling clk_hw_init_rate_request().
>
> How is that enforced?

I'm sure we can come up with some coccinelle script if needed, but yeah
it's a pretty loose requirement for now.

> This has only become a problem after commit 948fb0969eae ("clk: Always
> clamp the rounded rate") from what I can tell. I guess we can't skip the
> clamp when both min/max are zero though because it may be stack junk?

Yeah, but it isn't the only issue, really.

Prior to this series, the most glaring issue is that most
CLK_SET_RATE_PARENT handling is just updating the rate, or even just
copying the clk_request_rate and forwarding it to the parent:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L553

In this case, the requested rate is wrong, but the best_parent_hw and
best_parent_rate fields will be the one of the child clock, and thus
will point to the parent itself, not the parent's parent.

at91 has the same issue in multiple places:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-generated.c#L191
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-master.c#L402
https://elixir.bootlin.com/linux/latest/source/drivers/clk/at91/clk-master.c#L731

So making it available to drivers seems to add some value at least,
whether we require it or not.

> But I looked at all the call sites and either they zero initialize the
> whole struct (qcom) or they copy the req from what is passed into the
> determine_rate clk_op (others). So we could simply not clamp if both
> values are equal to zero and then qcom would be happy, but that has been
> fixed by setting the min/max to 0 and max instead.

Like I said, it's more of a symptom of a larger issue than a bug in
itself. It's not just that the driver doesn't enforce any boundary (so
at the clk_hw level), which would be fine, it's also that it will ignore
any user boundary, and that's where the bug is in my book. And it was
there prior to the patch you mentioned.

I'd be ok to add that kind of check for the time being, but it should at
least come with some kind of warning to report that something fishy is
going on and needs to be addressed.

> That leaves the other users, which already copy what is being passed
> in, i.e. what is done by clk_core_init_rate_req().

> I guess my question is who is going to use this? And if we can't even
> enforce that it is used then it feels like we shouldn't add it. Maybe it
> can be useful to cleanup the core request initialization logic because
> it's sort of spread out but probably not as a clk_hw API.

The main user really will be a unit test I added for
__clk_determine_rate() to make sure the returned clk_request_rate is
sane. All the drivers we discussed so far would probably need to use
clk_core_forward_rate_req() instead.

If we follow what is being done for the rest of the clk_core_*
functions, clk_core_forward_rate_req() isn't available to providers and
isn't exported, to it would be difficult to use as is in the tests.
Could we create a clk_internal.h header or something maybe to export it
only to the tests if you don't want to expose it to drivers?

Maxime