Re: [PATCH v1 101/101] pwm: Add more locking

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6149 bytes --]

On Tue, Aug 08, 2023 at 07:19:31PM +0200, 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.
> 
> As with the previous commit this was not expected to be a problem in the
> presence of device links, but still it can happen with the command
> sequence mentioned in the previous commit. Even if this should turn out
> to be a problem that could be fixed by improving device links, this is a
> necessary preparation to create race-free pwmchip character devices.
> 
> A not so nice side effect is that all calls to the PWM API are
> serialized now. If this turns out to be problematic this can be replaced
> by per-pwm_chip locking later. As long as this bottleneck isn't known to
> be a problem in practise, the simpler approach of a single lock is used
> here.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c  | 50 ++++++++++++++++++++++++++++++++++++---------
>  include/linux/pwm.h |  2 ++
>  2 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index fcf30f77da34..66743ded66f6 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -230,6 +230,7 @@ static struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm,
>  	dev->release = pwmchip_release;
>  
>  	chip->npwm = npwm;
> +	chip->ready = false;
>  
>  	for (i = 0; i < chip->npwm; i++) {
>  		struct pwm_device *pwm = &chip->pwms[i];
> @@ -309,6 +310,8 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  		module_put(owner);
>  	}
>  
> +	chip->ready = true;
> +
>  	mutex_unlock(&pwm_lock);
>  
>  	return ret;
> @@ -324,12 +327,25 @@ EXPORT_SYMBOL_GPL(__pwmchip_add);
>  void pwmchip_remove(struct pwm_chip *chip)
>  {
>  	pwmchip_sysfs_unexport(chip);
> +	unsigned int i;

This looks weird, mixing declarations and code.

>  
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_remove(chip);
>  
>  	mutex_lock(&pwm_lock);
>  
> +	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);

s/pwm #%u/PWM #%u/

> +			if (pwm->chip->ops->free)
> +				pwm->chip->ops->free(pwm->chip, pwm);
> +		}
> +	}
> +
> +	chip->ready = false;
> +
>  	idr_remove(&pwmchip_idr, chip->id);
>  
>  	mutex_unlock(&pwm_lock);
> @@ -505,7 +521,7 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
>  int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>  {
>  	struct pwm_chip *chip;
> -	int err;
> +	int err = 0;
>  
>  	/*
>  	 * Some lowlevel driver's implementations of .apply() make use of
> @@ -522,17 +538,24 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>  
>  	chip = pwm->chip;
>  
> +	mutex_lock(&pwm_lock);
> +
> +	if (!chip->ready) {
> +		err = -ENXIO;
> +		goto out_unlock;
> +	}
> +
>  	if (state->period == pwm->state.period &&
>  	    state->duty_cycle == pwm->state.duty_cycle &&
>  	    state->polarity == pwm->state.polarity &&
>  	    state->enabled == pwm->state.enabled &&
>  	    state->usage_power == pwm->state.usage_power)
> -		return 0;
> +		goto out_unlock;
>  
>  	err = chip->ops->apply(chip, pwm, state);
>  	trace_pwm_apply(pwm, state, err);
>  	if (err)
> -		return err;
> +		goto out_unlock;
>  
>  	pwm->state = *state;
>  
> @@ -542,7 +565,10 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>  	 */
>  	pwm_apply_state_debug(pwm, state);
>  
> -	return 0;
> +out_unlock:
> +	mutex_unlock(&pwm_lock);
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(pwm_apply_state);
>  
> @@ -566,7 +592,12 @@ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>  		return -ENOSYS;
>  
>  	mutex_lock(&pwm_lock);
> -	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
> +
> +	if (pwm->chip->ready)
> +		err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
> +	else
> +		err = -ENXIO;
> +
>  	mutex_unlock(&pwm_lock);
>  
>  	return err;
> @@ -978,18 +1009,17 @@ void pwm_put(struct pwm_device *pwm)
>  
>  	mutex_lock(&pwm_lock);
>  
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> -		pr_warn("PWM device already freed\n");

Don't we want to keep this message? We do want to make sure that we're
always calling things in the right order and this might help catch
errors.

> +	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags))
>  		goto out;
> -	}
>  
> -	if (pwm->chip->ops->free)
> +	if (pwm->chip->ready && pwm->chip->ops->free)
>  		pwm->chip->ops->free(pwm->chip, pwm);

These callbacks may do things like decrease internal reference counts or
free memory, etc. Don't we want to run those even if the PWM chip isn't
operational anymore? Wouldn't we otherwise risk leaking memory and/or
other resources?

>  
>  	pwm->label = NULL;
>  
> -	put_device(&pwm->chip->dev);
>  out:
> +	put_device(&pwm->chip->dev);
> +
>  	mutex_unlock(&pwm_lock);
>  }
>  EXPORT_SYMBOL_GPL(pwm_put);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 3dd46b89ab8b..f5b65994a30e 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -289,6 +289,7 @@ struct pwm_ops {
>   * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
>   * @list: list node for internal use
>   * @pwms: array of PWM devices allocated by the framework
> + * @ready: tracks if the chip is operational
>   */
>  struct pwm_chip {
>  	struct device dev;
> @@ -302,6 +303,7 @@ struct pwm_chip {
>  	unsigned int of_pwm_n_cells;
>  
>  	/* only used internally by the PWM framework */
> +	bool ready;

Can we find a better name for this? Maybe something like "available" or
"operational"? Those are a bit longer, but I feel like they convey more
accurately what's going on. In other words, "ready" is very vague.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]