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

[David Lechner <dlechner@baylibre.com>]

On 7/16/24 2:06 AM, Uwe Kleine-König wrote:
> Hello David,
> 
> On Mon, Jul 15, 2024 at 05:23:45PM -0500, David Lechner wrote:
>> 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().
> 
> The "exact" parameter has the purpose to make this fail. While
> implementing the idea I wondered if I should drop the parameter and
> make .set_waveform() imply exact=true.

Would consumers then be expected to implement a retry loop to
handle the error when an exact=true call failed because the same
rounding was no longer possible?

> 
> Two more thoughts about this: First, I think the most usual use cases
> are rounding up or maybe rounding closest (instead of rounding down as
> done by default). It's easy to implement a helper function in the pwm
> core that holds the chip lock and does the necessary function calls to
> determine the rounded setting needed.

Would these same functions also get ioctls for the cdev interface?

> 
> The second thought is: Even when holding the chip lock, another clk
> consumer can theoretically change the flexibility of a participating clk
> while the right settings are determined for a given pwm consumer. Also
> if I use clk_round_rate() to determine the resulting rate of a parent
> clock, it's not sure that I can set this rate because again the
> situation might have changed since I called clk_round_rate() or because
> another consumer refuses my request to change the rate.
> 
> So as soon as you consider changing an upstream clock to reach a certain
> PWM setting, this all degrades to a racy best effort quest.
> 
>> 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.
> 
> I would expect that the "Add more locking" patch doesn't enable more
> races than are possible without it? I don't understand the problems you
> think of here.
> 

When we consider other non-PWM clock consumers you are right, it
is not any different that before. I think you understood me fully.
I agree that there aren't any serious issues here any more than
the current state of things.