[v2,2/8] pwm: Add more locking

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(-)

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);
>  };
>

Hello David,

On Mon, Jul 15, 2024 at 11:56:05AM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> > @@ -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.

It might not be technically necessary, but __pwmchip_add() is the
exception here and holding the lock consistently while accessing
chip->operational has some benefit, too. I prefer to keep it.

> > @@ -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.

I think I picked dev_alert back when this was indeed alarming and
leaving behind some dangling pointers. Today you're right, "action must
be taken immediately" doesn't apply any more. I'll go for dev_warn().

> > @@ -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()

ack. It also protects the new lowlevel driver callbacks, so will reword
to just speak about pwm_ops callbacks.

Thanks for your feedback, I really appreciate people looking in detail
before the new API is set in stone.

Best regards
Uwe