Re: [PATCH] ACPICA: Enable sleep button on ACPI legacy wake

From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Anchal Agarwal <anchalag@amazon.com>
Cc: sblbir@amazon.com, fllinden@amazon.com,
	linux-acpi@vger.kernel.org, erik.kaneda@intel.com,
	Bob Moore <robert.moore@intel.com>
Subject: Re: [PATCH] ACPICA: Enable sleep button on ACPI legacy wake
Date: Wed, 15 Jan 2020 11:24:47 +0100	[thread overview]
Message-ID: <2310267.gkPY8ABrMH@kreacher> (raw)
In-Reply-To: <20200102223919.GA22256@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com>

On Thursday, January 2, 2020 11:39:19 PM CET Anchal Agarwal wrote:
> On Thu, Dec 12, 2019 at 11:10:32PM +0100, Rafael J. Wysocki wrote:
> > On Monday, October 29, 2018 6:52:16 PM CET Anchal Agarwal wrote:
> > > Currently we do not see sleep_enable bit set after guest resumes
> > > from hibernation. Hibernation is triggered in guest on receiving
> > > a sleep trigger from the hypervisor(S4 state). We see that power
> > > button is enabled on wake up from S4 state however sleep button
> > > isn't.
> > 
> > I'm not against this change in principle, although it may change behavior
> > in a somewhat unexpected way on some systems.
> > 
> Apologies for late response, I was out on vacation. 
> Can you please elaborate more on that?

See below.

> > > This causes subsequent invocation of sleep state to fail
> > > in the guest.
> > 
> > However, can you explain this in a bit more detail?
> >  
> I can explain this for my particular case, however, any code path going
> through acpi_hw_legacy_wake path will not have sleep button enabled. Why
> is only power button event enabled and not sleep here?

Good question and related to the potentially unexpected behavior part.

Presumably, on some systems the sleep button could not be enabled
automatically as it triggered another suspend right away after that.

That's speculation, though.

> In my case, I am using sleep button event to generate S4 state
> which causes guest VM running on xen to hibernate.

Usually, however, the sleep button is used to trigger transitions into S3
(or suspend-to-idle on systems that support it).

> I have found
> in my debugging process that after first resume when kernel goes 
> through this code path, sleep button event is not enabled and 
> when hypervisor try to generate sci interrupt, its not received in
> the guest since this path does not enable sleep event.

OK

So the patch can be applied AFAICS, but if it is reported to cause problems to
happen, it will need to be reverted.

Please resend the patch with a proper CC list (including Bob, Erik, the ACPICA
devel list etc).

Thanks!