Re: [PATCH v2 2/8] pwm: Add more locking

[David Lechner <dlechner@baylibre.com>]

On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> This ensures that a pwm_chip that has no corresponding driver isn't used
> and that a driver doesn't go away while a callback is still running.
> 
> In the presence of device links this isn't necessary yet (so this is no
> fix) but for pwm character device support this is needed.
> 
> To not serialize all pwm_apply_state() calls, this introduces a per chip
> lock. An additional complication is that for atomic chips a mutex cannot
> be used (as pwm_apply_atomic() must not sleep) and a spinlock cannot be
> held while calling an operation for a sleeping chip. So depending on the
> chip being atomic or not a spinlock or a mutex is used.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/core.c  | 95 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pwm.h | 13 +++++++
>  2 files changed, 100 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6e752e148b98..b97e2ea0691d 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c

...

> @@ -1138,6 +1190,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_add(chip);
>  
> +	scoped_guard(pwmchip, chip)
> +		chip->operational = true;

Strictly speaking, is the pwmchip lock actually needed here since nothing else
can access the chip until device_add() is called?

I guess it doesn't hurt to take it anyway though.

> +
>  	ret = device_add(&chip->dev);
>  	if (ret)
>  		goto err_device_add;
> @@ -1145,6 +1200,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	return 0;
>  
>  err_device_add:
> +	scoped_guard(pwmchip, chip)
> +		chip->operational = false;
> +
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_remove(chip);
>  
> @@ -1164,11 +1222,27 @@ void pwmchip_remove(struct pwm_chip *chip)
>  {
>  	pwmchip_sysfs_unexport(chip);
>  
> -	if (IS_ENABLED(CONFIG_OF))
> -		of_pwmchip_remove(chip);
> +	scoped_guard(mutex, &pwm_lock) {
> +		unsigned int i;
> +
> +		scoped_guard(pwmchip, chip)
> +			chip->operational = false;
> +
> +		for (i = 0; i < chip->npwm; ++i) {
> +			struct pwm_device *pwm = &chip->pwms[i];
> +
> +			if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +				dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);

Is it really so serious that dev_alert() is needed? dev_warn() or
dev_err() seems more appropriate IMHO.

> +				if (pwm->chip->ops->free)
> +					pwm->chip->ops->free(pwm->chip, pwm);
> +			}
> +		}
> +
> +		if (IS_ENABLED(CONFIG_OF))
> +			of_pwmchip_remove(chip);
>  
> -	scoped_guard(mutex, &pwm_lock)
>  		idr_remove(&pwm_chips, chip->id);
> +	}
>  
>  	device_del(&chip->dev);
>  }
> @@ -1538,12 +1612,17 @@ void pwm_put(struct pwm_device *pwm)
>  
>  	guard(mutex)(&pwm_lock);
>  
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +	/*
> +	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
> +	 * don't warn in this case. This can only happen if a consumer called
> +	 * pwm_put() twice.
> +	 */
> +	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
>  		pr_warn("PWM device already freed\n");
>  		return;
>  	}
>  
> -	if (chip->ops->free)
> +	if (chip->operational && chip->ops->free)
>  		pwm->chip->ops->free(pwm->chip, pwm);
>  
>  	pwm->label = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 8acd60b53f58..464054a45e57 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -275,6 +275,9 @@ struct pwm_ops {
>   * @of_xlate: request a PWM device given a device tree PWM specifier
>   * @atomic: can the driver's ->apply() be called in atomic context
>   * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
> + * @operational: signals if the chip can be used (or is already deregistered)
> + * @nonatomic_lock: mutex for nonatomic chips
> + * @atomic_lock: mutex for atomic chips
>   * @pwms: array of PWM devices allocated by the framework
>   */
>  struct pwm_chip {
> @@ -290,6 +293,16 @@ struct pwm_chip {
>  
>  	/* only used internally by the PWM framework */
>  	bool uses_pwmchip_alloc;
> +	bool operational;
> +	union {
> +		/*
> +		 * depending on the chip being atomic or not either the mutex or
> +		 * the spinlock is used. It protects .operational and
> +		 * synchronizes calls to the .ops->apply and .ops->get_state()

nit: inconsistent use of (), and also synchronizes calls to .ops->capture()

> +		 */
> +		struct mutex nonatomic_lock;
> +		struct spinlock atomic_lock;
> +	};
>  	struct pwm_device pwms[] __counted_by(npwm);
>  };
>