Re: [PATCH v2 4/8] pwm: Provide new consumer API functions for waveforms

[David Lechner <dlechner@baylibre.com>]

On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> Provide API functions for consumers to work with waveforms.
> 
> Note that one relevant difference between pwm_get_state() and
> pwm_get_waveform*() is that the latter yields the actually configured
> hardware state, while the former yields the last state passed to
> pwm_apply*() and so doesn't account for hardware specific rounding.
> 

...

> +
> +/* PWM consumer APIs */
> +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
>  int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_adjust_config(struct pwm_device *pwm);


It seems like there could be a potential race condition between rounding
and setting a PWM.

Consider two PWM devices that share the same clock and the driver can
set the clock rate to accommodate a wider range of periods or get a
more accurate duty length.

Thread 1				Thread 2
--------				--------
PWM consumer A calls round_waveform()
a few times, e.g. to round up or round
closest. Clock is not exclusive so
rounding assumes the rate can be
changed to get the best rate.
					PWM consumer B call set_waveform().
					clk_set_rate_exclusive() is called
					on the clock so the rate can no
					longer be changed and the rate is
					not the one PWM consumer A selected
					in the rounding operation.
PWM consumer A calls set_waveform().
This will either fail or will
not get the same results that
was returned by round_waveform().

---

If it wasn't for the userspace cdev interface, I would suggest
to drop pwm_round_waveform_might_sleep() and pass an optional
function pointer to pwm_set_waveform_might_sleep() instead of
the `bool exact` argument so that the full operation of rounding
and setting could be performed with the pwmchip lock held without
consumer drivers having to worry about getting it right. Consumer
drivers would then just have to implement functions to suit their
needs, or there could even be standard ones like round_closest().

But that still might not fix it in all cases e.g. if two pwmchips
share the same clock as opposed to one pwmchip with two outputs.
If this is possible, then then "pwm: Add more locking" patch
might already cause some problems with race conditions in the
existing PWM apply() ops. Although, I suppose this kind of
clock coordination between pwm chips (and potentially other
devices) should be handled outside of the PWM subsystem.