[PATCH 0/5] clk: Make clk_rate_exclusive_get() return void

[Maxime Ripard]

On Wed, Dec 13, 2023 at 04:52:52PM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 13, 2023 at 12:54:14PM +0100, Maxime Ripard wrote:
> > On Wed, Dec 13, 2023 at 12:08:29PM +0100, Uwe Kleine-König wrote:
> > > On Wed, Dec 13, 2023 at 09:36:49AM +0100, Maxime Ripard wrote:
> > > > On Wed, Dec 13, 2023 at 08:43:00AM +0100, Uwe Kleine-König wrote:
> > > > > On Wed, Dec 13, 2023 at 08:16:04AM +0100, Maxime Ripard wrote:
> > > > > > On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote:
> > > > > > > clk_rate_exclusive_get() returns zero unconditionally. Most users "know"
> > > > > > > that and don't check the return value. This series fixes the four users
> > > > > > > that do error checking on the returned value and then makes function
> > > > > > > return void.
> > > > > > > 
> > > > > > > Given that the changes to the drivers are simple and so merge conflicts
> > > > > > > (if any) should be easy to handle, I suggest to merge this complete
> > > > > > > series via the clk tree.
> > > > > > 
> > > > > > I don't think it's the right way to go about it.
> > > > > > 
> > > > > > clk_rate_exclusive_get() should be expected to fail. For example if
> > > > > > there's another user getting an exclusive rate on the same clock.
> > > > > > 
> > > > > > If we're not checking for it right now, then it should probably be
> > > > > > fixed, but the callers checking for the error are right to do so if they
> > > > > > rely on an exclusive rate. It's the ones that don't that should be
> > > > > > modified.
> > > > > 
> > > > > If some other consumer has already "locked" a clock that I call
> > > > > clk_rate_exclusive_get() for, this isn't an error. In my bubble I call
> > > > > this function because I don't want the rate to change e.g. because I
> > > > > setup some registers in the consuming device to provide a fixed UART
> > > > > baud rate or i2c bus frequency (and that works as expected).
> > > > 
> > > > [a long text of mostly right things (Uwe's interpretation) that are
> > > > however totally unrelated to the patches under discussion.]
> > 
> > I'm glad you consider it "mostly" right.
> 
> there was no offense intended. I didn't agree to all points, but didn't
> think it was helpful to discuss that given that I considered them
> orthogonal to my suggested modifications.
>  
> > > The clk API works with and without my patches in exactly the same way.
> > > It just makes more explicit that clk_rate_exclusive_get() cannot fail
> > > today and removes the error handling from consumers that is never used.
> > 
> > Not really, no.
> 
> What exactly do you oppose here? Both of my sentences are correct?!

That the API works in the exact same way.

> > An API is an interface, meant to provide an abstraction. The only
> > relevant thing is whether or not that function, from an abstract point
> > of view, can fail.
> 
> What is the ideal API that you imagine? For me the ideal API is:
> 
> A consumer might call clk_rate_exclusive_get() and after that returns
> all other consumers are prohibited to change the rate of the clock
> (directly and indirectly) until clk_rate_exclusive_put() is called. If
> this ends in a double lock (i.e. two different consumers locked the
> clock), then I cannot change the rate (and neither can anybody else).
> 
> That is fine iff I don't need to change the rate and just want to rely
> on it to keep its current value (which is a valid use case). And if I
> want to change the rate but another consumer prevents that, I handle
> that in the same away as I handle all other failures to set the rate to
> the value I need. I have to prepare for that anyhow even if I have
> ensured that I'm the only one having exclusivity on that clock.
> 
> Letting clk_rate_exclusive_get() fail in the assumption that the
> consumer also wants to modify the rate is wrong. The obvious point where
> to stop such consumers is when they call clk_rate_set(). And those who
> don't modify the rate then continue without interruption even if there
> are two lockers.
> 
> This can easily be implemented without clk_rate_exclusive_get() ever
> failing.
> 
> > Can you fail to get the exclusivity? Yes. On a theoretical basis, you
> > can, and the function was explicitly documented as such.
> 
> Sure, you could modify the clk internals such that
> clk_rate_exclusive_get() needs to allocate memory. Or that it fails if
> another consumer already has called it. At least the latter is a change
> in semantics that requires to review (and maybe fix) all users. Also
> note that calling clk_rate_exclusive_get() essentially locks all parent
> clocks up to the root clock. So if clk_rate_exclusive_get() fails in the
> presence of another locker, you can only have one locker per clock
> hierarchy because it's impossible that both grab the lock on the root
> clock.

We're not discussing the same thing. You're talking about from a
technical point of view, I'm talking about it from an abstraction point
of view. Let's use another example: kmalloc cannot fail. Are we going to
remove every possible check for a null pointer in the kernel?

No, of course we won't, because at some point it might and the error
handling will be valuable.

Same story here.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20231215/56ce49fc/attachment.sig>