| Message ID | 20241213035235.2479642-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PM: wakeup: implement devm_device_init_wakeup() helper | expand |
On Dec 13, 2024 at 12:52:35 +0900, Joe Hattori wrote: > Some drivers that enable device wakeup fail to properly disable it > during their cleanup, which results in a memory leak. > > To address this, introduce devm_device_init_wakeup(), a managed variant > of device_init_wakeup(dev, true). With this managed helper, wakeup > functionality will be automatically disabled when the device is > released, ensuring a more reliable cleanup process. > > This need for this addition arose during a previous discussion [1]. > > [1]: > https://lore.kernel.org/linux-rtc/20241212100403.3799667-1-joe@pf.is.s.u-tokyo.ac.jp/ CC Alexandre who I see is an important part of this thread. > > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > include/linux/pm_wakeup.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index 222f7530806c..baf4f982858a 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -240,4 +240,31 @@ static inline int device_init_wakeup(struct device *dev, bool enable) > return 0; > } > > +static void devm_device_disable_wakeup(void *data) > +{ > + struct device *dev = data; > + > + device_wakeup_disable(dev); > + device_set_wakeup_capable(dev, false); > +} > + > +/** > + * devm_device_init_wakeup - Resource managed device wakeup initialization. > + * @dev: Device to handle. > + * > + * This function is the devm managed version of device_init_wakeup(dev, true). > + */ > +static inline int devm_device_init_wakeup(struct device *dev) > +{ > + int err; > + > + device_set_wakeup_capable(dev, true); > + err = device_wakeup_enable(dev); > + if (err) { > + device_set_wakeup_capable(dev, false); > + return err; > + } > + return devm_add_action_or_reset(dev, devm_device_disable_wakeup, dev); Why not just call in device_init_wakeup which already does all this for you? Then the devm_disable will just be device_init_wakeup(dev, false); See for eg. how runtime pm does it: int devm_pm_runtime_enable(struct device *dev) { pm_runtime_enable(dev);
On Dec 13, 2024 at 14:56:42 +0530, Dhruva Gole wrote: > On Dec 13, 2024 at 12:52:35 +0900, Joe Hattori wrote: > > Some drivers that enable device wakeup fail to properly disable it > > during their cleanup, which results in a memory leak. > > > > To address this, introduce devm_device_init_wakeup(), a managed variant > > of device_init_wakeup(dev, true). With this managed helper, wakeup > > functionality will be automatically disabled when the device is > > released, ensuring a more reliable cleanup process. > > > > This need for this addition arose during a previous discussion [1]. > > > > [1]: > > https://lore.kernel.org/linux-rtc/20241212100403.3799667-1-joe@pf.is.s.u-tokyo.ac.jp/ > > CC Alexandre who I see is an important part of this thread. Also, please use Suggested-by: tag as mentioned in https://www.kernel.org/doc/html/latest/process/submitting-patches.html A Suggested-by: tag indicates that the patch idea is suggested by the person named and ensures credit to the person for the idea.
Hi Dhruva, Thank you for your review. On 12/13/24 18:26, Dhruva Gole wrote: > On Dec 13, 2024 at 12:52:35 +0900, Joe Hattori wrote: >> Some drivers that enable device wakeup fail to properly disable it >> during their cleanup, which results in a memory leak. >> >> To address this, introduce devm_device_init_wakeup(), a managed variant >> of device_init_wakeup(dev, true). With this managed helper, wakeup >> functionality will be automatically disabled when the device is >> released, ensuring a more reliable cleanup process. >> >> This need for this addition arose during a previous discussion [1]. >> >> [1]: >> https://lore.kernel.org/linux-rtc/20241212100403.3799667-1-joe@pf.is.s.u-tokyo.ac.jp/ > > CC Alexandre who I see is an important part of this thread. Yes, just sent a v2 patch with Alexandre in CC and in the "Suggested-by" tag. > >> >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> include/linux/pm_wakeup.h | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h >> index 222f7530806c..baf4f982858a 100644 >> --- a/include/linux/pm_wakeup.h >> +++ b/include/linux/pm_wakeup.h >> @@ -240,4 +240,31 @@ static inline int device_init_wakeup(struct device *dev, bool enable) >> return 0; >> } >> >> +static void devm_device_disable_wakeup(void *data) >> +{ >> + struct device *dev = data; >> + >> + device_wakeup_disable(dev); >> + device_set_wakeup_capable(dev, false); >> +} >> + >> +/** >> + * devm_device_init_wakeup - Resource managed device wakeup initialization. >> + * @dev: Device to handle. >> + * >> + * This function is the devm managed version of device_init_wakeup(dev, true). >> + */ >> +static inline int devm_device_init_wakeup(struct device *dev) >> +{ >> + int err; >> + >> + device_set_wakeup_capable(dev, true); >> + err = device_wakeup_enable(dev); >> + if (err) { >> + device_set_wakeup_capable(dev, false); >> + return err; >> + } >> + return devm_add_action_or_reset(dev, devm_device_disable_wakeup, dev); > > Why not just call in device_init_wakeup which already does all this for > you? Makes sense, applied in the v2 patch. > > > Then the devm_disable will just be device_init_wakeup(dev, false); > > > See for eg. how runtime pm does it: > > int devm_pm_runtime_enable(struct device *dev) > { > pm_runtime_enable(dev); > Best, Joe
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index 222f7530806c..baf4f982858a 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -240,4 +240,31 @@ static inline int device_init_wakeup(struct device *dev, bool enable) return 0; } +static void devm_device_disable_wakeup(void *data) +{ + struct device *dev = data; + + device_wakeup_disable(dev); + device_set_wakeup_capable(dev, false); +} + +/** + * devm_device_init_wakeup - Resource managed device wakeup initialization. + * @dev: Device to handle. + * + * This function is the devm managed version of device_init_wakeup(dev, true). + */ +static inline int devm_device_init_wakeup(struct device *dev) +{ + int err; + + device_set_wakeup_capable(dev, true); + err = device_wakeup_enable(dev); + if (err) { + device_set_wakeup_capable(dev, false); + return err; + } + return devm_add_action_or_reset(dev, devm_device_disable_wakeup, dev); +} + #endif /* _LINUX_PM_WAKEUP_H */
Some drivers that enable device wakeup fail to properly disable it during their cleanup, which results in a memory leak. To address this, introduce devm_device_init_wakeup(), a managed variant of device_init_wakeup(dev, true). With this managed helper, wakeup functionality will be automatically disabled when the device is released, ensuring a more reliable cleanup process. This need for this addition arose during a previous discussion [1]. [1]: https://lore.kernel.org/linux-rtc/20241212100403.3799667-1-joe@pf.is.s.u-tokyo.ac.jp/ Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- include/linux/pm_wakeup.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)