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

[Maxime Ripard]

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.

> 
> 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.

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.

Can you fail to get the exclusivity? Yes. On a theoretical basis, you
can, and the function was explicitly documented as such.

Whether or not the function actually can fail in its current
implementation is irrelevant.

> Yes, my series doesn't fix any race conditions that are there without
> doubt in some consumers. It also doesn't make the situation any worse.

Sure it does. If we ever improve that function to handle those unrelated
cases, then all your patches will have to be reverted, while we already
had code to deal with it written down.

> It also doesn't fix other problems that are orthogonal to the intention
> of this patch series (neither makes it any of them any worse).
> 
> It's just dead code removal and making sure no new dead code of the same
> type is introduced in the future.

Again, it's not. It's a modification of the abstraction.

> Is there anyone working on improving the clk framework regarding how clk
> rate exclusivity works? I'd probably not notice, but I guess there is
> noone that I need to consider for.

I started working on it.

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/20231213/620130c9/attachment.sig>