Received-SPF: pass (google.com: domain of linux-kernel+bounces-104381-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240314075429.1164810-1-d-gole@ti.com> <CAJZ5v0iUt+tudMV-rJ80GR6iNQVv06+054h0UyNgpEOUF6QLRQ@mail.gmail.com>
 <20240314151846.u3r3eaklrl3hf7pi@dhruva> <CAJZ5v0gwkKa+AYgOwydzsKjo=_M56t88PwVo7R+fe-53abAdVw@mail.gmail.com>
 <20240315051828.su2qx57ecyhv3up3@dhruva>
In-Reply-To: <20240315051828.su2qx57ecyhv3up3@dhruva>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Fri, 15 Mar 2024 13:11:11 +0100
Message-ID: <CAJZ5v0jbHwiZemtNAoM-jmgB_58VqmKUkqv4P7qrPkxWzBzMyQ@mail.gmail.com>
Subject: Re: [PATCH] PM: wakeup: Add a missing return case in init_wakeup
To: Dhruva Gole <d-gole@ti.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Tony Lindgren <tony@atomide.com>, theo.lebrun@bootlin.com, 
	Sibi Sankar <quic_sibis@quicinc.com>, Viresh Kumar <viresh.kumar@linaro.org>, 
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Ulf Hansson <ulf.hansson@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 15, 2024 at 6:18=E2=80=AFAM 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=E2=80=AFPM Dhruva Gole <d-gole@ti.com> wro=
te:
> > >
> > > Hi,
> > >
> > > On Mar 14, 2024 at 15:01:36 +0100, Rafael J. Wysocki wrote:
> > > > On Thu, Mar 14, 2024 at 8:55=E2=80=AFAM Dhruva Gole <d-gole@ti.com>=
 wrote:
> > > > >
> > > > > The device_wakeup_disable call can return an error if no dev exis=
ts
> > > > > however this was being ignored. Catch this return value and propa=
gate 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.