Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
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
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
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
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
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,
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
On Wed 13 Dec 2023 at 17:44, Neil Armstrong wrote: > Hi Maxime, > > Le 13/12/2023 à 09:36, Maxime Ripard a écrit : >> Hi, >> 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). >> I guess it's a larger conversation, but I don't see how that can >> possibly work. >> The way the API is designed, you have no guarantee (outside of >> clk_rate_exclusive_*) that the rate is going to change. >> And clk_rate_exclusive_get() doesn't allow the rate to change while in >> the "critical section". >> So the only possible thing to do is clk_set_rate() + >> clk_rate_exclusive_get(). > > There's clk_set_rate_exclusive() for this purpose. > >> So there's a window where the clock can indeed be changed, and the >> consumer that is about to lock its rate wouldn't be aware of it. >> I guess it would work if you don't care about the rate at all, you just >> want to make sure it doesn't change. >> Out of the 7 users of that function, 3 are in that situation, so I guess >> it's fair. >> 3 are open to that race condition I mentioned above. >> 1 is calling clk_set_rate while in the critical section, which works if >> there's a single user but not if there's multiple, so it should be >> discouraged. >> >>> In this case I won't be able to change the rate of the clock, but that >>> is signalled by clk_set_rate() failing (iff and when I need awother >>> rate) which also seems the right place to fail to me. >> Which is ignored by like half the callers, including the one odd case I >> mentioned above. >> And that's super confusing still: you can *always* get exclusivity, but >> not always do whatever you want with the rate when you have it? How are >> drivers supposed to recover from that? You can handle failing to get >> exclusivity, but certainly not working around variable guarantees. >> >>> It's like that since clk_rate_exclusive_get() was introduced in 2017 >>> (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). >> Right, but "it's always been that way" surely can't be an argument, >> otherwise you wouldn't have done that series in the first place. >> >>> BTW, I just noticed that my assertion "Most users \"know\" that >>> [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of >>> next-20231213 there are 3 callers ignoring the return value of >>> clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. >>> I expected this function to be used more extensively. (In fact I think >>> it should be used more as several drivers rely on the clk rate not >>> changing.) >> Yes, but also it's super difficult to use in practice, and most devices >> don't care. >> The current situation is something like this: >>* Only a handful of devices really care about their clock rate, and >> often only for one of their clock if they have several. You would >> probably get all the devices that create an analog signal somehow >> there, so audio, display, i2c, spi, uarts, etc. Plus the ones doing >> frequency scaling so CPU and GPUs. >>* CPUs and GPUs are very likely to have a dedicated clock, so we can >> rule the "another user is going to mess with my clock" case. >>* UARTs/i2c/etc. are usually taking their clock from the bus interface >> directly which is pretty much never going to change (for good >> reason). And the rate of the bus is not really likely to change. >>* SPI/NAND/MMC usually have their dedicated clock too, and the bus >> rate is not likely to change after the initial setup either. >> So, the only affected devices are the ones generating external signals, >> with the rate
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Hi Neil, On Wed, Dec 13, 2023 at 05:44:28PM +0100, Neil Armstrong wrote: > Le 13/12/2023 à 09:36, Maxime Ripard a écrit : > > 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). > > > > I guess it's a larger conversation, but I don't see how that can > > possibly work. > > > > The way the API is designed, you have no guarantee (outside of > > clk_rate_exclusive_*) that the rate is going to change. > > > > And clk_rate_exclusive_get() doesn't allow the rate to change while in > > the "critical section". > > > > So the only possible thing to do is clk_set_rate() + > > clk_rate_exclusive_get(). > > There's clk_set_rate_exclusive() for this purpose. Sure. But that assumes you'll never need to change the rate while in the critical section. > > So there's a window where the clock can indeed be changed, and the > > consumer that is about to lock its rate wouldn't be aware of it. > > > > I guess it would work if you don't care about the rate at all, you just > > want to make sure it doesn't change. > > > > Out of the 7 users of that function, 3 are in that situation, so I guess > > it's fair. > > > > 3 are open to that race condition I mentioned above. > > > > 1 is calling clk_set_rate while in the critical section, which works if > > there's a single user but not if there's multiple, so it should be > > discouraged. > > > > > In this case I won't be able to change the rate of the clock, but that > > > is signalled by clk_set_rate() failing (iff and when I need awother > > > rate) which also seems the right place to fail to me. > > > > Which is ignored by like half the callers, including the one odd case I > > mentioned above. > > > > And that's super confusing still: you can *always* get exclusivity, but > > not always do whatever you want with the rate when you have it? How are > > drivers supposed to recover from that? You can handle failing to get > > exclusivity, but certainly not working around variable guarantees. > > > > > It's like that since clk_rate_exclusive_get() was introduced in 2017 > > > (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). > > > > Right, but "it's always been that way" surely can't be an argument, > > otherwise you wouldn't have done that series in the first place. > > > > > BTW, I just noticed that my assertion "Most users \"know\" that > > > [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of > > > next-20231213 there are 3 callers ignoring the return value of > > > clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. > > > I expected this function to be used more extensively. (In fact I think > > > it should be used more as several drivers rely on the clk rate not > > > changing.) > > > > Yes, but also it's super difficult to use in practice, and most devices > > don't care. > > > > The current situation is something like this: > > > >* Only a handful of devices really care about their clock rate, and > > often only for one of their clock if they have several. You would > > probably get all the devices that create an analog signal somehow > > there, so audio, display, i2c, spi, uarts, etc. Plus the ones doing > > frequency scaling so CPU and GPUs. > > > >* CPUs and GPUs are very likely to have a dedicated clock, so we can > > rule the "another user is going to mess with my clock" case. > > > >* UARTs/i2c/etc. are usually taking their clock from the bus interface > > directly which is
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
On Wed 13 Dec 2023 at 08:16, Maxime Ripard wrote: > [[PGP Signed Part:Undecided]] > Hi, > > On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote: >> Hello, >> >> 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. Yes, at some point it might. That is why the API returns an error code. What CCF (or any other framework) should be no concern to the consummer. Driver not checking the return are taking there chances, and that is up to them. It is like regmap. Most calls can return an error code but the vast majority of driver happily ignore that. > 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. > I'm not sure that would be right. For sure, restricting a to single user was not my intent when I wrote the thing. The intent was for a consumer to state that it cannot tolerate a rate change of the clock it is using. It is fine for several consumers to state that for a single clock, as long as they 'agree' on the rate. Two instances of the same device could be a good example of that. Those consumers should use 'clk_set_rate_exclusive()' to set the rate and protect it atomically. Calling 'clk_exclusive_get()' then 'clk_set_rate()' is racy as both instance could effectively lock the rate without actually getting the rate they want :/ Admittingly, the API naming is terrible when it comes to this ... > Maxime > > [[End of PGP Signed Part]] -- Jerome
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Hi Maxime, Le 13/12/2023 à 09:36, Maxime Ripard a écrit : Hi, 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). I guess it's a larger conversation, but I don't see how that can possibly work. The way the API is designed, you have no guarantee (outside of clk_rate_exclusive_*) that the rate is going to change. And clk_rate_exclusive_get() doesn't allow the rate to change while in the "critical section". So the only possible thing to do is clk_set_rate() + clk_rate_exclusive_get(). There's clk_set_rate_exclusive() for this purpose. So there's a window where the clock can indeed be changed, and the consumer that is about to lock its rate wouldn't be aware of it. I guess it would work if you don't care about the rate at all, you just want to make sure it doesn't change. Out of the 7 users of that function, 3 are in that situation, so I guess it's fair. 3 are open to that race condition I mentioned above. 1 is calling clk_set_rate while in the critical section, which works if there's a single user but not if there's multiple, so it should be discouraged. In this case I won't be able to change the rate of the clock, but that is signalled by clk_set_rate() failing (iff and when I need awother rate) which also seems the right place to fail to me. Which is ignored by like half the callers, including the one odd case I mentioned above. And that's super confusing still: you can *always* get exclusivity, but not always do whatever you want with the rate when you have it? How are drivers supposed to recover from that? You can handle failing to get exclusivity, but certainly not working around variable guarantees. It's like that since clk_rate_exclusive_get() was introduced in 2017 (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). Right, but "it's always been that way" surely can't be an argument, otherwise you wouldn't have done that series in the first place. BTW, I just noticed that my assertion "Most users \"know\" that [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of next-20231213 there are 3 callers ignoring the return value of clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. I expected this function to be used more extensively. (In fact I think it should be used more as several drivers rely on the clk rate not changing.) Yes, but also it's super difficult to use in practice, and most devices don't care. The current situation is something like this: * Only a handful of devices really care about their clock rate, and often only for one of their clock if they have several. You would probably get all the devices that create an analog signal somehow there, so audio, display, i2c, spi, uarts, etc. Plus the ones doing frequency scaling so CPU and GPUs. * CPUs and GPUs are very likely to have a dedicated clock, so we can rule the "another user is going to mess with my clock" case. * UARTs/i2c/etc. are usually taking their clock from the bus interface directly which is pretty much never going to change (for good reason). And the rate of the bus is not really likely to change. * SPI/NAND/MMC usually have their dedicated clock too, and the bus rate is not likely to change after the initial setup either. So, the only affected devices are the ones generating external signals, with the rate changing during the life of the system. Even for audio or video devices, that's fairly unlikely to happen. And you need to have multiple devices sharing the same clock tree for that issue to occur, which is further reducing the chances it happens. Well, thanks for HW designers, this exists and some SoCs has less PLLs than needed, and they can't be
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
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
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
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. > > 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. 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. Can you fail to get the exclusivity? Yes. On a theoretical basis, you can, and the function was explicitly documented as such. Whether or not the function actually can fail in its current implementation is irrelevant. > Yes, my series doesn't fix any race conditions that are there without > doubt in some consumers. It also doesn't make the situation any worse. Sure it does. If we ever improve that function to handle those unrelated cases, then all your patches will have to be reverted, while we already had code to deal with it written down. > It also doesn't fix other problems that are orthogonal to the intention > of this patch series (neither makes it any of them any worse). > > It's just dead code removal and making sure no new dead code of the same > type is introduced in the future. Again, it's not. It's a modification of the abstraction. > 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. Maxime signature.asc Description: PGP signature
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Hello Maxime, 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.] 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. Yes, my series doesn't fix any race conditions that are there without doubt in some consumers. It also doesn't make the situation any worse. It also doesn't fix other problems that are orthogonal to the intention of this patch series (neither makes it any of them any worse). It's just dead code removal and making sure no new dead code of the same type is introduced in the future. 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. And if in the future someone redesigns how all that works *and* clk_rate_exclusive_get() stays around *and* that makes it possible that clk_rate_exclusive_get() fails (and thus breaking all consumers that don't care for the actual clk rate and only want it to not change), then they'll have to review all users anyhow and reintroduce error handling. I can live with that and suggest until then we're happy that we have a few drivers with less dead code than before. Cheers! Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Hi, 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). I guess it's a larger conversation, but I don't see how that can possibly work. The way the API is designed, you have no guarantee (outside of clk_rate_exclusive_*) that the rate is going to change. And clk_rate_exclusive_get() doesn't allow the rate to change while in the "critical section". So the only possible thing to do is clk_set_rate() + clk_rate_exclusive_get(). So there's a window where the clock can indeed be changed, and the consumer that is about to lock its rate wouldn't be aware of it. I guess it would work if you don't care about the rate at all, you just want to make sure it doesn't change. Out of the 7 users of that function, 3 are in that situation, so I guess it's fair. 3 are open to that race condition I mentioned above. 1 is calling clk_set_rate while in the critical section, which works if there's a single user but not if there's multiple, so it should be discouraged. > In this case I won't be able to change the rate of the clock, but that > is signalled by clk_set_rate() failing (iff and when I need awother > rate) which also seems the right place to fail to me. Which is ignored by like half the callers, including the one odd case I mentioned above. And that's super confusing still: you can *always* get exclusivity, but not always do whatever you want with the rate when you have it? How are drivers supposed to recover from that? You can handle failing to get exclusivity, but certainly not working around variable guarantees. > It's like that since clk_rate_exclusive_get() was introduced in 2017 > (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). Right, but "it's always been that way" surely can't be an argument, otherwise you wouldn't have done that series in the first place. > BTW, I just noticed that my assertion "Most users \"know\" that > [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of > next-20231213 there are 3 callers ignoring the return value of > clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. > I expected this function to be used more extensively. (In fact I think > it should be used more as several drivers rely on the clk rate not > changing.) Yes, but also it's super difficult to use in practice, and most devices don't care. The current situation is something like this: * Only a handful of devices really care about their clock rate, and often only for one of their clock if they have several. You would probably get all the devices that create an analog signal somehow there, so audio, display, i2c, spi, uarts, etc. Plus the ones doing frequency scaling so CPU and GPUs. * CPUs and GPUs are very likely to have a dedicated clock, so we can rule the "another user is going to mess with my clock" case. * UARTs/i2c/etc. are usually taking their clock from the bus interface directly which is pretty much never going to change (for good reason). And the rate of the bus is not really likely to change. * SPI/NAND/MMC usually have their dedicated clock too, and the bus rate is not likely to change after the initial setup either. So, the only affected devices are the ones generating external signals, with the rate changing during the life of the system. Even for audio or video devices, that's fairly unlikely to happen. And you need to have multiple devices sharing the same clock tree for that issue to occur, which is further reducing the chances it happens. Realistically speaking, this only occurs with multi-head display outputs where it's somewhat likely to have all
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Hello Maxime, 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). In this case I won't be able to change the rate of the clock, but that is signalled by clk_set_rate() failing (iff and when I need awother rate) which also seems the right place to fail to me. It's like that since clk_rate_exclusive_get() was introduced in 2017 (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). BTW, I just noticed that my assertion "Most users \"know\" that [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of next-20231213 there are 3 callers ignoring the return value of clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. I expected this function to be used more extensively. (In fact I think it should be used more as several drivers rely on the clk rate not changing.) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
Hi, On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote: > Hello, > > 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. Maxime signature.asc Description: PGP signature
