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 <[email protected]>
---
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 8:55 AM Dhruva Gole <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
--
Best regards,
Dhruva Gole <[email protected]>
On Thu, Mar 14, 2024 at 4:18 PM Dhruva Gole <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.
--
Best regards,
Dhruva Gole <[email protected]>
On Fri, Mar 15, 2024 at 6:18 AM Dhruva Gole <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.