| Message ID | 20250129112056.3051949-1-d-gole@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] PM: wakeup: set device_set_wakeup_capable to false in case of error | expand |
On Wed, Jan 29, 2025 at 12:21 PM Dhruva Gole <d-gole@ti.com> wrote: > > In device_init_wakeup enable, we first set device_set_wakeup_capable to > true. However in case the device_wakeup_enable fails for whatever reason, > there was no error handling being done. > Consequenty, there was no cleanup being done to device_set_wakeup_capable. s/Consequenty/Consequently/ > If a certain API is enabling something, it should take care of disabling it > in error scenarios. In this case device_init_wakeup should on it's own > check for errors and clean up accordingly. Why do you think that failing device_wakeup_enable() will always mean that the device is not in fact wakeup capable? > Cc: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > Signed-off-by: Dhruva Gole <d-gole@ti.com> > --- > > This patch was briefly proposed in a related thread [1], where this discussion > was taking place. > That probably got missed due to some confusion around the device_init_wakeup > return value. > > There is infact error returning being done in drivers/base/power/wakeup.c, and > ideally we should be using that info as done in this patch. > > If this patch get accepted, it might even bring forth few hidden bugs > due to missing error handling, and it will also change the patch for > devm_device_init_wakeup() helper slightly[2]. > > [1] https://lore.kernel.org/linux-pm/20241218064335.c72gmw56ogtp36a2@lcpd911/ > [2] https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/ > > --- > include/linux/pm_wakeup.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index d501c09c60cd..ed62a7055a54 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -231,9 +231,13 @@ static inline void pm_wakeup_hard_event(struct device *dev) > */ > static inline int device_init_wakeup(struct device *dev, bool enable) > { > + int err; > if (enable) { > device_set_wakeup_capable(dev, true); > - return device_wakeup_enable(dev); > + err = device_wakeup_enable(dev); > + if (err) > + device_set_wakeup_capable(dev, false); > + return err; > } > device_wakeup_disable(dev); > device_set_wakeup_capable(dev, false); > -- > 2.34.1 >
Hi Rafael, On Jan 29, 2025 at 17:18:24 +0100, Rafael J. Wysocki wrote: > On Wed, Jan 29, 2025 at 12:21 PM Dhruva Gole <d-gole@ti.com> wrote: > > > > In device_init_wakeup enable, we first set device_set_wakeup_capable to > > true. However in case the device_wakeup_enable fails for whatever reason, > > there was no error handling being done. > > Consequenty, there was no cleanup being done to device_set_wakeup_capable. > > s/Consequenty/Consequently/ My bad. I will fix it if we decide to revise this patch. > > > If a certain API is enabling something, it should take care of disabling it > > in error scenarios. In this case device_init_wakeup should on it's own > > check for errors and clean up accordingly. > > Why do you think that failing device_wakeup_enable() will always mean > that the device is not in fact wakeup capable? Well, I was looking at it more from a mirror image perspective. By definition, @enable: Whether or not to enable @dev as a wakeup device. If this is false, then in that path we are marking device "not wakeup capable". Then why not apply the same logic if something goes bad while trying to "enable" the device as wakeup? Why must a device claim to be wakeup capable if device_wakeup_enable went bad? Or, then the other way round, why must we device_set_wakeup_capable false in the case where we just want to disable the device wakeup? It's not like we're taking away the capability itself right? Just disabling it for that moment? I am just trying to make sense of the definiton both ways, enable / disable. > > > Cc: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > --- > > > > This patch was briefly proposed in a related thread [1], where this discussion > > was taking place. > > That probably got missed due to some confusion around the device_init_wakeup > > return value. > > > > There is infact error returning being done in drivers/base/power/wakeup.c, and > > ideally we should be using that info as done in this patch. > > > > If this patch get accepted, it might even bring forth few hidden bugs > > due to missing error handling, and it will also change the patch for > > devm_device_init_wakeup() helper slightly[2]. > > > > [1] https://lore.kernel.org/linux-pm/20241218064335.c72gmw56ogtp36a2@lcpd911/ > > [2] https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/ > > > > --- > > include/linux/pm_wakeup.h | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > > index d501c09c60cd..ed62a7055a54 100644 > > --- a/include/linux/pm_wakeup.h > > +++ b/include/linux/pm_wakeup.h > > @@ -231,9 +231,13 @@ static inline void pm_wakeup_hard_event(struct device *dev) > > */ > > static inline int device_init_wakeup(struct device *dev, bool enable) > > { > > + int err; > > if (enable) { > > device_set_wakeup_capable(dev, true); > > - return device_wakeup_enable(dev); > > + err = device_wakeup_enable(dev); > > + if (err) > > + device_set_wakeup_capable(dev, false); > > + return err; > > } > > device_wakeup_disable(dev); > > device_set_wakeup_capable(dev, false); > > -- > > 2.34.1 > >
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index d501c09c60cd..ed62a7055a54 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -231,9 +231,13 @@ static inline void pm_wakeup_hard_event(struct device *dev) */ static inline int device_init_wakeup(struct device *dev, bool enable) { + int err; if (enable) { device_set_wakeup_capable(dev, true); - return device_wakeup_enable(dev); + err = device_wakeup_enable(dev); + if (err) + device_set_wakeup_capable(dev, false); + return err; } device_wakeup_disable(dev); device_set_wakeup_capable(dev, false);
In device_init_wakeup enable, we first set device_set_wakeup_capable to true. However in case the device_wakeup_enable fails for whatever reason, there was no error handling being done. Consequenty, there was no cleanup being done to device_set_wakeup_capable. If a certain API is enabling something, it should take care of disabling it in error scenarios. In this case device_init_wakeup should on it's own check for errors and clean up accordingly. Cc: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> Signed-off-by: Dhruva Gole <d-gole@ti.com> --- This patch was briefly proposed in a related thread [1], where this discussion was taking place. That probably got missed due to some confusion around the device_init_wakeup return value. There is infact error returning being done in drivers/base/power/wakeup.c, and ideally we should be using that info as done in this patch. If this patch get accepted, it might even bring forth few hidden bugs due to missing error handling, and it will also change the patch for devm_device_init_wakeup() helper slightly[2]. [1] https://lore.kernel.org/linux-pm/20241218064335.c72gmw56ogtp36a2@lcpd911/ [2] https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/ --- include/linux/pm_wakeup.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)