| Message ID | 1568244975-66795-1-git-send-email-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: hyperv-keyboard: Add the support of hibernation | expand |
Hi Dexuan, On Wed, Sep 11, 2019 at 11:36:20PM +0000, Dexuan Cui wrote: > We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event() call > does not prevent the system from entering hibernation: the hibernation > is a relatively long process, which can be aborted by the call > pm_wakeup_hard_event(), which is invoked upon keyboard events. > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > This patch is basically a pure Hyper-V specific change and it has a > build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement > suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's > Hyper-V tree's hyperv-next branch: > https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next > > I request this patch should go through Sasha's tree rather than the > input subsystemi's tree. > > Hi Dmitry, can you please Ack? > > drivers/input/serio/hyperv-keyboard.c | 68 ++++++++++++++++++++++++++++++++--- > 1 file changed, 63 insertions(+), 5 deletions(-) > > diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c > index 88ae7c2..277dc4c 100644 > --- a/drivers/input/serio/hyperv-keyboard.c > +++ b/drivers/input/serio/hyperv-keyboard.c > @@ -10,6 +10,7 @@ > #include <linux/hyperv.h> > #include <linux/serio.h> > #include <linux/slab.h> > +#include <linux/suspend.h> > > /* > * Current version 1.0 > @@ -95,6 +96,9 @@ struct hv_kbd_dev { > struct completion wait_event; > spinlock_t lock; /* protects 'started' field */ > bool started; > + > + struct notifier_block pm_nb; > + bool hibernation_in_progress; Why do you use notifier block instead of exposing proper PM methods if you want to support hibernation? Thanks.
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > Sent: Thursday, September 19, 2019 9:18 AM > > Hi Dexuan, > > On Wed, Sep 11, 2019 at 11:36:20PM +0000, Dexuan Cui wrote: > > We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event() > call > > does not prevent the system from entering hibernation: the hibernation > > is a relatively long process, which can be aborted by the call > > pm_wakeup_hard_event(), which is invoked upon keyboard events. > > > > diff --git a/drivers/input/serio/hyperv-keyboard.c > b/drivers/input/serio/hyperv-keyboard.c > > index 88ae7c2..277dc4c 100644 > > --- a/drivers/input/serio/hyperv-keyboard.c > > +++ b/drivers/input/serio/hyperv-keyboard.c > > @@ -10,6 +10,7 @@ > > #include <linux/hyperv.h> > > #include <linux/serio.h> > > #include <linux/slab.h> > > +#include <linux/suspend.h> > > > > /* > > * Current version 1.0 > > @@ -95,6 +96,9 @@ struct hv_kbd_dev { > > struct completion wait_event; > > spinlock_t lock; /* protects 'started' field */ > > bool started; > > + > > + struct notifier_block pm_nb; > > + bool hibernation_in_progress; > > Why do you use notifier block instead of exposing proper PM methods if > you want to support hibernation? > > Dmitry Hi, In the patch I do implement hv_kbd_suspend() and hv_kbd_resume(), and add them into the hv_kbd_drv struct: @@ -416,6 +472,8 @@ static struct hv_driver hv_kbd_drv = { .id_table = id_table, .probe = hv_kbd_probe, .remove = hv_kbd_remove, + .suspend = hv_kbd_suspend, + .resume = hv_kbd_resume, The .suspend and .resume callbacks are inroduced by another patch (which uses the dev_pm_ops struct): 271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation") (which is on the Hyper-V tree's hyperv-next branch: https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/commit/?h=hyperv-next&id=271b2224d42f88870e6b060924ee374871c131fc ) The only purpose of the notifier is to set the variable kbd_dev->hibernation_in_progress to true during the hibernation process. As I explained in the changelog, the hibernation is a long process (which can take 10+ seconds), during which the user may unintentionally touch the keyboard, causing key up/down events, which are still handled by hv_kbd_on_receive(), which calls pm_wakeup_hard_event(), which calls some other functions which increase the global counter "pm_abort_suspend", and hence pm_wakeup_pending() becomes true. 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. 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(). Thanks, -- Dexuan
> From: linux-hyperv-owner@vger.kernel.org > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui > Sent: Friday, September 20, 2019 11:56 PM > To: dmitry.torokhov@gmail.com > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; Stephen Hemminger > <sthemmin@microsoft.com>; sashal@kernel.org; > linux-hyperv@vger.kernel.org; linux-input@vger.kernel.org; > linux-kernel@vger.kernel.org; Michael Kelley <mikelley@microsoft.com> > Subject: RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation > > > From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > > Sent: Thursday, September 19, 2019 9:18 AM > > > > Hi Dexuan, > > > > On Wed, Sep 11, 2019 at 11:36:20PM +0000, Dexuan Cui wrote: > > > We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event() > > call > > > does not prevent the system from entering hibernation: the hibernation > > > is a relatively long process, which can be aborted by the call > > > pm_wakeup_hard_event(), which is invoked upon keyboard events. > > > > > > diff --git a/drivers/input/serio/hyperv-keyboard.c > > b/drivers/input/serio/hyperv-keyboard.c > > > index 88ae7c2..277dc4c 100644 > > > --- a/drivers/input/serio/hyperv-keyboard.c > > > +++ b/drivers/input/serio/hyperv-keyboard.c > > > @@ -10,6 +10,7 @@ > > > #include <linux/hyperv.h> > > > #include <linux/serio.h> > > > #include <linux/slab.h> > > > +#include <linux/suspend.h> > > > > > > /* > > > * Current version 1.0 > > > @@ -95,6 +96,9 @@ struct hv_kbd_dev { > > > struct completion wait_event; > > > spinlock_t lock; /* protects 'started' field */ > > > bool started; > > > + > > > + struct notifier_block pm_nb; > > > + bool hibernation_in_progress; > > > > Why do you use notifier block instead of exposing proper PM methods if > > you want to support hibernation? > > > > Dmitry > > Hi, > In the patch I do implement hv_kbd_suspend() and hv_kbd_resume(), and > add them into the hv_kbd_drv struct: > > @@ -416,6 +472,8 @@ static struct hv_driver hv_kbd_drv = { > .id_table = id_table, > .probe = hv_kbd_probe, > .remove = hv_kbd_remove, > + .suspend = hv_kbd_suspend, > + .resume = hv_kbd_resume, > > The .suspend and .resume callbacks are inroduced by another patch (which > uses the dev_pm_ops struct): > 271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC > drivers for hibernation") > (which is on the Hyper-V tree's hyperv-next branch: > https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/commit/?h=hyperv-next&id=271b2224d42f88870e6b060924ee374871c131fc > ) > > The only purpose of the notifier is to set the variable > kbd_dev->hibernation_in_progress to true during the hibernation process. > > As I explained in the changelog, the hibernation is a long process (which > can take 10+ seconds), during which the user may unintentionally touch > the keyboard, causing key up/down events, which are still handled by > hv_kbd_on_receive(), which calls pm_wakeup_hard_event(), which > calls some other functions which increase the global counter > "pm_abort_suspend", and hence pm_wakeup_pending() becomes true. > > 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. > > 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(). > > -- Dexuan Hi Dmitry, Does my answer make sense? If yes, can I have an Acked-by from you? Thanks, -- Dexuan
On Sat, Sep 21, 2019 at 06:56:04AM +0000, Dexuan Cui wrote: > > From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > > Sent: Thursday, September 19, 2019 9:18 AM > > > > Hi Dexuan, > > > > On Wed, Sep 11, 2019 at 11:36:20PM +0000, Dexuan Cui wrote: > > > We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event() > > call > > > does not prevent the system from entering hibernation: the hibernation > > > is a relatively long process, which can be aborted by the call > > > pm_wakeup_hard_event(), which is invoked upon keyboard events. > > > > > > diff --git a/drivers/input/serio/hyperv-keyboard.c > > b/drivers/input/serio/hyperv-keyboard.c > > > index 88ae7c2..277dc4c 100644 > > > --- a/drivers/input/serio/hyperv-keyboard.c > > > +++ b/drivers/input/serio/hyperv-keyboard.c > > > @@ -10,6 +10,7 @@ > > > #include <linux/hyperv.h> > > > #include <linux/serio.h> > > > #include <linux/slab.h> > > > +#include <linux/suspend.h> > > > > > > /* > > > * Current version 1.0 > > > @@ -95,6 +96,9 @@ struct hv_kbd_dev { > > > struct completion wait_event; > > > spinlock_t lock; /* protects 'started' field */ > > > bool started; > > > + > > > + struct notifier_block pm_nb; > > > + bool hibernation_in_progress; > > > > Why do you use notifier block instead of exposing proper PM methods if > > you want to support hibernation? > > > > Dmitry > > Hi, > In the patch I do implement hv_kbd_suspend() and hv_kbd_resume(), and > add them into the hv_kbd_drv struct: > > @@ -416,6 +472,8 @@ static struct hv_driver hv_kbd_drv = { > .id_table = id_table, > .probe = hv_kbd_probe, > .remove = hv_kbd_remove, > + .suspend = hv_kbd_suspend, > + .resume = hv_kbd_resume, > > The .suspend and .resume callbacks are inroduced by another patch (which > uses the dev_pm_ops struct): > 271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation") > (which is on the Hyper-V tree's hyperv-next branch: > https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/commit/?h=hyperv-next&id=271b2224d42f88870e6b060924ee374871c131fc ) > > The only purpose of the notifier is to set the variable > kbd_dev->hibernation_in_progress to true during the hibernation process. > > As I explained in the changelog, the hibernation is a long process (which > can take 10+ seconds), during which the user may unintentionally touch > the keyboard, causing key up/down events, which are still handled by > hv_kbd_on_receive(), which calls pm_wakeup_hard_event(), which > calls some other functions which increase the global counter > "pm_abort_suspend", and hence pm_wakeup_pending() becomes true. > > 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. > > 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. 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. 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. Thanks.
> 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 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
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 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. If waking the machine up by mouse/keyboard activity is not desired in Hyper-V environment, then simply disable them 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. > > Do you think this idea (notifer + disabling event handling) is acceptable? No, I believe this a hack, that is why I am pushing back on this. > > 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. How long is the prepare() phase on your systems? User may wiggle mouse at any time really, even before the notifier fires up. > > > 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. ¯\_(ツ)_/¯ 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()?). Thanks.
> 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
> From: Dexuan Cui > Sent: Wednesday, October 2, 2019 10:35 PM > > ... > > > > ¯\_(ツ)_/¯ 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. > > -- Dexuan I think I understood now: it looks the vmbus driver should implement a prepare() or freeze(), which calls the hyperv_keyboard driver's prepare() or freeze(), which can set the flag or disable the keyboard event handling. This way we don't need the notifier. Please let me know if I still don't get it right. Thanks, -- Dexuan
On Thu, Oct 03, 2019 at 06:44:04AM +0000, Dexuan Cui wrote: > > From: Dexuan Cui > > Sent: Wednesday, October 2, 2019 10:35 PM > > > ... > > > > > > ¯\_(ツ)_/¯ 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. > > > > -- Dexuan > > I think I understood now: it looks the vmbus driver should implement > a prepare() or freeze(), which calls the hyperv_keyboard driver's > prepare() or freeze(), which can set the flag or disable the keyboard > event handling. This way we don't need the notifier. Right. I think in practice the current suspend implementation can work as freeze() for the HV keyboard, because in suspend you shut off vmbus channel, so there should not be wakeup signals anymore. What you do not want is to have the current resume to be used in place of thaw(), as there you re-enable the vmbus channel and resume sending wakeup requests as you are writing out the hibernation image to storage. I think if vmbus allowed HV keyboard driver to supply empty thaw() and poweroff() implementations, while using suspend() as freeze() and resume() as restore(), it would solve the issue for you. Thanks.
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > Sent: Thursday, October 3, 2019 10:46 AM > > > > I think I understood now: it looks the vmbus driver should implement > > a prepare() or freeze(), which calls the hyperv_keyboard driver's > > prepare() or freeze(), which can set the flag or disable the keyboard > > event handling. This way we don't need the notifier. > > Right. I think in practice the current suspend implementation can work > as freeze() for the HV keyboard, because in suspend you shut off vmbus > channel, so there should not be wakeup signals anymore. What you do not > want is to have the current resume to be used in place of thaw(), as > there you re-enable the vmbus channel and resume sending wakeup requests > as you are writing out the hibernation image to storage. > > I think if vmbus allowed HV keyboard driver to supply empty thaw() and > poweroff() implementations, while using suspend() as freeze() and > resume() as restore(), it would solve the issue for you. > > Dmitry Exactly. I'll have to fix vmbus first, then post a v2 for this patch. Thanks, -- Dexuan
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > Sent: Thursday, October 3, 2019 10:46 AM > To: Dexuan Cui <decui@microsoft.com> > On Thu, Oct 03, 2019 at 06:44:04AM +0000, Dexuan Cui wrote: > > ... > > I think I understood now: it looks the vmbus driver should implement > > a prepare() or freeze(), which calls the hyperv_keyboard driver's > > prepare() or freeze(), which can set the flag or disable the keyboard > > event handling. This way we don't need the notifier. > > Right. I think in practice the current suspend implementation can work > as freeze() for the HV keyboard, because in suspend you shut off vmbus > channel, so there should not be wakeup signals anymore. What you do not > want is to have the current resume to be used in place of thaw(), as > there you re-enable the vmbus channel and resume sending wakeup requests > as you are writing out the hibernation image to storage. > > I think if vmbus allowed HV keyboard driver to supply empty thaw() and > poweroff() implementations, while using suspend() as freeze() and > resume() as restore(), it would solve the issue for you. > > Dmitry Hi Dmitry, Sorry for the late reply! I finally came back on this patch. :-) After I dug more into the issues, this is my understanding now: As I checked the code in drivers/ , it doesn't look commom for a driver to distinguish between thaw() and restore(). Typically a driver uses the macro SET_SYSTEM_SLEEP_PM_OPS() to define the dev_pm_ops, and the macro uses the same function resume_fn as thaw() and restore(). BTW, the macro already uses the same function suspend_fn as suspend() and freeze(), and uses the same function resume_fn as resume() and restore(). And, it looks unusual for a driver to provide an empty thaw(), if any. If I follow your suggestions, I'll have to fix the vmbus driver first (i.e. drivers/hv/vmbus_drv.c: vmbus_pm()) by manually assigning a new function vmbus_thaw() to the thaw() dev_pm_op, and vmbus_thaw() should call the Hyper-V keyboard driver's empty hv_kbd_thaw(), meaning I have to add a .thaw function pointer to the struct hv_driver. IMHO all these changes look too big just for the rare corner cases of the unexpected wake-up issues. More important, even if we make the suggested changes, we actually only fix the unexpected wakeup caused by PMSG_THAW , and there are still some corner cases of failures (please see below). Before any of the dev_pm_op is called, the global counter 'pm_abort_suspend' can be already non-zero, meaning pm_wakeup_pending() is true, so try_to_freeze_tasks() returns -EBUSY, i.e. hibernate() -> freeze_processes() or hibernate() -> hibernation_snapshot() -> freeze_kernel_threads() fails. When the VM boots up and tries to resume from the saved file from disk, before the fresh new kernel's Hyper-V keyboard device is PMSG_QUIESCE'ed, the global counter 'pm_abort_suspend' can be already non-zero (I can cause this scenario by holding the Enter key when the kernel starts), so pm_wakeup_pending() is true, and the below freeze_processes() or device_suspend() can return -EBUSY and the resume process fails. software_resume() -> freeze_processes() pm_wakeup_clear(true) -> Note: this resets the counter to 0. try_to_freeze_tasks -> pm_wakeup_pending load_image_and_restore() -> hibernation_restore() -> dpm_suspend_start() -> dpm_suspend() -> device_suspend() -> __device_suspend() -> pm_wakeup_pending() IMO on a Linux physical machine these issues should happen as well. I think we can fix them separately. For this patch, I suggest we keep it simple like the below: [PATCH v2] Input: hyperv-keyboard: Add the support of hibernation During the suspend process and resume process, if there is any keyboard event, there is a small chance the suspend and the resume process can be aborted because of hv_kbd_on_receive() -> pm_wakeup_hard_event(). This behavior can be avoided by disabling the Hyper-V keyboard device as a wakeup source: echo disabled > /sys/bus/vmbus/drivers/hyperv_keyboard/XXX/power/wakeup (XXX is the device's GUID). Signed-off-by: Dexuan Cui <decui@microsoft.com> --- drivers/input/serio/hyperv-keyboard.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c index e486a8a74c40..df4e9f6f4529 100644 --- a/drivers/input/serio/hyperv-keyboard.c +++ b/drivers/input/serio/hyperv-keyboard.c @@ -259,6 +259,8 @@ static int hv_kbd_connect_to_vsp(struct hv_device *hv_dev) u32 proto_status; int error; + reinit_completion(&kbd_dev->wait_event); + request = &kbd_dev->protocol_req; memset(request, 0, sizeof(struct synth_kbd_protocol_request)); request->header.type = __cpu_to_le32(SYNTH_KBD_PROTOCOL_REQUEST); @@ -380,6 +382,29 @@ static int hv_kbd_remove(struct hv_device *hv_dev) return 0; } +static int hv_kbd_suspend(struct hv_device *hv_dev) +{ + vmbus_close(hv_dev->channel); + + return 0; +} + +static int hv_kbd_resume(struct hv_device *hv_dev) +{ + int ret; + + ret = vmbus_open(hv_dev->channel, + KBD_VSC_SEND_RING_BUFFER_SIZE, + KBD_VSC_RECV_RING_BUFFER_SIZE, + NULL, 0, + hv_kbd_on_channel_callback, + hv_dev); + if (ret == 0) + ret = hv_kbd_connect_to_vsp(hv_dev); + + return ret; +} + static const struct hv_vmbus_device_id id_table[] = { /* Keyboard guid */ { HV_KBD_GUID, }, @@ -393,6 +418,8 @@ static struct hv_driver hv_kbd_drv = { .id_table = id_table, .probe = hv_kbd_probe, .remove = hv_kbd_remove, + .suspend = hv_kbd_suspend, + .resume = hv_kbd_resume, .driver = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, -- I plan to post this as v2. Looking forward to your comments! Thanks, -- Dexuan
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> > Sent: Thursday, October 3, 2019 10:46 AM > ... > Right. I think in practice the current suspend implementation can work > as freeze() for the HV keyboard, because in suspend you shut off vmbus > channel, so there should not be wakeup signals anymore. What you do not > want is to have the current resume to be used in place of thaw(), as > there you re-enable the vmbus channel and resume sending wakeup requests > as you are writing out the hibernation image to storage. > > I think if vmbus allowed HV keyboard driver to supply empty thaw() and > poweroff() implementations, while using suspend() as freeze() and > resume() as restore(), it would solve the issue for you. > > Dmitry BTW, IMO thaw() should not be empty, because drivers/base/power/main.c: pm_op() uses ops->thaw for both PM_EVENT_THAW and PM_EVENT_RECOVER. PMSG_RECOVER is the same as PM_EVENT_RECOVER. If some step in hibernation_snapshot() -> create_image() fails, we call dpm_resume_start(PMSG_RECOVER) at the end of create_image(), and call dpm_resume() in hibernation_snapshot(). An empty thaw() can not bring the device back to normal. Thanks, -- Dexuan
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c index 88ae7c2..277dc4c 100644 --- a/drivers/input/serio/hyperv-keyboard.c +++ b/drivers/input/serio/hyperv-keyboard.c @@ -10,6 +10,7 @@ #include <linux/hyperv.h> #include <linux/serio.h> #include <linux/slab.h> +#include <linux/suspend.h> /* * Current version 1.0 @@ -95,6 +96,9 @@ struct hv_kbd_dev { struct completion wait_event; spinlock_t lock; /* protects 'started' field */ bool started; + + struct notifier_block pm_nb; + bool hibernation_in_progress; }; static void hv_kbd_on_receive(struct hv_device *hv_dev, @@ -168,7 +172,7 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev, * "echo freeze > /sys/power/state" can't really enter the * state because the Enter-UP can trigger a wakeup at once. */ - if (!(info & IS_BREAK)) + if (!(info & IS_BREAK) && !kbd_dev->hibernation_in_progress) pm_wakeup_hard_event(&hv_dev->device); break; @@ -179,10 +183,10 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev, } } -static void hv_kbd_handle_received_packet(struct hv_device *hv_dev, - struct vmpacket_descriptor *desc, - u32 bytes_recvd, - u64 req_id) +static void +hv_kbd_handle_received_packet(struct hv_device *hv_dev, + const struct vmpacket_descriptor *desc, + u32 bytes_recvd, u64 req_id) { struct synth_kbd_msg *msg; u32 msg_sz; @@ -282,6 +286,8 @@ static int hv_kbd_connect_to_vsp(struct hv_device *hv_dev) u32 proto_status; int error; + reinit_completion(&kbd_dev->wait_event); + request = &kbd_dev->protocol_req; memset(request, 0, sizeof(struct synth_kbd_protocol_request)); request->header.type = __cpu_to_le32(SYNTH_KBD_PROTOCOL_REQUEST); @@ -332,6 +338,29 @@ static void hv_kbd_stop(struct serio *serio) spin_unlock_irqrestore(&kbd_dev->lock, flags); } +static int hv_kbd_pm_notify(struct notifier_block *nb, + unsigned long val, void *ign) +{ + struct hv_kbd_dev *kbd_dev; + + kbd_dev = container_of(nb, struct hv_kbd_dev, pm_nb); + + switch (val) { + case PM_HIBERNATION_PREPARE: + case PM_RESTORE_PREPARE: + kbd_dev->hibernation_in_progress = true; + return NOTIFY_OK; + + case PM_POST_HIBERNATION: + case PM_POST_RESTORE: + kbd_dev->hibernation_in_progress = false; + return NOTIFY_OK; + + default: + return NOTIFY_DONE; + } +} + static int hv_kbd_probe(struct hv_device *hv_dev, const struct hv_vmbus_device_id *dev_id) { @@ -380,6 +409,9 @@ static int hv_kbd_probe(struct hv_device *hv_dev, device_init_wakeup(&hv_dev->device, true); + kbd_dev->pm_nb.notifier_call = hv_kbd_pm_notify; + register_pm_notifier(&kbd_dev->pm_nb); + return 0; err_close_vmbus: @@ -394,6 +426,7 @@ static int hv_kbd_remove(struct hv_device *hv_dev) { struct hv_kbd_dev *kbd_dev = hv_get_drvdata(hv_dev); + unregister_pm_notifier(&kbd_dev->pm_nb); serio_unregister_port(kbd_dev->hv_serio); vmbus_close(hv_dev->channel); kfree(kbd_dev); @@ -403,6 +436,29 @@ static int hv_kbd_remove(struct hv_device *hv_dev) return 0; } +static int hv_kbd_suspend(struct hv_device *hv_dev) +{ + vmbus_close(hv_dev->channel); + + return 0; +} + +static int hv_kbd_resume(struct hv_device *hv_dev) +{ + int ret; + + ret = vmbus_open(hv_dev->channel, + KBD_VSC_SEND_RING_BUFFER_SIZE, + KBD_VSC_RECV_RING_BUFFER_SIZE, + NULL, 0, + hv_kbd_on_channel_callback, + hv_dev); + if (ret == 0) + ret = hv_kbd_connect_to_vsp(hv_dev); + + return ret; +} + static const struct hv_vmbus_device_id id_table[] = { /* Keyboard guid */ { HV_KBD_GUID, }, @@ -416,6 +472,8 @@ static int hv_kbd_remove(struct hv_device *hv_dev) .id_table = id_table, .probe = hv_kbd_probe, .remove = hv_kbd_remove, + .suspend = hv_kbd_suspend, + .resume = hv_kbd_resume, .driver = { .probe_type = PROBE_PREFER_ASYNCHRONOUS, },
We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event() call does not prevent the system from entering hibernation: the hibernation is a relatively long process, which can be aborted by the call pm_wakeup_hard_event(), which is invoked upon keyboard events. Signed-off-by: Dexuan Cui <decui@microsoft.com> --- This patch is basically a pure Hyper-V specific change and it has a build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's Hyper-V tree's hyperv-next branch: https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next I request this patch should go through Sasha's tree rather than the input subsystemi's tree. Hi Dmitry, can you please Ack? drivers/input/serio/hyperv-keyboard.c | 68 ++++++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 5 deletions(-)