[PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Fri Dec 15 10:49:26 PST 2023
Hello again,
On Fri, Dec 15, 2023 at 04:15:47PM +0100, Uwe Kleine-König wrote:
> 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 :-\
A colleague pointed me to https://lwn.net/Articles/627419/ which
suggests that indeed kmalloc doesn't fail in most situations. The
relevant difference to the clk_rate_exclusive_get() function is that
making kmalloc fail would result in a better (more robust) kernel. And
so I'm on your side that removing error codes probably isn't a smart
move even if probably nobody will tackle the obvious improvement to
kmalloc any time soon.
If I understand your plans correctly, you intend to introduce a "give me
exclusive control over the clk rate" function (let's call it
clk_rate_lock_maxime() just to have a name for it). You would implement
clk_rate_lock_maxime() in a way that it would fail for a second caller,
right? And that's because the second caller wouldn't be able to call
clk_set_rate() and/or it would stop the first caller from being able to
change the rate. (Which one? Both?)
What if a consumer installed a clock notifier that refuses any rate
change. Would calling clk_rate_lock_maxime() by another consumer fail?
A "yes" would be hard to implement, because you cannot reliably see for
a given installed notifier if it refuses changing the clock. A "no"
would be inconsistent, because the holder of clk_rate_lock_maxime()
cannot change the rate then.
After that destructive excursion, let me try being more constructive:
Would you consider renaming clk_rate_exclusive_get() to clk_rate_lock()
an improvement? Maybe also make the clk rate readonly for the caller
then. (I wouldn't require that, but a colleague argued that would make
the semantic of that function simpler.) As in my use case I don't want
to modify the clock, that's good enough for me. Also that function can
be implemented in a way that never fails, so it could return void.
Is that a compromise that you would agree on?
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/f4a6fe2a/attachment.sig>
More information about the linux-amlogic
mailing list