From: Dexuan Cui <decui@microsoft.com>
To: "dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>
Cc: KY Srinivasan <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
"sashal@kernel.org" <sashal@kernel.org>,
"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Michael Kelley <mikelley@microsoft.com>
Subject: RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
Date: Thu, 3 Oct 2019 05:35:14 +0000 [thread overview]
Message-ID: <PU1P153MB01696258D9983DF59D68E748BF9F0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20190930230652.GW237523@dtor-ws>
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
> Sent: Monday, September 30, 2019 4:07 PM
>
> On Mon, Sep 30, 2019 at 10:09:27PM +0000, Dexuan Cui wrote:
> > > From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
> > > Sent: Friday, September 27, 2019 5:32 PM
> > > > ...
> > > > pm_wakeup_pending() is tested in a lot of places in the suspend
> > > > process and eventually an unintentional keystroke (or mouse movement,
> > > > when it comes to the Hyper-V mouse driver drivers/hid/hid-hyperv.c)
> > > > causes the whole hibernation process to be aborted. Usually this
> > > > behavior is not expected by the user, I think.
> > >
> > > Why not? If a device is configured as wakeup source, then it activity
> > > should wake up the system, unless you disable it.
> >
> > Generally speaking, I agree, but compared to a physical machine, IMO
> > the scenario is a little different when it comes to a VM running on Hyper-V:
> > on the host there is a window that represents the VM, and the user can
> > unintentionally switch the keyboard input focus to the window (or move
> > the mouse/cursor over the window) and then the host automatically
> > sends some special keystrokes (and mouse events) , and this aborts the
> > hibernation process.
> >
> > And, when it comes to the Hyper-V mouse device, IMO it's easy for the
> > user to unintentionally move the mouse after the "hibernation" button
> > is clicked. I suppose a physical machine would have the same issue, though.
>
> If waking the machine up by mouse/keyboard activity is not desired in
> Hyper-V environment, then simply disable them as wakeup sources.
Sorry for the late reply! I have been sidetracked by something else...
Several years ago, we marked Hyper-V mouse/keyboard devices as wake
sources to fix such a bug: the VM can not be woken up after we run
"echo freeze > /sys/power/state". IMO we should keep the mouse/keyboard
as wakeup sources.
> >
> > > > So, I use the notifier to set the flag variable and with it the driver can
> > > > know when it should not call pm_wakeup_hard_event().
> > >
> > > No, please implement hibernation support properly, as notifier + flag is
> > > a hack.
> >
> > The keyboard/mouse driver can avoid the flag by disabling the
> > keyboard/mouse event handling, but the problem is that they don't know
> > when exactly they should disable the event handling. I think the PM
> > notifier is the only way to tell the drivers a hibernation process is ongoing.
>
> Whatever initiates hibernation (in userspace) can adjust wakeup sources
> as needed if you want them disabled completely.
Good to know this! I just found the userspace is able to disable the Hyper-V
mouse/keyboard as wakeup sources by something like:
echo disabled > /sys/bus/vmbus/devices/XXX/power/wakeup
(XXX is the device GUID).
> >
> > Do you think this idea (notifier + disabling event handling) is acceptable?
>
> No, I believe this a hack, that is why I am pushing back on this.
Ok, I think we can get rid of the notifier completely, and tell the users to disable
the 2 wakeup sources, if they think the wakeup behavior is undesired.
> >
> > If not, then I'll have to remove the notifier completely, and document this as
> > a known issue to the user: when a hibernation process is started, be careful
> > to not switch input focus and not touch the keyboard/mouse until the
> > hibernation process is finished. :-)
> >
> > > In this particular case you do not want to have your
> > > hv_kbd_resume() to be called in place of pm_ops->thaw() as that is what
> > > reenables the keyboard vmbus channel and causes the undesired wakeup
> > > events.
> >
> > This is only part of the issues. Another example: before the
> > pm_ops()->freeze()'s of all the devices are called, pm_wakeup_pending()
> > is already tested in a lot of places (e.g. in try_to_freeze_tasks ()) in the
> > suspend process, and can abort the whole suspend process upon the user's
> > unintentional input focus switch, keystroke and mouse movement.
>
> How long is the prepare() phase on your systems?
I have no specific data, but I know it's fast.
> User may wiggle mouse at any time really, even before the notifier fires up.
This doesn't matter, because the counter "pm_abort_suspend" is cleared at
a later place. The code path is:
hibernate() ->
__pm_notifier_call_chain(PM_HIBERNATION_PREPARE, -1, &nr_calls)
freeze_processes() ->
pm_wakeup_clear() ->
atomic_set(&pm_abort_suspend, 0);
This patch sets the flag in the PM_HIBERNATION_PREPARE notifier, so
there is no race.
Since I'm going to get rid of the notifier, we don't care at all about this now.
> >
> > > Your vmbus implementation should allow individual drivers to
> > > control the set of PM operations that they wish to use, instead of
> > > forcing everything through suspend/resume.
> > >
> > > Dmitry
> >
> > Since the devices are pure software-emulated devices, no PM operation was
> > supported in the past, and now suspend/resume are the only two PM
> operations
> > we're going to support. If the idea (notifier + disabling event handling) is not
> > good enough, we'll have to document the issue to the user, as I described
> above.
>
> ¯\_(ツ)_/¯ If you do not want to implement hibernation properly in vmbus
> code that is totally up to you (have you read in pm.h how freeze() is
> different from suspend()?).
> Dmitry
I understand freeze() is different from suspend(). Here I treat suspend() as a
heavyweight freeze() for simplicity and IMHO the extra cost of time is
neglectable considering the long hibernation process, which can take
5~10+ seconds.
Even if I implement all the pm ops, IMO the issue we're talking about
(i.e. the hibernation process can be aborted by user's keyboard/mouse
activities) still exists. Actually I think a physical Linux machine should have
the same issue.
In practice, IMO the issue is not a big concern, as the VM usually runs in
a remote data center, and the user has no access to the VM's
keyboard/mouse. :-)
I hope I understood your comments. I'll post a v2 without the notifier.
Please Ack the v2 if it looks good to you.
Thanks,
-- Dexuan
next prev parent reply other threads:[~2019-10-03 5:35 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-11 23:36 [PATCH] Input: hyperv-keyboard: Add the support of hibernation Dexuan Cui
2019-09-19 16:17 ` dmitry.torokhov
2019-09-21 6:56 ` Dexuan Cui
2019-09-25 19:49 ` Dexuan Cui
2019-09-28 0:31 ` dmitry.torokhov
2019-09-30 22:09 ` Dexuan Cui
2019-09-30 23:06 ` dmitry.torokhov
2019-10-03 5:35 ` Dexuan Cui [this message]
2019-10-03 6:44 ` Dexuan Cui
2019-10-03 17:45 ` dmitry.torokhov
2019-10-03 18:18 ` Dexuan Cui
2019-11-05 5:18 ` Dexuan Cui
2019-11-05 5:43 ` Dexuan Cui
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=PU1P153MB01696258D9983DF59D68E748BF9F0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM \
--to=decui@microsoft.com \
--cc=dmitry.torokhov@gmail.com \
--cc=haiyangz@microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mikelley@microsoft.com \
--cc=sashal@kernel.org \
--cc=sthemmin@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).