| Message ID | 20230808171931.944154-102-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
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
On Fri, Oct 06, 2023 at 12:09:59PM +0200, Thierry Reding wrote: > 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. No, if this triggers we already got dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i); in pwmchip_remove() before. > > > + 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? It is run, in pwmchip_remove(). > > 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. I don't think that "available" or "operational" is relevant better than "ready", but I don't care much. I'd pick "operational" over "available". Best regards Uwe
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; 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); + 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"); + 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); 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; struct pwm_device pwms[]; };
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(-)