Re: [PATCH] PM: wakeup: Add a missing return case in init_wakeup

[Rafael J. Wysocki]

On Fri, Mar 15, 2024 at 6:18 AM Dhruva Gole <d-gole@xxxxxx> 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@xxxxxx> 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@xxxxxx> 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.