| Message ID | 20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] PM: wakeup: implement devm_device_init_wakeup() helper | expand |
On Dec 14, 2024 at 11:16:52 +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/ > > Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > Changes in V2: > - Utilize the device_init_wakeup() function. You took my suggestion, but forgot to put me in CC I guess :) [...] > +/** > + * 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) Rafael, Should I submit a patch series to convert the regular device_init_wakeup from int to void? This anyway doesn't return anything but 0 today and I can already see some drivers using if(device_init_wakeup) which would essentially be just dead code. I can try and patch that up as well. The fact that this is a return type `int` is quite misleading to it's users I guess? > +{ > + int err; > + > + err = device_init_wakeup(dev, true); > + if (err) { > + device_set_wakeup_capable(dev, false); I don't see any point to this check. I am not sure if there's any case where device_init_wakeup returns anything but 0. Even if it did, setting wakeup_capable false should be handled within init_wakeup and not here. > + return err; With above taken into consideration, you don't need to return err. > + } > + return devm_add_action_or_reset(dev, device_disable_wakeup, dev); This much maybe enough, in that case let's keep the devm_ version as return type int just for this. Also, please CC all involved folks in future revisions. People have filters setup which may prevent them from looking at all emails that come on a list. Best way to grab their attention is by CC-ing them on each patch/ series.
Thank you for your review. On 12/16/24 17:01, Dhruva Gole wrote: > On Dec 14, 2024 at 11:16:52 +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/ >> >> Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> Changes in V2: >> - Utilize the device_init_wakeup() function. > > You took my suggestion, but forgot to put me in CC I guess :) Sorry, added you in CC in the v3 patch. > > [...] >> +/** >> + * 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) > > Rafael, Should I submit a patch series to convert the regular device_init_wakeup from int to void? > This anyway doesn't return anything but 0 today and I can already see > some drivers using if(device_init_wakeup) which would essentially be > just dead code. I can try and patch that up as well. > The fact that this is a return type `int` is quite misleading to it's > users I guess? > >> +{ >> + int err; >> + >> + err = device_init_wakeup(dev, true); >> + if (err) { >> + device_set_wakeup_capable(dev, false); > > I don't see any point to this check. I am not sure if there's any case > where device_init_wakeup returns anything but 0. Even if it did, setting > wakeup_capable false should be handled within init_wakeup and not here. Makes sense. Addressed in the v3 patch. > >> + return err; > > With above taken into consideration, you don't need to return err. > >> + } >> + return devm_add_action_or_reset(dev, device_disable_wakeup, dev); > > This much maybe enough, in that case let's keep the devm_ version as > return type int just for this. > > > Also, please CC all involved folks in future revisions. People have > filters setup which may prevent them from looking at all emails that > come on a list. Best way to grab their attention is by CC-ing them on > each patch/ series. Understood, will keep in mind. > Best, Joe
Joe, On Dec 18, 2024 at 13:10:01 +0900, Joe Hattori wrote: > Thank you for your review. > > On 12/16/24 17:01, Dhruva Gole wrote: > > On Dec 14, 2024 at 11:16:52 +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/ > > > > > > Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > > > --- > > > Changes in V2: > > > - Utilize the device_init_wakeup() function. [...] > > > +/** > > > + * 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) > > > > Rafael, Should I submit a patch series to convert the regular device_init_wakeup from int to void? > > This anyway doesn't return anything but 0 today and I can already see > > some drivers using if(device_init_wakeup) which would essentially be > > just dead code. I can try and patch that up as well. > > The fact that this is a return type `int` is quite misleading to it's > > users I guess? Kindly disregard this, I was looking at the #else part of PM_SLEEP config. There is infact error handling being done in drivers/base/power/wakeup.c > > > > > +{ > > > + int err; > > > + > > > + err = device_init_wakeup(dev, true); > > > + if (err) { > > > + device_set_wakeup_capable(dev, false); > > > > I don't see any point to this check. I am not sure if there's any case > > where device_init_wakeup returns anything but 0. Even if it did, setting > > wakeup_capable false should be handled within init_wakeup and not here. > > Makes sense. Addressed in the v3 patch. Apologies for the confusion, I think the v2 patch itself is correct. I overlooked the drivers/base/power/wakeup.c implementation. You can add my R-by to the v2 version of this patch, Reviewed-by: Dhruva Gole <d-gole@ti.com>
Rafael, On Dec 18, 2024 at 11:11:50 +0530, Dhruva Gole wrote: > Joe, > > On Dec 18, 2024 at 13:10:01 +0900, Joe Hattori wrote: > > Thank you for your review. > > > > On 12/16/24 17:01, Dhruva Gole wrote: > > > On Dec 14, 2024 at 11:16:52 +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/ > > > > > > > > Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > > > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > > > > --- > > > > Changes in V2: > > > > - Utilize the device_init_wakeup() function. > [...] > > > > +/** > > > > + * 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) > > > > > > Rafael, Should I submit a patch series to convert the regular device_init_wakeup from int to void? > > > This anyway doesn't return anything but 0 today and I can already see > > > some drivers using if(device_init_wakeup) which would essentially be > > > just dead code. I can try and patch that up as well. > > > The fact that this is a return type `int` is quite misleading to it's > > > users I guess? > > Kindly disregard this, I was looking at the #else part of PM_SLEEP config. > There is infact error handling being done in drivers/base/power/wakeup.c > > > > > > > > +{ > > > > + int err; > > > > + > > > > + err = device_init_wakeup(dev, true); > > > > + if (err) { > > > > + device_set_wakeup_capable(dev, false); > > > > > > I don't see any point to this check. I am not sure if there's any case > > > where device_init_wakeup returns anything but 0. Even if it did, setting > > > wakeup_capable false should be handled within init_wakeup and not here. > > > > Makes sense. Addressed in the v3 patch. > > Apologies for the confusion, I think the v2 patch itself is correct. > I overlooked the drivers/base/power/wakeup.c implementation. As I review this code further, I am forming the opinion that device_init_wakeup itself needs to be patched up here, because if a certain API is enabling something, it should take care of disabling it in error scenarios. What I am essentially suggesting is something like this: 8<---------------------------------------------------------------------------- device_set_wakeup_capable(dev, true); - return device_wakeup_enable(dev); + err = device_wakeup_enable(dev); + if (err) + device_set_wakeup_capable(dev, false); ----------------------------------------------------------------------------->8 Joe, would you be able to patch up the device_init_wakeup if Rafael is onboard with this idea? You would need to update this patch accordingly as well.
On Mon, Dec 16, 2024 at 9:02 AM Dhruva Gole <d-gole@ti.com> wrote: > > On Dec 14, 2024 at 11:16:52 +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/ > > > > Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > > --- > > Changes in V2: > > - Utilize the device_init_wakeup() function. > > You took my suggestion, but forgot to put me in CC I guess :) > > [...] > > +/** > > + * 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) > > Rafael, Should I submit a patch series to convert the regular device_init_wakeup from int to void? > This anyway doesn't return anything but 0 today and I can already see > some drivers using if(device_init_wakeup) which would essentially be > just dead code. I can try and patch that up as well. > The fact that this is a return type `int` is quite misleading to it's > users I guess? Yes, it would be better to make it void.
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index 222f7530806c..81ad82714ad7 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -240,4 +240,27 @@ static inline int device_init_wakeup(struct device *dev, bool enable) return 0; } +static void device_disable_wakeup(void *dev) +{ + device_init_wakeup(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; + + err = device_init_wakeup(dev, true); + if (err) { + device_set_wakeup_capable(dev, false); + return err; + } + return devm_add_action_or_reset(dev, 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/ Suggested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- Changes in V2: - Utilize the device_init_wakeup() function. --- include/linux/pm_wakeup.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)