Re: [PATCH] regulator: core: Add devres versions of regulator_enable/disable
On Mon, Aug 12, 2019 at 8:51 PM Chuhong Yuan wrote: > > On Mon, Aug 12, 2019 at 7:07 PM Mark Brown wrote: > > > > On Sat, Aug 10, 2019 at 09:44:45AM +0800, Chuhong Yuan wrote: > > > On Fri, Aug 9, 2019 at 11:11 PM Mark Brown wrote: > > > > > > I'm not super keen on managed versions of these functions since they're > > > > very likely to cause reference counting issues between the probe/remove > > > > path and the suspend/resume path which aren't obvious from the code, I'm > > > > especially worried about double frees on release. > > > > > I find that 29 of 31 cases I found call regulator_disable() only when > > > encounter > > > probe failure or in .remove. > > > So I think the devm versions of regulator_enable/disable() will not cause > > > big > > > problems. > > > > There's way more drivers using regulators than that... > > > > I wrote a new coccinelle script to detect all regulator_disable() in .remove, > 101 drivers are found in total. > Within them, 25 drivers cannot benefit from devres version of > regulator_enable() > since they have additional non-devm operations after > regulator_disable() in .remove. I find 6 of 25 can benefit from devm_regulator_enable(). They are included in my previously found 147 cases so I incorrectly skipped them while checking. Therefore, there are 82 cases that can benefit from devm_regulator_enable() and 66 of them(80.5%) only call regulator_disable() when fail in probe or in .remove. > Within the left 76 cases, 60 drivers (79%) only use > regulator_disable() when encounter > probe failure or in .remove. > The left 16 cases mostly use regulator_disable() in _suspend(). > Furthermore, 3 cases of 76 are found to forget to disable regulator > when fail in probe. > So I think a devres version of regulator_enable/disable() has more > benefits than potential > risk. > > > > I even found a driver to forget to disable regulator when encounter > > > probe failure, > > > which is drivers/iio/adc/ti-adc128s052.c. > > > And a devm version of regulator_enable() can prevent such mistakes. > > > > Yes, it's useful for that.
Re: [PATCH] regulator: core: Add devres versions of regulator_enable/disable
On Mon, Aug 12, 2019 at 7:07 PM Mark Brown wrote: > > On Sat, Aug 10, 2019 at 09:44:45AM +0800, Chuhong Yuan wrote: > > On Fri, Aug 9, 2019 at 11:11 PM Mark Brown wrote: > > > > I'm not super keen on managed versions of these functions since they're > > > very likely to cause reference counting issues between the probe/remove > > > path and the suspend/resume path which aren't obvious from the code, I'm > > > especially worried about double frees on release. > > > I find that 29 of 31 cases I found call regulator_disable() only when > > encounter > > probe failure or in .remove. > > So I think the devm versions of regulator_enable/disable() will not cause > > big > > problems. > > There's way more drivers using regulators than that... > I wrote a new coccinelle script to detect all regulator_disable() in .remove, 101 drivers are found in total. Within them, 25 drivers cannot benefit from devres version of regulator_enable() since they have additional non-devm operations after regulator_disable() in .remove. Within the left 76 cases, 60 drivers (79%) only use regulator_disable() when encounter probe failure or in .remove. The left 16 cases mostly use regulator_disable() in _suspend(). Furthermore, 3 cases of 76 are found to forget to disable regulator when fail in probe. So I think a devres version of regulator_enable/disable() has more benefits than potential risk. > > I even found a driver to forget to disable regulator when encounter > > probe failure, > > which is drivers/iio/adc/ti-adc128s052.c. > > And a devm version of regulator_enable() can prevent such mistakes. > > Yes, it's useful for that.
Re: [PATCH] regulator: core: Add devres versions of regulator_enable/disable
On Sat, Aug 10, 2019 at 09:44:45AM +0800, Chuhong Yuan wrote: > On Fri, Aug 9, 2019 at 11:11 PM Mark Brown wrote: > > I'm not super keen on managed versions of these functions since they're > > very likely to cause reference counting issues between the probe/remove > > path and the suspend/resume path which aren't obvious from the code, I'm > > especially worried about double frees on release. > I find that 29 of 31 cases I found call regulator_disable() only when > encounter > probe failure or in .remove. > So I think the devm versions of regulator_enable/disable() will not cause big > problems. There's way more drivers using regulators than that... > I even found a driver to forget to disable regulator when encounter > probe failure, > which is drivers/iio/adc/ti-adc128s052.c. > And a devm version of regulator_enable() can prevent such mistakes. Yes, it's useful for that. signature.asc Description: PGP signature
Re: [PATCH] regulator: core: Add devres versions of regulator_enable/disable
On Fri, Aug 9, 2019 at 11:11 PM Mark Brown wrote: > > On Fri, Aug 09, 2019 at 11:03:52AM +0800, Chuhong Yuan wrote: > > I wrote a coccinelle script to detect possible chances > > of utilizing devm_() APIs to simplify the driver. > > The script found 147 drivers in total and 22 of them > > have be patched. > > > Within the 125 left ones, at least 31 of them (24.8%) > > are hindered from benefiting from devm_() APIs because > > of lack of a devres version of regulator_enable(). > > I'm not super keen on managed versions of these functions since they're > very likely to cause reference counting issues between the probe/remove > path and the suspend/resume path which aren't obvious from the code, I'm > especially worried about double frees on release. I find that 29 of 31 cases I found call regulator_disable() only when encounter probe failure or in .remove. So I think the devm versions of regulator_enable/disable() will not cause big problems. I even found a driver to forget to disable regulator when encounter probe failure, which is drivers/iio/adc/ti-adc128s052.c. And a devm version of regulator_enable() can prevent such mistakes.
Re: [PATCH] regulator: core: Add devres versions of regulator_enable/disable
On Fri, Aug 09, 2019 at 11:03:52AM +0800, Chuhong Yuan wrote: > I wrote a coccinelle script to detect possible chances > of utilizing devm_() APIs to simplify the driver. > The script found 147 drivers in total and 22 of them > have be patched. > Within the 125 left ones, at least 31 of them (24.8%) > are hindered from benefiting from devm_() APIs because > of lack of a devres version of regulator_enable(). I'm not super keen on managed versions of these functions since they're very likely to cause reference counting issues between the probe/remove path and the suspend/resume path which aren't obvious from the code, I'm especially worried about double frees on release. signature.asc Description: PGP signature
[PATCH] regulator: core: Add devres versions of regulator_enable/disable
I wrote a coccinelle script to detect possible chances of utilizing devm_() APIs to simplify the driver. The script found 147 drivers in total and 22 of them have be patched. Within the 125 left ones, at least 31 of them (24.8%) are hindered from benefiting from devm_() APIs because of lack of a devres version of regulator_enable(). Therefore I implemented devm_regulator_enable/disable() to make more drivers possible to use devm_() APIs. Signed-off-by: Chuhong Yuan --- drivers/regulator/devres.c | 55 ++ 1 file changed, 55 insertions(+) diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c index 3ea1c170f840..507151a71fd3 100644 --- a/drivers/regulator/devres.c +++ b/drivers/regulator/devres.c @@ -115,6 +115,61 @@ void devm_regulator_put(struct regulator *regulator) } EXPORT_SYMBOL_GPL(devm_regulator_put); +static void devm_regulator_off(struct device *dev, void *res) +{ + regulator_disable(*(struct regulator **)res); +} + +/** + * devm_regulator_enable - Resource managed regulator_enable() + * @regulator: regulator to enable + * + * Managed regulator_enable(). Regulator enabled is automatically + * disabled on driver detach. See regulator_enable() for more + * information. + */ +int devm_regulator_enable(struct device *dev, struct regulator *regulator) +{ + struct regulator **ptr; + int ret; + + ptr = devres_alloc(devm_regulator_off, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ret = regulator_enable(regulator); + if (!ret) { + *ptr = regulator; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return ret; +} +EXPORT_SYMBOL_GPL(devm_regulator_enable); + +/** + * devm_regulator_disable - Resource managed regulator_disable() + * @regulator: regulator to disable + * + * Disable a regulator enabled by devm_regulator_enable(). + * Normally this function will not need to be called and the + * resource management code will ensure that the regulator is + * disabled. + */ +void devm_regulator_disable(struct regulator *regulator) +{ + int rc; + + rc = devres_release(regulator->dev, devm_regulator_off, + devm_regulator_match, regulator); + + if (rc != 0) + WARN_ON(rc); +} +EXPORT_SYMBOL_GPL(devm_regulator_disable); + struct regulator_bulk_devres { struct regulator_bulk_data *consumers; int num_consumers; -- 2.20.1