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

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

On Wed, Jan 15, 2020 at 11:24:47AM +0100, Rafael J. Wysocki wrote:
> 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!
Sure can I add your Reviewed-by or Acked-by?

Thanks,
Anchal
> 
> 
>