Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Input: hyperv-keyboard: Add the support of hibernation
@ 2019-09-11 23:36 Dexuan Cui
  2019-09-19 16:17 ` dmitry.torokhov
  0 siblings, 1 reply; 11+ messages in thread
From: Dexuan Cui @ 2019-09-11 23:36 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	dmitry.torokhov, linux-hyperv, linux-input, linux-kernel,
	Michael Kelley
  Cc: Dexuan Cui

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;
 };
 
 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,
 	},
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  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
  0 siblings, 1 reply; 11+ messages in thread
From: dmitry.torokhov @ 2019-09-19 16:17 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  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
  0 siblings, 2 replies; 11+ messages in thread
From: Dexuan Cui @ 2019-09-21  6:56 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-09-21  6:56   ` Dexuan Cui
@ 2019-09-25 19:49     ` Dexuan Cui
  2019-09-28  0:31     ` dmitry.torokhov
  1 sibling, 0 replies; 11+ messages in thread
From: Dexuan Cui @ 2019-09-25 19:49 UTC (permalink / raw)
  To: Dexuan Cui, dmitry.torokhov
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  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
  1 sibling, 1 reply; 11+ messages in thread
From: dmitry.torokhov @ 2019-09-28  0:31 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-09-28  0:31     ` dmitry.torokhov
@ 2019-09-30 22:09       ` Dexuan Cui
  2019-09-30 23:06         ` dmitry.torokhov
  0 siblings, 1 reply; 11+ messages in thread
From: Dexuan Cui @ 2019-09-30 22:09 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-09-30 22:09       ` Dexuan Cui
@ 2019-09-30 23:06         ` dmitry.torokhov
  2019-10-03  5:35           ` Dexuan Cui
  0 siblings, 1 reply; 11+ messages in thread
From: dmitry.torokhov @ 2019-09-30 23:06 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-09-30 23:06         ` dmitry.torokhov
@ 2019-10-03  5:35           ` Dexuan Cui
  2019-10-03  6:44             ` Dexuan Cui
  0 siblings, 1 reply; 11+ messages in thread
From: Dexuan Cui @ 2019-10-03  5:35 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-10-03  5:35           ` Dexuan Cui
@ 2019-10-03  6:44             ` Dexuan Cui
  2019-10-03 17:45               ` dmitry.torokhov
  0 siblings, 1 reply; 11+ messages in thread
From: Dexuan Cui @ 2019-10-03  6:44 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-10-03  6:44             ` Dexuan Cui
@ 2019-10-03 17:45               ` dmitry.torokhov
  2019-10-03 18:18                 ` Dexuan Cui
  0 siblings, 1 reply; 11+ messages in thread
From: dmitry.torokhov @ 2019-10-03 17:45 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
  2019-10-03 17:45               ` dmitry.torokhov
@ 2019-10-03 18:18                 ` Dexuan Cui
  0 siblings, 0 replies; 11+ messages in thread
From: Dexuan Cui @ 2019-10-03 18:18 UTC (permalink / raw)
  To: dmitry.torokhov
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, sashal,
	linux-hyperv, linux-input, linux-kernel, Michael Kelley

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2019-10-03  6:44             ` Dexuan Cui
2019-10-03 17:45               ` dmitry.torokhov
2019-10-03 18:18                 ` Dexuan Cui

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org linux-hyperv@archiver.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox