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

[Uwe Kleine-König]

Hello,

On Fri, Dec 15, 2023 at 01:34:26PM +0100, Maxime Ripard wrote:
> 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.

Yeah ok, if you call clk_rate_exclusive_get() and want to check the
return code you always got 0 before and now you get a compiler error. So
there is a difference. What I meant is: Calling clk_rate_exclusive_get()
with my patches has the exact same effects as before (apart from setting
the register used to transport the return value to zero).
 
> > > 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.

In your abstract argumentation clk_rate_exclusive_get() has a
different and stronger semantic than it has today. This stronger
semantic indeed will make this function not succeed in every case. It
should return an error indication and users should check it.

But as your clk_rate_exclusive_get() is a different function than
today's clk_rate_exclusive_get(), I still think our argument isn't
helpful. I want to do something with apples and you're arguing against
that by only talking about oranges.

> Let's use another example: kmalloc cannot fail.

Oh really?

... [a few greps later] ...

While the memory allocation stuff is sufficiently complex that I don't
claim to have grokked it, I think it can (and should) fail. Either I
missed something, or I just burned some more time to convince myself
that kmalloc is just another orange :-\

> Are we going to remove every possible check for a null pointer in the
> kernel?

If you were right with your claim that kmalloc() cannot fail, we should
IMHO consider that. Or maybe better make it robust (in the sense that a
caller of kmalloc() can indeed use the memory returned by it) which
enforces that it might fail at times as even on big machines there is
only a finite amount of memory.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20231215/5d54a0f0/attachment.sig>