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?! > 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. > > 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. That is indeed a reason to postpone my patches. Feel free to Cc: me when you're done. And please mention if you need longer than (say) 6 months, then I'd argue that applying my patches now without caring for out-of-tree users is the way to go. My demand for such a rework would be that there is a function for consumers to call that don't have the requirement for a certain rate but only any fixed rate that results in locking the clock's rate to whatever it currently is. Today that function exists and is called clk_rate_exclusive_get(); this might not be the best name, so maybe rename it to something that you consider more sensible at the start of your rework?! Semantically that is similar to read_lock() (which never fails and still prevents any writers). And clk_set_rate() is like try_upgrade_read_lock_to_write_lock(); actually_change_the_rate() downgrade_write_lock_to_read_lock(); where try_upgrade_read_lock_to_write_lock() fails if there are other readers. So maybe a sensible name for today's clk_rate_exclusive_get() is clk_rate_read_lock()? If your variant of clk_rate_exclusive_get() might fail, you can already prepare for me questioning why this is sensible and needed. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
