| Message ID | 20230808171931.944154-71-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:00PM +0200, Uwe Kleine-König wrote: > It's required to not free the memory underlying a requested PWM > while a consumer still has a reference to it. While currently a pwm_chip > doesn't life long enough in all cases, linking the struct pwm to the > pwm_chip results in the right lifetime as soon as the pwmchip is living > long enough. This happens with the following commits. > > Note this is a breaking change for all pwm drivers that don't use > pwmchip_alloc(). > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/core.c | 24 +++++++++--------------- > include/linux/pwm.h | 2 +- > 2 files changed, 10 insertions(+), 16 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index cfcddf62ab01..3b8d41fdda1b 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -198,7 +198,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > void *pwmchip_priv(struct pwm_chip *chip) > { > - return &chip[1]; > + return &chip->pwms[chip->npwm]; I already disliked &chip[1] and this isn't making things any better. I fully realize that this is going to give us the right address, but it just looks wrong. Can we not do something like: return (void *)chip + sizeof(*chip); instead? That would make it more explict that we're trying to get at the extra data that was allocated. It also makes things a bit more robust and doesn't explode when somebody decides to add fields after "pwms". > } > EXPORT_SYMBOL_GPL(pwmchip_priv); > > @@ -208,7 +208,7 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si > size_t alloc_size; > unsigned int i; > > - alloc_size = sizeof(*chip) + sizeof_priv; > + alloc_size = sizeof(*chip) + npwm * sizeof(struct pwm_device) + sizeof_priv; > > chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); > if (!chip) > @@ -217,6 +217,13 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si > chip->dev = parent; > chip->npwm = npwm; > > + for (i = 0; i < chip->npwm; i++) { > + struct pwm_device *pwm = &chip->pwms[i]; > + > + pwm->chip = chip; > + pwm->hwpwm = i; > + } > + > return chip; > } > EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); > @@ -243,26 +250,15 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > > chip->owner = owner; > > - chip->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL); I think the structure of this patch series is a bit weird. Basically you're not actually improving things until the very end, at which point all questions get resolved. What this patch does isn't actually changing anything about the object lifetimes. chip->pwms still goes away at the same time (effectively) because the chip's memory allocation will be released shortly after pwmchip_remove() is called. It isn't until the very end of the series that you actually fix up the lifetime problem. So I read through the entire series trying to make sense of all this and commenting where things aren't going to work as expected, only to realize it isn't all going to fall in place until the very end. So I think you should either make this clearer in the commit message or make sure that things like pwmchip_alloc() do the right things from the start. Thierry > - if (!chip->pwms) > - return -ENOMEM; > - > mutex_lock(&pwm_lock); > > ret = idr_alloc(&pwmchip_idr, chip, 0, 0, GFP_KERNEL); > if (ret < 0) { > mutex_unlock(&pwm_lock); > - kfree(chip->pwms); > return ret; > } > > chip->id = ret; > - for (i = 0; i < chip->npwm; i++) { > - struct pwm_device *pwm = &chip->pwms[i]; > - > - pwm->chip = chip; > - pwm->hwpwm = i; > - } > > mutex_unlock(&pwm_lock); > > @@ -293,8 +289,6 @@ void pwmchip_remove(struct pwm_chip *chip) > idr_remove(&pwmchip_idr, chip->id); > > mutex_unlock(&pwm_lock); > - > - kfree(chip->pwms); > } > EXPORT_SYMBOL_GPL(pwmchip_remove); > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 3c0da17e193c..fbcba204de44 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -301,7 +301,7 @@ struct pwm_chip { > unsigned int of_pwm_n_cells; > > /* only used internally by the PWM framework */ > - struct pwm_device *pwms; > + struct pwm_device pwms[]; > }; > > #if IS_ENABLED(CONFIG_PWM) > -- > 2.40.1 >
On Fri, Oct 06, 2023 at 11:38:12AM +0200, Thierry Reding wrote: > On Tue, Aug 08, 2023 at 07:19:00PM +0200, Uwe Kleine-König wrote: > > It's required to not free the memory underlying a requested PWM > > while a consumer still has a reference to it. While currently a pwm_chip > > doesn't life long enough in all cases, linking the struct pwm to the > > pwm_chip results in the right lifetime as soon as the pwmchip is living > > long enough. This happens with the following commits. > > > > Note this is a breaking change for all pwm drivers that don't use > > pwmchip_alloc(). > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/core.c | 24 +++++++++--------------- > > include/linux/pwm.h | 2 +- > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > index cfcddf62ab01..3b8d41fdda1b 100644 > > --- a/drivers/pwm/core.c > > +++ b/drivers/pwm/core.c > > @@ -198,7 +198,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > > > void *pwmchip_priv(struct pwm_chip *chip) > > { > > - return &chip[1]; > > + return &chip->pwms[chip->npwm]; > > I already disliked &chip[1] and this isn't making things any better. I > fully realize that this is going to give us the right address, but it > just looks wrong. Can we not do something like: > > return (void *)chip + sizeof(*chip); In practise this works, but I'm not 100% confident that the compiler might not add padding that breaks this. I don't particularly like this function either and will think a bit more about it for v2. > instead? That would make it more explict that we're trying to get at the > extra data that was allocated. It also makes things a bit more robust > and doesn't explode when somebody decides to add fields after "pwms". Things will explode if the flexible array member isn't at the end of struct pwm_chip no matter how you implement pwmchip_priv. > > } > > EXPORT_SYMBOL_GPL(pwmchip_priv); > > > > @@ -208,7 +208,7 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si > > size_t alloc_size; > > unsigned int i; > > > > - alloc_size = sizeof(*chip) + sizeof_priv; > > + alloc_size = sizeof(*chip) + npwm * sizeof(struct pwm_device) + sizeof_priv; > > > > chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); > > if (!chip) > > @@ -217,6 +217,13 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si > > chip->dev = parent; > > chip->npwm = npwm; > > > > + for (i = 0; i < chip->npwm; i++) { > > + struct pwm_device *pwm = &chip->pwms[i]; > > + > > + pwm->chip = chip; > > + pwm->hwpwm = i; > > + } > > + > > return chip; > > } > > EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); > > @@ -243,26 +250,15 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > > > > chip->owner = owner; > > > > - chip->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL); > > I think the structure of this patch series is a bit weird. Basically > you're not actually improving things until the very end, at which point > all questions get resolved. > > What this patch does isn't actually changing anything about the object > lifetimes. chip->pwms still goes away at the same time (effectively) > because the chip's memory allocation will be released shortly after > pwmchip_remove() is called. > > It isn't until the very end of the series that you actually fix up the > lifetime problem. So I read through the entire series trying to make > sense of all this and commenting where things aren't going to work as > expected, only to realize it isn't all going to fall in place until the > very end. > > So I think you should either make this clearer in the commit message or > make sure that things like pwmchip_alloc() do the right things from the > start. The commit log for the function introducing pwmchip_alloc() has: Compared to the status quo [...] it doesn't change anything relevant (yet). The intention is that after all drivers are switched to use this allocation function, its possible to add a struct device to struct pwm_chip to properly track the latter's lifetime without touching all drivers again. The intention of this text is exactly what you asked for. If you have a better wording I'm open to hear your suggestions. Best regards Uwe
On Fri, Oct 06, 2023 at 01:04:51PM +0200, Uwe Kleine-König wrote: > On Fri, Oct 06, 2023 at 11:38:12AM +0200, Thierry Reding wrote: > > On Tue, Aug 08, 2023 at 07:19:00PM +0200, Uwe Kleine-König wrote: > > > It's required to not free the memory underlying a requested PWM > > > while a consumer still has a reference to it. While currently a pwm_chip > > > doesn't life long enough in all cases, linking the struct pwm to the > > > pwm_chip results in the right lifetime as soon as the pwmchip is living > > > long enough. This happens with the following commits. > > > > > > Note this is a breaking change for all pwm drivers that don't use > > > pwmchip_alloc(). > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/core.c | 24 +++++++++--------------- > > > include/linux/pwm.h | 2 +- > > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > index cfcddf62ab01..3b8d41fdda1b 100644 > > > --- a/drivers/pwm/core.c > > > +++ b/drivers/pwm/core.c > > > @@ -198,7 +198,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > > > > > void *pwmchip_priv(struct pwm_chip *chip) > > > { > > > - return &chip[1]; > > > + return &chip->pwms[chip->npwm]; > > > > I already disliked &chip[1] and this isn't making things any better. I > > fully realize that this is going to give us the right address, but it > > just looks wrong. Can we not do something like: > > > > return (void *)chip + sizeof(*chip); > > In practise this works, but I'm not 100% confident that the compiler > might not add padding that breaks this. I don't particularly like this > function either and will think a bit more about it for v2. I'm not at all a fan of this whole pwmchip_alloc() business and I would prefer if we could somehow just keep embedding this into the driver- specific structures and take care of the lifetime management with less intrusion. However, I don't see how that could easily be done. It would be slightly easier if we didn't use the flexible array, I suppose. > > > instead? That would make it more explict that we're trying to get at the > > extra data that was allocated. It also makes things a bit more robust > > and doesn't explode when somebody decides to add fields after "pwms". > > Things will explode if the flexible array member isn't at the end of > struct pwm_chip no matter how you implement pwmchip_priv. Perhaps one more reason to avoid the flexible array member. It's not that big a deal, but I'm all for keeping things simple, and that whole business of computing the allocation size and then making sure we point at the right offsets just doesn't seem like the optimal way to do things, even though I'm well aware that it's common practice elsewhere. Thierry
Hello Thierry, On Fri, Oct 06, 2023 at 01:16:35PM +0200, Thierry Reding wrote: > On Fri, Oct 06, 2023 at 01:04:51PM +0200, Uwe Kleine-König wrote: > > On Fri, Oct 06, 2023 at 11:38:12AM +0200, Thierry Reding wrote: > > > On Tue, Aug 08, 2023 at 07:19:00PM +0200, Uwe Kleine-König wrote: > > > > It's required to not free the memory underlying a requested PWM > > > > while a consumer still has a reference to it. While currently a pwm_chip > > > > doesn't life long enough in all cases, linking the struct pwm to the > > > > pwm_chip results in the right lifetime as soon as the pwmchip is living > > > > long enough. This happens with the following commits. > > > > > > > > Note this is a breaking change for all pwm drivers that don't use > > > > pwmchip_alloc(). > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > drivers/pwm/core.c | 24 +++++++++--------------- > > > > include/linux/pwm.h | 2 +- > > > > 2 files changed, 10 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > > index cfcddf62ab01..3b8d41fdda1b 100644 > > > > --- a/drivers/pwm/core.c > > > > +++ b/drivers/pwm/core.c > > > > @@ -198,7 +198,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > > > > > > > void *pwmchip_priv(struct pwm_chip *chip) > > > > { > > > > - return &chip[1]; > > > > + return &chip->pwms[chip->npwm]; > > > > > > I already disliked &chip[1] and this isn't making things any better. I > > > fully realize that this is going to give us the right address, but it > > > just looks wrong. Can we not do something like: > > > > > > return (void *)chip + sizeof(*chip); > > > > In practise this works, but I'm not 100% confident that the compiler > > might not add padding that breaks this. I don't particularly like this > > function either and will think a bit more about it for v2. > > I'm not at all a fan of this whole pwmchip_alloc() business and I would > prefer if we could somehow just keep embedding this into the driver- > specific structures and take care of the lifetime management with less > intrusion. > > However, I don't see how that could easily be done. It would be slightly > easier if we didn't use the flexible array, I suppose. Without that flexible array member you'd need a pointer instead of the struct pwm_device pwms[] in struct pwm_chip and a separate allocation. While handling the flexible array isn't exactly pretty, I prefer it compared to the consequences of the alternative (two allocation instead of one, so more memory management cruft, less cache locality, more pointer dereferences). Also given that the ugliness of the flexible array approach is limited to this one function it's IMHO easily possible to get over it. The idea used by the counter core (in drivers/counter/counter-core.c) with a struct counter_device_allochelper doesn't work in combination with the flexible array. netdev_priv uses a similar approach to what you suggested (and adds some complexity by adding an alignment). Anyhow, if we continue to have a single allocation for struct pwm_chip, the array of pwm_devices belonging to the chip and priv data (which I think is a good idea), then we cannot avoid some memory address calculation because the array size and the size of private data differs per driver (or even per chip). Something like https://lore.kernel.org/linux-iio/20230724110204.46285-3-andriy.shevchenko@linux.intel.com could be used (but I'm unsure if this makes things prettier). The struct_size macro from include/linux/overflow.h could be beneficial though? Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index cfcddf62ab01..3b8d41fdda1b 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -198,7 +198,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip) void *pwmchip_priv(struct pwm_chip *chip) { - return &chip[1]; + return &chip->pwms[chip->npwm]; } EXPORT_SYMBOL_GPL(pwmchip_priv); @@ -208,7 +208,7 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si size_t alloc_size; unsigned int i; - alloc_size = sizeof(*chip) + sizeof_priv; + alloc_size = sizeof(*chip) + npwm * sizeof(struct pwm_device) + sizeof_priv; chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); if (!chip) @@ -217,6 +217,13 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si chip->dev = parent; chip->npwm = npwm; + for (i = 0; i < chip->npwm; i++) { + struct pwm_device *pwm = &chip->pwms[i]; + + pwm->chip = chip; + pwm->hwpwm = i; + } + return chip; } EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); @@ -243,26 +250,15 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) chip->owner = owner; - chip->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL); - if (!chip->pwms) - return -ENOMEM; - mutex_lock(&pwm_lock); ret = idr_alloc(&pwmchip_idr, chip, 0, 0, GFP_KERNEL); if (ret < 0) { mutex_unlock(&pwm_lock); - kfree(chip->pwms); return ret; } chip->id = ret; - for (i = 0; i < chip->npwm; i++) { - struct pwm_device *pwm = &chip->pwms[i]; - - pwm->chip = chip; - pwm->hwpwm = i; - } mutex_unlock(&pwm_lock); @@ -293,8 +289,6 @@ void pwmchip_remove(struct pwm_chip *chip) idr_remove(&pwmchip_idr, chip->id); mutex_unlock(&pwm_lock); - - kfree(chip->pwms); } EXPORT_SYMBOL_GPL(pwmchip_remove); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 3c0da17e193c..fbcba204de44 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -301,7 +301,7 @@ struct pwm_chip { unsigned int of_pwm_n_cells; /* only used internally by the PWM framework */ - struct pwm_device *pwms; + struct pwm_device pwms[]; }; #if IS_ENABLED(CONFIG_PWM)
It's required to not free the memory underlying a requested PWM while a consumer still has a reference to it. While currently a pwm_chip doesn't life long enough in all cases, linking the struct pwm to the pwm_chip results in the right lifetime as soon as the pwmchip is living long enough. This happens with the following commits. Note this is a breaking change for all pwm drivers that don't use pwmchip_alloc(). Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/core.c | 24 +++++++++--------------- include/linux/pwm.h | 2 +- 2 files changed, 10 insertions(+), 16 deletions(-)