| Message ID | 20230808171931.944154-2-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:17:51PM +0200, Uwe Kleine-König wrote: > This function allocates a struct pwm_chip and driver data. Compared to > the status quo the split into pwm_chip and driver data is new, otherwise > 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. Proper lifetime tracking is a necessary precondition to > introduce character device support for PWMs (that implements atomic > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm > userspace support). > > The new function pwmchip_priv() (obviously?) only works for chips > allocated with devm_pwmchip_alloc(). > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > .../driver-api/driver-model/devres.rst | 1 + > Documentation/driver-api/pwm.rst | 10 +++---- > drivers/pwm/core.c | 30 ++++++++++++++++--- > include/linux/pwm.h | 5 ++++ > 4 files changed, 37 insertions(+), 9 deletions(-) > > diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst > index 8be086b3f829..73a9ee074737 100644 > --- a/Documentation/driver-api/driver-model/devres.rst > +++ b/Documentation/driver-api/driver-model/devres.rst > @@ -414,6 +414,7 @@ POWER > devm_reboot_mode_unregister() > > PWM > + devm_pwmchip_alloc() > devm_pwmchip_add() > devm_pwm_get() > devm_fwnode_pwm_get() > diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst > index 3fdc95f7a1d1..a3824bd58e4c 100644 > --- a/Documentation/driver-api/pwm.rst > +++ b/Documentation/driver-api/pwm.rst > @@ -134,11 +134,11 @@ to implement the pwm_*() functions itself. This means that it's impossible > to have multiple PWM drivers in the system. For this reason it's mandatory > for new drivers to use the generic PWM framework. > > -A new PWM controller/chip can be added using pwmchip_add() and removed > -again with pwmchip_remove(). pwmchip_add() takes a filled in struct > -pwm_chip as argument which provides a description of the PWM chip, the > -number of PWM devices provided by the chip and the chip-specific > -implementation of the supported PWM operations to the framework. > +A new PWM controller/chip can be allocated using devm_pwmchip_alloc, then added > +using pwmchip_add() and removed again with pwmchip_remove(). pwmchip_add() > +takes a filled in struct pwm_chip as argument which provides a description of > +the PWM chip, the number of PWM devices provided by the chip and the > +chip-specific implementation of the supported PWM operations to the framework. > > When implementing polarity support in a PWM driver, make sure to respect the > signal conventions in the PWM framework. By definition, normal polarity > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 8aa3feec12a9..cfcddf62ab01 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -196,6 +196,31 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > return true; > } > > +void *pwmchip_priv(struct pwm_chip *chip) > +{ > + return &chip[1]; > +} > +EXPORT_SYMBOL_GPL(pwmchip_priv); > + > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) > +{ > + struct pwm_chip *chip; > + size_t alloc_size; > + unsigned int i; > + > + alloc_size = sizeof(*chip) + sizeof_priv; > + > + chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); Are you sure this works the way you want it to? If you allocate using device-managed functions, this memory will be released when the driver is unbound from the device, so we're basically back to square one, aren't we? > + if (!chip) > + return ERR_PTR(-ENOMEM); > + > + chip->dev = parent; > + chip->npwm = npwm; > + > + return chip; > +} > +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); > + > /** > * __pwmchip_add() - register a new PWM chip > * @chip: the PWM chip to add > @@ -208,8 +233,6 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > */ > int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > { > - struct pwm_device *pwm; > - unsigned int i; Am I missing something? You seem to be using this variable in the for loop below, so how can you remove it? Thierry > int ret; > > if (!chip || !chip->dev || !chip->ops || !chip->npwm) > @@ -234,9 +257,8 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > } > > chip->id = ret; > - > for (i = 0; i < chip->npwm; i++) { > - pwm = &chip->pwms[i]; > + struct pwm_device *pwm = &chip->pwms[i]; > > pwm->chip = chip; > pwm->hwpwm = i; > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 6f139784d6f5..3c0da17e193c 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -5,6 +5,7 @@ > #include <linux/err.h> > #include <linux/mutex.h> > #include <linux/of.h> > +#include <linux/compiler_attributes.h> > > struct pwm_chip; > > @@ -380,6 +381,10 @@ static inline void pwm_disable(struct pwm_device *pwm) > int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, > unsigned long timeout); > > +void *pwmchip_priv(struct pwm_chip *chip) __attribute_const__; > + > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv); > + > int __pwmchip_add(struct pwm_chip *chip, struct module *owner); > #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE) > void pwmchip_remove(struct pwm_chip *chip); > -- > 2.40.1 >
Hello Thierry, On Fri, Oct 06, 2023 at 11:23:49AM +0200, Thierry Reding wrote: > On Tue, Aug 08, 2023 at 07:17:51PM +0200, Uwe Kleine-König wrote: > > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) > > +{ > > + struct pwm_chip *chip; > > + size_t alloc_size; > > + unsigned int i; > > + > > + alloc_size = sizeof(*chip) + sizeof_priv; > > + > > + chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); > > Are you sure this works the way you want it to? If you allocate using > device-managed functions, this memory will be released when the driver > is unbound from the device, so we're basically back to square one, > aren't we? After this patch the problem with broken lifetimes isn't fixed. Today the way the pwm_chip is allocated is a problem in each driver. With this function (that indeed suffers from the same problem) the issue can be shifted from each individual driver to this function (patches #2 - #69). Then the lifetime tracking can get fixed in a single place without touching all drivers in one single commit (patches #70, #100 and #101). With the whole series applied this devm_kzalloc is indeed gone -- this can however only happen when all drivers use devm_pwmchip_alloc(). If you have a better idea how to split such a conversion in managable and reviewable patches, I'm all ears. > > + if (!chip) > > + return ERR_PTR(-ENOMEM); > > + > > + chip->dev = parent; > > + chip->npwm = npwm; > > + > > + return chip; > > +} > > +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); > > + > > /** > > * __pwmchip_add() - register a new PWM chip > > * @chip: the PWM chip to add > > @@ -208,8 +233,6 @@ static bool pwm_ops_check(const struct pwm_chip *chip) > > */ > > int __pwmchip_add(struct pwm_chip *chip, struct module *owner) > > { > > - struct pwm_device *pwm; > > - unsigned int i; > > Am I missing something? You seem to be using this variable in the for > loop below, so how can you remove it? Yeah, that series might not be bisectable. This will be fixed in the next iteration. Best regards Uwe
On Fri, Oct 06, 2023 at 12:56:26PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Oct 06, 2023 at 11:23:49AM +0200, Thierry Reding wrote: > > On Tue, Aug 08, 2023 at 07:17:51PM +0200, Uwe Kleine-König wrote: > > > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) > > > +{ > > > + struct pwm_chip *chip; > > > + size_t alloc_size; > > > + unsigned int i; > > > + > > > + alloc_size = sizeof(*chip) + sizeof_priv; > > > + > > > + chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); > > > > Are you sure this works the way you want it to? If you allocate using > > device-managed functions, this memory will be released when the driver > > is unbound from the device, so we're basically back to square one, > > aren't we? > > After this patch the problem with broken lifetimes isn't fixed. Today > the way the pwm_chip is allocated is a problem in each driver. With this > function (that indeed suffers from the same problem) the issue can be > shifted from each individual driver to this function (patches #2 - #69). > Then the lifetime tracking can get fixed in a single place without > touching all drivers in one single commit (patches #70, #100 and #101). > With the whole series applied this devm_kzalloc is indeed gone -- this > can however only happen when all drivers use devm_pwmchip_alloc(). > > If you have a better idea how to split such a conversion in managable > and reviewable patches, I'm all ears. Is there anything that would prevent us from merging the really interesting bits from patch 100 into this first patch? It might make the first patch a bit larger, but at the same time it would immediately become clear why this is useful rather than 99 patches of churn without actual improvements. Thierry
Hello Thierry, On Fri, Oct 06, 2023 at 01:08:18PM +0200, Thierry Reding wrote: > On Fri, Oct 06, 2023 at 12:56:26PM +0200, Uwe Kleine-König wrote: > > On Fri, Oct 06, 2023 at 11:23:49AM +0200, Thierry Reding wrote: > > > On Tue, Aug 08, 2023 at 07:17:51PM +0200, Uwe Kleine-König wrote: > > > > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) > > > > +{ > > > > + struct pwm_chip *chip; > > > > + size_t alloc_size; > > > > + unsigned int i; > > > > + > > > > + alloc_size = sizeof(*chip) + sizeof_priv; > > > > + > > > > + chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); > > > > > > Are you sure this works the way you want it to? If you allocate using > > > device-managed functions, this memory will be released when the driver > > > is unbound from the device, so we're basically back to square one, > > > aren't we? > > > > After this patch the problem with broken lifetimes isn't fixed. Today > > the way the pwm_chip is allocated is a problem in each driver. With this > > function (that indeed suffers from the same problem) the issue can be > > shifted from each individual driver to this function (patches #2 - #69). > > Then the lifetime tracking can get fixed in a single place without > > touching all drivers in one single commit (patches #70, #100 and #101). > > With the whole series applied this devm_kzalloc is indeed gone -- this > > can however only happen when all drivers use devm_pwmchip_alloc(). > > > > If you have a better idea how to split such a conversion in managable > > and reviewable patches, I'm all ears. > > Is there anything that would prevent us from merging the really > interesting bits from patch 100 into this first patch? The main part of patch #100 is that struct pwm_chip gets a member of type struct device. Sure, we *could* do that (and replace all usages of chip->dev by chip->dev.parent), but I see no benefit in that, because you only can use the embedded dev once all drivers don't place it in memory obtained by devm_kzalloc(). So it would only make patch #1 more strange (because why would you use a struct device only to use the parent pointer?). So unless I miss something (which I think I don't) there is nothing that can sensibly be moved in pwmchip_alloc() before all drivers are converted. Best regards Uwe
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index 8be086b3f829..73a9ee074737 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -414,6 +414,7 @@ POWER devm_reboot_mode_unregister() PWM + devm_pwmchip_alloc() devm_pwmchip_add() devm_pwm_get() devm_fwnode_pwm_get() diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst index 3fdc95f7a1d1..a3824bd58e4c 100644 --- a/Documentation/driver-api/pwm.rst +++ b/Documentation/driver-api/pwm.rst @@ -134,11 +134,11 @@ to implement the pwm_*() functions itself. This means that it's impossible to have multiple PWM drivers in the system. For this reason it's mandatory for new drivers to use the generic PWM framework. -A new PWM controller/chip can be added using pwmchip_add() and removed -again with pwmchip_remove(). pwmchip_add() takes a filled in struct -pwm_chip as argument which provides a description of the PWM chip, the -number of PWM devices provided by the chip and the chip-specific -implementation of the supported PWM operations to the framework. +A new PWM controller/chip can be allocated using devm_pwmchip_alloc, then added +using pwmchip_add() and removed again with pwmchip_remove(). pwmchip_add() +takes a filled in struct pwm_chip as argument which provides a description of +the PWM chip, the number of PWM devices provided by the chip and the +chip-specific implementation of the supported PWM operations to the framework. When implementing polarity support in a PWM driver, make sure to respect the signal conventions in the PWM framework. By definition, normal polarity diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 8aa3feec12a9..cfcddf62ab01 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -196,6 +196,31 @@ static bool pwm_ops_check(const struct pwm_chip *chip) return true; } +void *pwmchip_priv(struct pwm_chip *chip) +{ + return &chip[1]; +} +EXPORT_SYMBOL_GPL(pwmchip_priv); + +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) +{ + struct pwm_chip *chip; + size_t alloc_size; + unsigned int i; + + alloc_size = sizeof(*chip) + sizeof_priv; + + chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL); + if (!chip) + return ERR_PTR(-ENOMEM); + + chip->dev = parent; + chip->npwm = npwm; + + return chip; +} +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); + /** * __pwmchip_add() - register a new PWM chip * @chip: the PWM chip to add @@ -208,8 +233,6 @@ static bool pwm_ops_check(const struct pwm_chip *chip) */ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) { - struct pwm_device *pwm; - unsigned int i; int ret; if (!chip || !chip->dev || !chip->ops || !chip->npwm) @@ -234,9 +257,8 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner) } chip->id = ret; - for (i = 0; i < chip->npwm; i++) { - pwm = &chip->pwms[i]; + struct pwm_device *pwm = &chip->pwms[i]; pwm->chip = chip; pwm->hwpwm = i; diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 6f139784d6f5..3c0da17e193c 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -5,6 +5,7 @@ #include <linux/err.h> #include <linux/mutex.h> #include <linux/of.h> +#include <linux/compiler_attributes.h> struct pwm_chip; @@ -380,6 +381,10 @@ static inline void pwm_disable(struct pwm_device *pwm) int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); +void *pwmchip_priv(struct pwm_chip *chip) __attribute_const__; + +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv); + int __pwmchip_add(struct pwm_chip *chip, struct module *owner); #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE) void pwmchip_remove(struct pwm_chip *chip);
This function allocates a struct pwm_chip and driver data. Compared to the status quo the split into pwm_chip and driver data is new, otherwise 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. Proper lifetime tracking is a necessary precondition to introduce character device support for PWMs (that implements atomic setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm userspace support). The new function pwmchip_priv() (obviously?) only works for chips allocated with devm_pwmchip_alloc(). Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- .../driver-api/driver-model/devres.rst | 1 + Documentation/driver-api/pwm.rst | 10 +++---- drivers/pwm/core.c | 30 ++++++++++++++++--- include/linux/pwm.h | 5 ++++ 4 files changed, 37 insertions(+), 9 deletions(-)