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/ |
Attachment:
signature.asc
Description: PGP signature
