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