Message-ID: <PU1P153MB0169C315F7F9EBEBED4C7A7DBF820@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
Date:   Mon, 30 Sep 2019 22:09:27 +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: 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 differnet 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.

> > 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.

Do you think this idea (notifer + disabling event handling) is acceptable?

If not, then I'll have to remove the notifer 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.

> 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 (notifer + disabling event handling) is not
good enough, we'll have to document the issue to the user, as I described above.

Thanks,
-- Dexuan

Powered by blists - more mailing lists