Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()

From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	Kevin Hilman <khilman@kernel.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Simon Horman <horms@verge.net.au>,
	Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH 1/3] PM / core: Assign the wakeup_path status flag in __device_prepare()
Date: Fri, 22 Dec 2017 20:12:16 +0100	[thread overview]
Message-ID: <1742152.iLm4rp6jIy@aspire.rjw.lan> (raw)
In-Reply-To: <CAPDyKFqa5Lt6jrDQd+V_cE+Bz2ZRJRe95-8bbJgV=LbkGPM0mw@mail.gmail.com>

On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, Dec 15, 2017 at 4:56 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> The PM core in the device_prepare() phase, resets the wakeup_path status
> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a
> >> ->suspend() callback for the device would update the device's wakeup
> >> setting, this doesn't become reflected in the wakeup_path status flag.
> >>
> >> In general this isn't a problem, because wakeup settings isn't supposed to
> >> be changed during those system suspend phases. Nevertheless, there are a
> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
> >> indeed called from ->suspend() callbacks.
> >
> > And why is this regarded as correct?
> 
> I am not saying that this behavior is correct. However, I am trying to
> improve the situation, which doesn't hurt or does it?

Adding a workaround for them kind of encourages new code to do the same
thing, which actually may really hurt.  So I assume that these call sites are
all legacy and that's why you don't want to touch them for now, but in that
case the commit message should make it very clear that this is about legacy
only and new code should not call device_set_wakeup_enable() during suspend.

[Something should be printed to the log if wakeup source objects
are created while system suspend is in progress I guess or similar.]

> More importantly, the next patch, which is about the wakeup path,
> depends on this.

Honestly, this sounds like "We have this change we really really would like to
make, but there's incorrect code getting in the way, so let's paper over it
and be done."  Not very nice. :-/

How many drivers actually do call device_set_wakeup_enable() during suspend?

Thanks,
Rafael