Message-ID: <PU1P153MB01696258D9983DF59D68E748BF9F0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
Date:   Thu, 3 Oct 2019 05:35:14 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>
CC:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "sashal@...nel.org" <sashal@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Michael Kelley <mikelley@...rosoft.com>
Subject: RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation

> From: dmitry.torokhov@...il.com <dmitry.torokhov@...il.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@...il.com <dmitry.torokhov@...il.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

Powered by blists - more mailing lists