Hello, On Wed, Oct 25, 2023 at 10:53:27AM +0100, Sean Young wrote: > On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote: > > On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote: > > > On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote: > > > > On 10/19/23 12:51, Uwe Kleine-König wrote: > > > > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote: > > > > >> On 10/17/23 11:17, Sean Young wrote: > > > > > I think it's very subjective if you consider this > > > > > churn or not. > > > > > > > > I consider it churn because I don't think adding a postfix > > > > for what is the default/expected behavior is a good idea > > > > (with GPIOs not sleeping is the expected behavior). > > > > > > > > I agree that this is very subjective and very much goes > > > > into the territory of bikeshedding. So please consider > > > > the above my 2 cents on this and lets leave it at that. > > > > > > You have a valid point. Let's focus on having descriptive function names. > > > > For a couple of days I've been trying to resist the bikeshedding (esp. > > given the changes to backlight are tiny) so I'll try to keep it as > > brief as I can: > > > > 1. I dislike the do_it() and do_it_cansleep() pairing. It is > > difficult to detect when a client driver calls do_it() by mistake. > > In fact a latent bug of this nature can only be detected by runtime > > testing with the small number of PWMs that do not support > > configuration from an atomic context. > > > > In contrast do_it() and do_it_atomic()[*] means that although > > incorrectly calling do_it() from an atomic context can be pretty > > catastrophic it is also trivially detected (with any PWM driver) > > simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.
Wrongly calling the atomic variant (no matter how it's named) in a
context where sleeping is possible is only a minor issue. Being faster
than necessary is hardly a problem, so it only hurts by not being an
preemption point with PREEMPT_VOLUNTARY which might not even be relevant
because we're near to a system call anyhow.
For me the naming is only very loosely related to the possible bugs. I
think calling the wrong function happens mainly because the driver author
isn't aware in which context the call happens and not because of wrong
assumptions about the sleepiness of a certain function call.
If you consider this an argument however, do_it + do_it_cansleep is
better than do_it_atomic + do_it as wrongly assuming do_it would sleep
is less bad than wrongly assuming do_it wouldn't sleep. (The latter is
catched by CONFIG_DEBUG_ATOMIC_SLEEP, but only if it's enabled.)
Having said that while my subjective preference ordering is (with first
= best):
do_it + do_it_cansleep
do_it_atomic + do_it_cansleep
do_it_atomic + do_it
wi(th a _might_sleep or _mightsleep suffix ranging below _cansleep)
I wouldn't get sleepless nights when I get overruled here
(uwe_cansleep :-).
> > No objections (beyond churn) to fully spelt out pairings such as
> > do_it_cansleep() and do_it_atomic()[*]!
>
> I must say I do like the look of this. Uwe, how do you feel about:
> pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about
> pwm_apply_atomic in the past, however I think this this the best
> option I've seen so far.
>
> > 2. If there is an API rename can we make sure the patch contains no
> > other changes (e.g. don't introduce any new API in the same patch).
> > Seperating renames makes the patches easier to review!
> > It makes each one smaller and easier to review!
>
> Yes, this should have been separated out. Will fix for next version.
+1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
