| Message ID | 20240314075429.1164810-1-d-gole@ti.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PM: wakeup: Add a missing return case in init_wakeup | expand |
On Thu, Mar 14, 2024 at 8:55 AM Dhruva Gole <d-gole@ti.com> wrote: > > The device_wakeup_disable call can return an error if no dev exists > however this was being ignored. Catch this return value and propagate it > onward in device_init_wakeup. Why does this matter to the callers of device_init_wakeup()? > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > --- > include/linux/pm_wakeup.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > index 6eb9adaef52b..64c7db35e693 100644 > --- a/include/linux/pm_wakeup.h > +++ b/include/linux/pm_wakeup.h > @@ -232,14 +232,15 @@ static inline void pm_wakeup_hard_event(struct device *dev) > */ > static inline int device_init_wakeup(struct device *dev, bool enable) > { > + int ret; > if (enable) { > device_set_wakeup_capable(dev, true); > - return device_wakeup_enable(dev); > + ret = device_wakeup_enable(dev); > } else { > - device_wakeup_disable(dev); > + ret = device_wakeup_disable(dev); > device_set_wakeup_capable(dev, false); > - return 0; > } > + return ret; > } > > #endif /* _LINUX_PM_WAKEUP_H */ > > base-commit: 9bb9b28d0568991b1d63e66fe75afa5f97ad1156 > -- > 2.34.1 >
Hi, On Mar 14, 2024 at 15:01:36 +0100, Rafael J. Wysocki wrote: > On Thu, Mar 14, 2024 at 8:55 AM Dhruva Gole <d-gole@ti.com> wrote: > > > > The device_wakeup_disable call can return an error if no dev exists > > however this was being ignored. Catch this return value and propagate it > > onward in device_init_wakeup. > > Why does this matter to the callers of device_init_wakeup()? If atall !dev->power.can_wakeup then the caller should know something is funny right? > > > > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > --- > > include/linux/pm_wakeup.h | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h > > index 6eb9adaef52b..64c7db35e693 100644 > > --- a/include/linux/pm_wakeup.h > > +++ b/include/linux/pm_wakeup.h > > @@ -232,14 +232,15 @@ static inline void pm_wakeup_hard_event(struct device *dev) > > */ > > static inline int device_init_wakeup(struct device *dev, bool enable) > > { > > + int ret; > > if (enable) { > > device_set_wakeup_capable(dev, true); > > - return device_wakeup_enable(dev); > > + ret = device_wakeup_enable(dev); > > } else { > > - device_wakeup_disable(dev); > > + ret = device_wakeup_disable(dev); > > device_set_wakeup_capable(dev, false); > > - return 0; > > } > > + return ret; > > } > > > > #endif /* _LINUX_PM_WAKEUP_H */ > > > > base-commit: 9bb9b28d0568991b1d63e66fe75afa5f97ad1156 > > -- > > 2.34.1 > >
On Thu, Mar 14, 2024 at 4:18 PM Dhruva Gole <d-gole@ti.com> wrote: > > Hi, > > On Mar 14, 2024 at 15:01:36 +0100, Rafael J. Wysocki wrote: > > On Thu, Mar 14, 2024 at 8:55 AM Dhruva Gole <d-gole@ti.com> wrote: > > > > > > The device_wakeup_disable call can return an error if no dev exists > > > however this was being ignored. Catch this return value and propagate it > > > onward in device_init_wakeup. > > > > Why does this matter to the callers of device_init_wakeup()? > > If atall !dev->power.can_wakeup then the caller should know something is > funny right? What would the caller do with this information? They attempted to disable wakeup on a device that doesn't exist or is not wake-capable, and so what?
On Mar 14, 2024 at 16:29:36 +0100, Rafael J. Wysocki wrote: > On Thu, Mar 14, 2024 at 4:18 PM Dhruva Gole <d-gole@ti.com> wrote: > > > > Hi, > > > > On Mar 14, 2024 at 15:01:36 +0100, Rafael J. Wysocki wrote: > > > On Thu, Mar 14, 2024 at 8:55 AM Dhruva Gole <d-gole@ti.com> wrote: > > > > > > > > The device_wakeup_disable call can return an error if no dev exists > > > > however this was being ignored. Catch this return value and propagate it > > > > onward in device_init_wakeup. > > > > > > Why does this matter to the callers of device_init_wakeup()? > > > > If atall !dev->power.can_wakeup then the caller should know something is > > funny right? > > What would the caller do with this information? > > They attempted to disable wakeup on a device that doesn't exist or is > not wake-capable, and so what? Using drivers/char/hw_random/xgene-rng.c as an example, we can atleast print a warning or something to make the user aware of an unclean state. Is the argument here that if the caller can't do anything meaningful then what's the point of returning any error? If so, then my preference would be just to propagate as much information upward from the stack whether the caller can make use of that error and in what way is upto the caller, if nothing else then even a warn or error print is still useful piece of information. However if it's useless to return anything from device_wakeup_disable then we might as well make that function a void or something and avoid any confusion as to if there's any point in returning error at that point.
On Fri, Mar 15, 2024 at 6:18 AM Dhruva Gole <d-gole@ti.com> wrote: > > On Mar 14, 2024 at 16:29:36 +0100, Rafael J. Wysocki wrote: > > On Thu, Mar 14, 2024 at 4:18 PM Dhruva Gole <d-gole@ti.com> wrote: > > > > > > Hi, > > > > > > On Mar 14, 2024 at 15:01:36 +0100, Rafael J. Wysocki wrote: > > > > On Thu, Mar 14, 2024 at 8:55 AM Dhruva Gole <d-gole@ti.com> wrote: > > > > > > > > > > The device_wakeup_disable call can return an error if no dev exists > > > > > however this was being ignored. Catch this return value and propagate it > > > > > onward in device_init_wakeup. > > > > > > > > Why does this matter to the callers of device_init_wakeup()? > > > > > > If atall !dev->power.can_wakeup then the caller should know something is > > > funny right? > > > > What would the caller do with this information? > > > > They attempted to disable wakeup on a device that doesn't exist or is > > not wake-capable, and so what? > > Using drivers/char/hw_random/xgene-rng.c as an example, we can atleast > print a warning or something to make the user aware of an unclean state. > > Is the argument here that if the caller can't do anything meaningful > then what's the point of returning any error? > > If so, then my preference would be just to propagate as much information > upward from the stack whether the caller can make use of that error and > in what way is upto the caller, if nothing else then even a warn or > error print is still useful piece of information. I'm not making a general argument, just talking about this particular case. The only error code returned by device_wakeup_disable() is -EINVAL and it is returned when nothing had to be done because it was not applicable to the argument passed by the caller. Quite frankly, I don't see why any caller of device_init_wakeup() passing false as its second argument would be interested in handling that error. > However if it's useless to return anything from device_wakeup_disable > then we might as well make that function a void or something and avoid > any confusion as to if there's any point in returning error at that > point. That would work for me.
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h index 6eb9adaef52b..64c7db35e693 100644 --- a/include/linux/pm_wakeup.h +++ b/include/linux/pm_wakeup.h @@ -232,14 +232,15 @@ static inline void pm_wakeup_hard_event(struct device *dev) */ static inline int device_init_wakeup(struct device *dev, bool enable) { + int ret; if (enable) { device_set_wakeup_capable(dev, true); - return device_wakeup_enable(dev); + ret = device_wakeup_enable(dev); } else { - device_wakeup_disable(dev); + ret = device_wakeup_disable(dev); device_set_wakeup_capable(dev, false); - return 0; } + return ret; } #endif /* _LINUX_PM_WAKEUP_H */
The device_wakeup_disable call can return an error if no dev exists however this was being ignored. Catch this return value and propagate it onward in device_init_wakeup. Signed-off-by: Dhruva Gole <d-gole@ti.com> --- include/linux/pm_wakeup.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) base-commit: 9bb9b28d0568991b1d63e66fe75afa5f97ad1156