Re: [PATCH v6 2/4] pwm: make it possible to apply pwm changes in atomic context

[Thierry Reding]

On Wed, Nov 29, 2023 at 09:13:35AM +0000, Sean Young wrote:
> Some pwm devices require sleeping, for example if the pwm device is
> connected over i2c. However, many pwm devices could be used from atomic
> context, e.g. memmory mapped pwm. This is useful for, for example, the
> pwm-ir-tx driver which requires precise timing. Sleeping causes havoc
> with the generated IR signal.
> 
> Since not all pmw devices can support atomic context, we also add a
> pwm_is_atomic() function to check if it is supported.

s/i2c/I2C/ and s/pwm/PWM/ in the above. Also, s/memmory/memory/

> 
> Signed-off-by: Sean Young <sean@xxxxxxxx>
> ---
>  Documentation/driver-api/pwm.rst |  9 +++++
>  drivers/pwm/core.c               | 63 ++++++++++++++++++++++++++------
>  drivers/pwm/pwm-renesas-tpu.c    |  1 -
>  include/linux/pwm.h              | 29 ++++++++++++++-
>  4 files changed, 87 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
> index f1d8197c8c43..1d4536fdf47c 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using::
>  
>  	int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state);
>  
> +Some PWM devices can be used from atomic context. You can check if this is
> +supported with::
> +
> +        bool pwm_is_atomic(struct pwm_device *pwm);

This is now going to look a bit odd. I think it'd be best to change this
to pwm_might_sleep() for consistency with the pwm_apply_might_sleep()
function. Fine to keep the actual member variable as atomic, though, so
we don't have to change the default for all drivers.

> +
> +If true, the PWM can be configured from atomic context with::
> +
> +	int pwm_apply_atomic(struct pwm_device *pwm, struct pwm_state *state);
> +
>  This API controls both the PWM period/duty_cycle config and the
>  enable/disable state.
>  
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index fc92ba622e56..63174e207400 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -463,24 +463,15 @@ static void pwm_apply_debug(struct pwm_device *pwm,
>  }
>  
>  /**
> - * pwm_apply_might_sleep() - atomically apply a new state to a PWM device
> + * pwm_apply_unchecked() - atomically apply a new state to a PWM device
>   * @pwm: PWM device
>   * @state: new state to apply
>   */
> -int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
> +static int pwm_apply_unchecked(struct pwm_device *pwm, const struct pwm_state *state)
>  {
>  	struct pwm_chip *chip;
>  	int err;
>  
> -	/*
> -	 * Some lowlevel driver's implementations of .apply() make use of
> -	 * mutexes, also with some drivers only returning when the new
> -	 * configuration is active calling pwm_apply_might_sleep() from atomic context
> -	 * is a bad idea. So make it explicit that calling this function might
> -	 * sleep.
> -	 */
> -	might_sleep();
> -
>  	if (!pwm || !state || !state->period ||
>  	    state->duty_cycle > state->period)
>  		return -EINVAL;
> @@ -501,16 +492,64 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>  
>  	pwm->state = *state;
>  
> +	return 0;
> +}
> +
> +/**
> + * pwm_apply_might_sleep() - atomically apply a new state to a PWM device
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @state: new state to apply
> + */
> +int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
> +{
> +	int err;
> +
> +	/*
> +	 * Some lowlevel driver's implementations of .apply() make use of
> +	 * mutexes, also with some drivers only returning when the new
> +	 * configuration is active calling pwm_apply_might_sleep() from atomic context
> +	 * is a bad idea. So make it explicit that calling this function might
> +	 * sleep.
> +	 */
> +	might_sleep();
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
> +		/*
> +		 * Catch any sleeping drivers when atomic is set.

I think this is slightly confusing. What we're really trying to catch
here is drivers that are marked as "atomic" but which still end up
sleeping during ->apply(), right? So maybe something like this would be
a bit more explicit:

		/*
		 * Catch any drivers that have been marked as atomic but
		 * that will sleep anyway.
		 */

> +/**
> + * pwm_apply_atomic() - apply a new state to a PWM device from atomic context
> + * Not all pwm devices support this function, check with pwm_is_atomic().

s/pwm/PWM/ here...

> + * @pwm: PWM device
> + * @state: new state to apply
> + */
> +int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
> +{
> +	WARN_ONCE(!pwm->chip->atomic,
> +		  "sleeping pwm driver used in atomic context");

... and in the warning message as well.

> +
> +	return pwm_apply_unchecked(pwm, state);
> +}
> +EXPORT_SYMBOL_GPL(pwm_apply_atomic);
> +
>  /**
>   * pwm_capture() - capture and report a PWM signal
>   * @pwm: PWM device
> diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
> index 4239f2c3e8b2..47ea92cd8c67 100644
> --- a/drivers/pwm/pwm-renesas-tpu.c
> +++ b/drivers/pwm/pwm-renesas-tpu.c
> @@ -11,7 +11,6 @@
>  #include <linux/init.h>
>  #include <linux/ioport.h>
>  #include <linux/module.h>
> -#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 5c5c456948a4..f1fa1243e95a 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -286,6 +286,7 @@ struct pwm_ops {
>   * @npwm: number of PWMs controlled by this chip
>   * @of_xlate: request a PWM device given a device tree PWM specifier
>   * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
> + * @atomic: can the driver call pwm_apply_atomic in atomic context

Maybe: "can the driver's ->apply() be called in atomic context"?

>   * @list: list node for internal use
>   * @pwms: array of PWM devices allocated by the framework
>   */
> @@ -299,6 +300,7 @@ struct pwm_chip {
>  	struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
>  					const struct of_phandle_args *args);
>  	unsigned int of_pwm_n_cells;
> +	bool atomic;
>  
>  	/* only used internally by the PWM framework */
>  	struct list_head list;
> @@ -308,6 +310,7 @@ struct pwm_chip {
>  #if IS_ENABLED(CONFIG_PWM)
>  /* PWM user APIs */
>  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);
>  
>  /**
> @@ -378,6 +381,17 @@ static inline void pwm_disable(struct pwm_device *pwm)
>  	pwm_apply_might_sleep(pwm, &state);
>  }
>  
> +/**
> + * pwm_is_atomic() - is pwm_apply_atomic() supported?
> + * @pwm: PWM device
> + *
> + * Returns: true pwm_apply_atomic() can be called from atomic context.
> + */
> +static inline bool pwm_is_atomic(struct pwm_device *pwm)

As I mentioned above, I think it'd be more consistent to call this
pwm_might_sleep().

> +{
> +	return pwm->chip->atomic;
> +}
> +
>  /* PWM provider APIs */
>  int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>  		unsigned long timeout);
> @@ -406,16 +420,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
>  				       struct fwnode_handle *fwnode,
>  				       const char *con_id);
>  #else
> +static inline bool pwm_is_atomic(struct pwm_device *pwm)
> +{
> +	return false;
> +}
> +
>  static inline int pwm_apply_might_sleep(struct pwm_device *pwm,
>  					const struct pwm_state *state)
>  {
>  	might_sleep();
> -	return -ENOTSUPP;
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int pwm_apply_atomic(struct pwm_device *pwm,
> +				   const struct pwm_state *state)
> +{
> +	return -EOPNOTSUPP;
>  }
>  
>  static inline int pwm_adjust_config(struct pwm_device *pwm)
>  {
> -	return -ENOTSUPP;
> +	return -EOPNOTSUPP;
>  }

What's wrong with ENOTSUPP?

Thierry

Attachment: signature.asc
Description: PGP signature