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

Maxime Ripard
2022-04-29T15:45:19Z

[-- Attachment #1: Type: text/plain, Size: 9175 bytes --]

On Thu, Apr 28, 2022 at 07:08:10PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-04-23 02:17:24)
> > On Fri, Apr 22, 2022 at 09:42:26PM -0700, Stephen Boyd wrote:
> > > I enjoy the ambiguity! :) This question has come up before and it
> > > doesn't really matter. Drivers can call clk_prepare_enable() if they
> > > want to be sure that clk_get_rate() is meaningful to them, or they can
> > > not.
> >
> > Right, but that alone already removes the ambiguity :)
> >
> > If not calling clk_enable() prior to calling clk_get_rate() is ok, then
> > we don't *need* to call it, both are valid.
> >
> > Therefore, the "you need to call clk_enable() before calling
> > clk_get_rate()" interpretation isn't the right one, right?
>
> It depends. The CCF isn't the only implementation of the clk API, so
> some drivers want to maintain API compatibility and call clk_enable()
> before getting the rate to make sure the rate is valid. If all other clk
> API implementations are removed or made to work without calling
> clk_enable() we should be able to update the clk.h documentation.
>
> >
> > > The CCF returns a rate that it gets from calling recalc_rate, which
> > > could be inaccurate for others reasons, either because some driver has
> > > called clk_set_rate() after the clk_get_rate()
> >
> > Right, but I guess that's ok, the CCF never claimed to support atomicity
> > in any way. That could also be the result of a parent changing its rate,
> > or a reparenting, etc.
> >
> > Still, it means that it was the best estimate we could do when
> > clk_get_rate() was called. It just turned out to be short-lived.
> >
> > > or because the clk is an orphan still and clk_get() succeeded
> >
> > In this case, core->parent is NULL, so we could argue that it's the same
> > case than clk_get_rate(NULL). It makes sense.
> >
> > > or because the clk_op couldn't calculate it at the time of caching.
> >
> > I guess this is what the discussion is all about, whether it's actually
> > something that we should find it acceptable or not.
> >
> > From what you're saying, it is indeed acceptable and thus clk_get_rate()
> > can return 0 not only from a NULL clock, but also if it's incapable of
> > figuring out a rate for that clock. We should thus document that
> > recalc_rate can indeed return 0 in such a case, and that users should
> > take this into account and not always expect a valid rate.
>
> I don't think there's anything to update.
>
> >
> > Would a documentation for recalc_rate like that:
> >
> > Recalculate the rate of this clock, by querying hardware. The parent
> > rate is an input parameter. It is up to the caller to ensure that the
> > prepare_mutex is held across this call. Returns the calculated rate. If
> > the driver cannot figure out a rate for this clock, it must return 0.
>
> What would the driver return besides 0 if the rate can't be calculated?
> UINT_MAX?

Well, we could expect the driver to never fail, and always be able to
calculate its rate (provided its parent is there). This is what this
patch is doing essentially, and it was working fine on a number of
platforms, so it doesn't seem unfeasible.

Either way, I'm not sure what your pushback is about here. Whether or
not the driver can or cannot return 0 on failure, that should be made
clear in the documentation, and for now it isn't.

If we want to maintain the status quo, then it means that recalc_rate
can return 0 on error, what's the harm in clearly stating that?

> Does the framework care that the rate couldn't be calculated?

I'd say to some extent, yes. The clk_set_rate_range() rework could just
ignore the call to clk_set_rate() if the current rate is 0. That would
have saved us from a couple of regressions.

> Weakening "must" to "should" or saying it "will most likely return 0" is
> better. But again I don't see much point in clarifying here. 0 Hz is a
> valid frequency. Maybe we should just say that 0 is valid?

Ok, so it's about the "must" part.

Yeah, maybe that's the best solution? I don't know, but the current
comment hints a bit at the "0 is an error" part with the reference to a
NULL clk.

> > Optional, but recommended - if this op is not set then clock rate will
> > be initialized to 0.
> >
> > And for clk_get_rate() (in drivers/clk/clk.c):
> >
> > Simply returns the cached rate of the clk, unless CLK_GET_RATE_NOCACHE
> > flag is set, which means a recalc_rate will be issued. Can be called if
> > the clock is enabled or not. If clk is NULL, or if an error occurred,
> > then returns 0.
>
> We should remove the clk_get_rate() documentation in clk.c and let the
> clk.h documentation be the one that is used. That will clarify that
> clk.c implements the API in clk.h. I've been meaning to clean up the
> documentation so I'll hack on that tomorrow and see if I can make this
> better.

ok, thanks :)

> > Work for everyone?
> >
> > > Indeed the CCF doesn't try to recalc the rate after enabling the clk.
> > > Maybe we should do that? It would mean that we have to schedule a work
> > > from the enable path to update the rate accounting outside of any
> > > atomic context.
> > >
> > > Just thinking out loud, the simpler solution is to probably drop all
> > > rate caching in the CCF and get the frequency on a clk_get_rate() call.
> >
> > The above is just enough for me. recalc_rate returning 0 is valid, and
> > should be taken properly into account.
> >
> > Longer term I guess it makes sense to drop the rate caching, but I've
> > introduced enough clock regressions already for a couple of releases :)
>
> Ok.
>
> >
> > > It complicates some of the core though when we check to see if we need
> > > to update clk rates. We could have some middle ground where drivers
> > > indicate that they want to update their cached rate because it's valid
> > > now (either from their enable path or from somewhere else). This may be
> > > nice actually because we could have clk providers call this to force a
> > > recalc down the tree from where they've updated. I think the qcom
> > > DisplayPort phy would want this.
> >
> > A good workaround for now would be to set CLK_GET_RATE_NOCACHE for those
> > clocks.
>
> It's not a good workaround. The clk flag isn't used internally. That's
> the problem. It's only used for callers of clk_get_rate(), but we have
> rate setting logic where a child doesn't know that the rate should be
> recalculated for the parent. It's also not a transitive flag, so if a
> clk with the flag set has children we don't care that clk_get_rate() is
> called on those children when it should really call up to the parent and
> figure out how high to go before calculating the frequency down. This is
> where an API that pushes the information to the framework makes sense so
> that we can push the update instead of pull it on clk_get_rate().

Ah, I see what you mean, thanks

> Anyway, it's a problem for another day.

Yeah, I have a can of worms big enough as it is :)

> > > > I think the latter is the proper interpretation though based on what the
> > > > drivers are doing, and even the CCF itself will call recalc_rate without
> > > > making sure that the clock is enabled (in __clk_core_init() for example).
> > > >
> > > > Then there is the question of whether returning 0 is fine. Again
> > > > clk_get_rate() (clk.c) documentation states that "If clk is NULL then
> > > > returns 0.". This is indeed returned in case of an error condition (in
> > > > clk_get_rate() itself, but also in clk_core_get_rate_nolock()).
> > >
> > > A NULL clk isn't an error. We use NULL in the CCF to indicate that it's
> > > an optional clk. Returning 0 from clk_get_rate() is not an error. If
> > > clk_get() returns an error pointer then it's an error. And NULL isn't an
> > > error value per PTR_ERR() (because NULL == 0 when casted, this isn't
> > > golang).
> >
> > Ok. So we can either group both the "we couldn't figure out a rate" and
> > the "our clock is NULL" case as the same case in clk_get_rate(), or we
> > can convert clk_get_rate() to return signed integers and a negative
> > error code. The latter seems much more intrusive to me and will probably
> > lead to a whole bunch of regressions, so I'd be more inclined to stick
> > to the former. Especially since drivers seem to either don't care or
> > treat 0 as an error already.
>
> Agreed. We can try to work around the "not ready to figure out the rate"
> problem by implementing EPROBE_DEFER for clks that are orphans. That
> needs some thought around cases where clks have parents that aren't
> described in the kernel and we want to reparent them via clk_set_rate()
> or clk_set_parent() or their parent is provided by the driver getting
> the clk (this latter case is a head spinner). In general indicating this
> not fully registered state through clk_get_rate() is wrong though, hence
> the decision to make clk_get_rate() always return something valid and
> have the "don't know" case be returning zero.

Ok

maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]