| Message ID | 8db2c6f239b9e101f85d556d9e203935c2da2570.1721040875.git.u.kleine-koenig@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: New abstraction and userspace API | expand |
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.
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. 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. 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. Best regards Uwe
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.
Hello David, On Tue, Jul 16, 2024 at 09:28:32AM -0500, David Lechner wrote: > On 7/16/24 2:06 AM, Uwe Kleine-König wrote: > > 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? That's the obvious consequence, yes. > > 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? Not sure. They should be available to libpwm users. But if libpwm delegates to the kernel via an ioctl or does the necessary math itself is a detail that I didn't think about yet. > > 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. Ack. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1f52cabe0131..4dcb7ec4223f 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -49,6 +49,30 @@ static void pwmchip_unlock(struct pwm_chip *chip) DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T)) +static bool pwm_wf_valid(const struct pwm_waveform *wf) +{ + /* + * For now restrict waveforms to period_length <= S64_MAX to provide + * some space for future extensions. One possibility is to simplify + * representing waveforms with inverted polarity using negative values + * somehow. + */ + if (wf->period_length > S64_MAX) + return false; + + if (wf->duty_length > wf->period_length) + return false; + + /* + * .duty_offset is supposed to be smaller than .period_length, apart + * from the corner case .duty_offset = 0 + .period = 0. + */ + if (wf->duty_offset && wf->duty_offset >= wf->period_length) + return false; + + return true; +} + static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state) { if (wf->period_length) { @@ -95,6 +119,29 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf) } } +static int pwmwfcmp(const struct pwm_waveform *a, const struct pwm_waveform *b) +{ + if (a->period_length > b->period_length) + return 1; + + if (a->period_length < b->period_length) + return -1; + + if (a->duty_length > b->duty_length) + return 1; + + if (a->duty_length < b->duty_length) + return -1; + + if (a->duty_offset > b->duty_offset) + return 1; + + if (a->duty_offset < b->duty_offset) + return -1; + + return 0; +} + static int pwm_check_rounding(const struct pwm_waveform *wf, const struct pwm_waveform *wf_rounded) { @@ -145,6 +192,160 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c #define WFHWSIZE 20 +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf) +{ + struct pwm_chip *chip = pwm->chip; + const struct pwm_ops *ops = chip->ops; + struct pwm_waveform wf_req = *wf; + char wfhw[WFHWSIZE]; + int ret_tohw, ret_fromhw; + + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); + + if (!pwm_wf_valid(wf)) + return -EINVAL; + + guard(pwmchip)(chip); + + if (!chip->operational) + return -ENODEV; + + ret_tohw = __pwm_round_waveform_tohw(chip, pwm, wf, wfhw); + if (ret_tohw < 0) + return ret_tohw; + + ret_fromhw = __pwm_round_waveform_fromhw(chip, pwm, wfhw, wf); + if (ret_fromhw < 0) + return ret_fromhw; + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && + ret_tohw == 0 && pwm_check_rounding(&wf_req, wf)) + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n", + wf_req.duty_length, wf_req.period_length, wf_req.duty_offset, + wf->duty_length, wf->period_length, wf->duty_offset); + + return ret_tohw; +} + +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf) +{ + struct pwm_chip *chip = pwm->chip; + const struct pwm_ops *ops = chip->ops; + char wfhw[WFHWSIZE]; + int err; + + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); + + guard(pwmchip)(chip); + + if (!chip->operational) + return -ENODEV; + + err = __pwm_read_waveform(chip, pwm, &wfhw); + if (err) + return err; + + return __pwm_round_waveform_fromhw(chip, pwm, &wfhw, wf); +} + +/* Called with the pwmchip lock held */ +static int __pwm_set_waveform(struct pwm_device *pwm, + const struct pwm_waveform *wf, + bool exact) +{ + struct pwm_chip *chip = pwm->chip; + const struct pwm_ops *ops = chip->ops; + char wfhw[WFHWSIZE]; + struct pwm_waveform wf_rounded; + int err; + + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); + + if (!pwm_wf_valid(wf)) + return -EINVAL; + + err = __pwm_round_waveform_tohw(chip, pwm, wf, &wfhw); + if (err) + return err; + + if ((IS_ENABLED(CONFIG_PWM_DEBUG) || exact) && wf->period_length) { + err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded); + if (err) + return err; + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm_check_rounding(wf, &wf_rounded)) + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n", + wf->duty_length, wf->period_length, wf->duty_offset, + wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset); + + if (exact && pwmwfcmp(wf, &wf_rounded)) { + dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n", + wf->duty_length, wf->period_length, wf->duty_offset, + wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset); + + return 1; + } + } + + err = __pwm_write_waveform(chip, pwm, &wfhw); + if (err) + return err; + + /* update .state */ + pwm_wf2state(wf, &pwm->state); + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && ops->read_waveform && wf->period_length) { + struct pwm_waveform wf_set; + + err = __pwm_read_waveform(chip, pwm, &wfhw); + if (err) + /* maybe ignore? */ + return err; + + err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_set); + if (err) + /* maybe ignore? */ + return err; + + if (pwmwfcmp(&wf_set, &wf_rounded) != 0) + dev_err(&chip->dev, + "Unexpected setting: requested %llu/%llu [+%llu], expected %llu/%llu [+%llu], set %llu/%llu [+%llu]\n", + wf->duty_length, wf->period_length, wf->duty_offset, + wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset, + wf_set.duty_length, wf_set.period_length, wf_set.duty_offset); + } + return 0; +} + +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, + const struct pwm_waveform *wf, bool exact) +{ + struct pwm_chip *chip = pwm->chip; + int err; + + might_sleep(); + + guard(pwmchip)(chip); + + if (!chip->operational) + return -ENODEV; + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) { + /* + * Catch any drivers that have been marked as atomic but + * that will sleep anyway. + */ + non_block_start(); + err = __pwm_set_waveform(pwm, wf, exact); + non_block_end(); + } else { + err = __pwm_set_waveform(pwm, wf, exact); + } + + return err; +} +EXPORT_SYMBOL_GPL(pwm_set_waveform_might_sleep); + static void pwm_apply_debug(struct pwm_device *pwm, const struct pwm_state *state) { diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 2a1f1f25a56c..2d1f36a84f1e 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -357,7 +357,11 @@ static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data) } #if IS_ENABLED(CONFIG_PWM) -/* PWM user APIs */ + +/* 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);
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. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/pwm/core.c | 201 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pwm.h | 6 +- 2 files changed, 206 insertions(+), 1 deletion(-)