[PATCH v4 0/3] Disable Suspend-to-Idle in Hyper-V and Fix Hibernation Interruptions

[PATCH v4 0/3] Disable Suspend-to-Idle in Hyper-V and Fix Hibernation Interruptions

[Erni Sri Satya Vennela]

It has been reported that Hyper-V VM users can unintentionally abort
hibernation by mouse or keyboard movements. To address this issue,
we have decided to remove the wakeup events for the Hyper-V keyboard
and mouse driver. However, this change introduces another problem: 
Suspend-to-Idle brings the system down with no method to wake it back up.

Given that there are no real users of Suspend-to-Idle in Hyper-V,
we have decided to disable this feature for VMBus. This results in:

$echo freeze > /sys/power/state
> bash: echo: write error: Operation not supported

The keyboard and mouse were previously registered as wakeup sources to
interrupt the freeze operation in a VM. Since the freeze operation itself
is no longer supported, we are disabling them as wakeup events.

This patchset ensures that the system remains stable and prevents
unintended interruptions during hibernation.
---
Changes in v4:
* Make keyboard and mouse wakeup capable by adding 
  device_set_wakeup_capable but not enabling them as wakeup sources.
  https://lore.kernel.org/lkml/Zv-j0qtWXsDz4Hah@google.com/
* Remove device_wakeup_init() call in mousevsc_remove()
v3:https://lore.kernel.org/lkml/1727685708-3524-1-git-send-email-ernis@linux.microsoft.com/

Changes in v3:
* Add 'Cc: stable@vger.kernel.org' in sign-off area.
v2:https://lore.kernel.org/lkml/1727683917-31485-1-git-send-email-ernis@linux.microsoft.com/

Changes in v2:
* Add "#define vmbus_freeze NULL" when CONFIG_PM_SLEEP is not 
  enabled.
* Change commit message to clarify that this change is specifc to
  Hyper-V based VMs.
v1:https://lore.kernel.org/lkml/1726176470-13133-1-git-send-email-ernis@linux.microsoft.com/
---

Erni Sri Satya Vennela (3):
  Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus
  Input: hyperv-keyboard - disable as wakeup source
  HID: hyperv: disable as wakeup source

 drivers/hid/hid-hyperv.c              |  3 +--
 drivers/hv/vmbus_drv.c                | 16 +++++++++++++++-
 drivers/input/serio/hyperv-keyboard.c |  2 +-
 3 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus

[Erni Sri Satya Vennela]

This change is specific to Hyper-V VM user.
If the Virtual Machine Connection window is focused,
a Hyper-V VM user can unintentionally touch the keyboard/mouse
when the VM is hibernating or resuming, and consequently the
hibernation or resume operation can be aborted unexpectedly.
Fix the issue by no longer registering the keyboard/mouse as
wakeup devices (see the other two patches for the
changes to drivers/input/serio/hyperv-keyboard.c and
drivers/hid/hid-hyperv.c).

The keyboard/mouse were registered as wakeup devices because the
VM needs to be woken up from the Suspend-to-Idle state after
a user runs "echo freeze > /sys/power/state". It seems like
the Suspend-to-Idle feature has no real users in practice, so
let's no longer support that by returning -EOPNOTSUPP if a
user tries to use that.

Fixes: 1a06d017fb3f ("Drivers: hv: vmbus: Fix Suspend-to-Idle for Generation-2 VM")
Cc: stable@vger.kernel.org
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>>
---
Changes in v4:
* No change

Changes in v3:
* Add "Cc: stable@vger.kernel.org" in sign-off area.

Changes in v2:
* Add "#define vmbus_freeze NULL" when CONFIG_PM_SLEEP is not 
  enabled.
---
 drivers/hv/vmbus_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 6d89d37b069a..4df6b12bf6a1 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -900,6 +900,19 @@ static void vmbus_shutdown(struct device *child_device)
 }
 
 #ifdef CONFIG_PM_SLEEP
+/*
+ * vmbus_freeze - Suspend-to-Idle
+ */
+static int vmbus_freeze(struct device *child_device)
+{
+/*
+ * Do not support Suspend-to-Idle ("echo freeze > /sys/power/state") as
+ * that would require registering the Hyper-V synthetic mouse/keyboard
+ * devices as wakeup devices, which can abort hibernation/resume unexpectedly.
+ */
+	return -EOPNOTSUPP;
+}
+
 /*
  * vmbus_suspend - Suspend a vmbus device
  */
@@ -938,6 +951,7 @@ static int vmbus_resume(struct device *child_device)
 	return drv->resume(dev);
 }
 #else
+#define vmbus_freeze NULL
 #define vmbus_suspend NULL
 #define vmbus_resume NULL
 #endif /* CONFIG_PM_SLEEP */
@@ -969,7 +983,7 @@ static void vmbus_device_release(struct device *device)
  */
 
 static const struct dev_pm_ops vmbus_pm = {
-	.suspend_noirq	= NULL,
+	.suspend_noirq  = vmbus_freeze,
 	.resume_noirq	= NULL,
 	.freeze_noirq	= vmbus_suspend,
 	.thaw_noirq	= vmbus_resume,
-- 
2.34.1

Re: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus

[Wei Liu]

On Mon, Dec 16, 2024 at 08:17:07PM -0800, Erni Sri Satya Vennela wrote:
> This change is specific to Hyper-V VM user.
> If the Virtual Machine Connection window is focused,
> a Hyper-V VM user can unintentionally touch the keyboard/mouse
> when the VM is hibernating or resuming, and consequently the
> hibernation or resume operation can be aborted unexpectedly.
> Fix the issue by no longer registering the keyboard/mouse as
> wakeup devices (see the other two patches for the
> changes to drivers/input/serio/hyperv-keyboard.c and
> drivers/hid/hid-hyperv.c).
> 
> The keyboard/mouse were registered as wakeup devices because the
> VM needs to be woken up from the Suspend-to-Idle state after
> a user runs "echo freeze > /sys/power/state". It seems like
> the Suspend-to-Idle feature has no real users in practice, so
> let's no longer support that by returning -EOPNOTSUPP if a
> user tries to use that.
> 
> Fixes: 1a06d017fb3f ("Drivers: hv: vmbus: Fix Suspend-to-Idle for Generation-2 VM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>>
> ---
> Changes in v4:
> * No change
> 
> Changes in v3:
> * Add "Cc: stable@vger.kernel.org" in sign-off area.
> 
> Changes in v2:
> * Add "#define vmbus_freeze NULL" when CONFIG_PM_SLEEP is not 
>   enabled.
> ---
>  drivers/hv/vmbus_drv.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 6d89d37b069a..4df6b12bf6a1 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -900,6 +900,19 @@ static void vmbus_shutdown(struct device *child_device)
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> +/*
> + * vmbus_freeze - Suspend-to-Idle
> + */
> +static int vmbus_freeze(struct device *child_device)
> +{
> +/*
> + * Do not support Suspend-to-Idle ("echo freeze > /sys/power/state") as
> + * that would require registering the Hyper-V synthetic mouse/keyboard
> + * devices as wakeup devices, which can abort hibernation/resume unexpectedly.
> + */

The indentation is off. I can fix it for you if the series gets
sufficient reviews. Please fix it yourself if you end up posting another
round.

Thanks,
Wei.

> +	return -EOPNOTSUPP;
> +}
> +
>  /*
>   * vmbus_suspend - Suspend a vmbus device
>   */
> @@ -938,6 +951,7 @@ static int vmbus_resume(struct device *child_device)
>  	return drv->resume(dev);
>  }
>  #else
> +#define vmbus_freeze NULL
>  #define vmbus_suspend NULL
>  #define vmbus_resume NULL
>  #endif /* CONFIG_PM_SLEEP */
> @@ -969,7 +983,7 @@ static void vmbus_device_release(struct device *device)
>   */
>  
>  static const struct dev_pm_ops vmbus_pm = {
> -	.suspend_noirq	= NULL,
> +	.suspend_noirq  = vmbus_freeze,
>  	.resume_noirq	= NULL,
>  	.freeze_noirq	= vmbus_suspend,
>  	.thaw_noirq	= vmbus_resume,
> -- 
> 2.34.1
> 

RE: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus

[Michael Kelley]

From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Monday, December 16, 2024 8:17 PM
> 
> This change is specific to Hyper-V VM user.
> If the Virtual Machine Connection window is focused,
> a Hyper-V VM user can unintentionally touch the keyboard/mouse
> when the VM is hibernating or resuming, and consequently the
> hibernation or resume operation can be aborted unexpectedly.
> Fix the issue by no longer registering the keyboard/mouse as
> wakeup devices (see the other two patches for the
> changes to drivers/input/serio/hyperv-keyboard.c and
> drivers/hid/hid-hyperv.c).

One question: For the keyboard and mouse patches, it looks like the
code change has the effect of changing the default value of
"power/wakeup" for the keyboard and mouse device under /sys to
be "disabled". But if a customer *wants* to enable keyboard and
mouse wakeups, he still has the option of just writing "enabled"
to "power/wakeup", and the wakeup behavior will be back like it
was before these patches. So in other words, the patches only
change the default enablement setting, and don't mess with the
ability to generate wakeups. Or does disabling the "freeze" option
for /sys/power/state mean that there's no scenario where wakeups
are useful, so the enabling vs. disabling of wakeups is moot?

As you may have noticed, I've posted a patch with documentation
about how hibernation works for Hyper-V VMs [1], and I probably
should add a paragraph about wakeups. So I want to make sure
I'm understanding correctly.

[1] https://lore.kernel.org/linux-hyperv/20241212231700.6977-1-mhklinux@outlook.com/

> 
> The keyboard/mouse were registered as wakeup devices because the
> VM needs to be woken up from the Suspend-to-Idle state after
> a user runs "echo freeze > /sys/power/state". It seems like
> the Suspend-to-Idle feature has no real users in practice, so
> let's no longer support that by returning -EOPNOTSUPP if a
> user tries to use that.
> 
> Fixes: 1a06d017fb3f ("Drivers: hv: vmbus: Fix Suspend-to-Idle for Generation-2 VM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>>
> ---
> Changes in v4:
> * No change
> 
> Changes in v3:
> * Add "Cc: stable@vger.kernel.org" in sign-off area.
> 
> Changes in v2:
> * Add "#define vmbus_freeze NULL" when CONFIG_PM_SLEEP is not
>   enabled.
> ---
>  drivers/hv/vmbus_drv.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 6d89d37b069a..4df6b12bf6a1 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -900,6 +900,19 @@ static void vmbus_shutdown(struct device *child_device)
>  }
> 
>  #ifdef CONFIG_PM_SLEEP
> +/*
> + * vmbus_freeze - Suspend-to-Idle
> + */
> +static int vmbus_freeze(struct device *child_device)
> +{
> +/*
> + * Do not support Suspend-to-Idle ("echo freeze > /sys/power/state") as
> + * that would require registering the Hyper-V synthetic mouse/keyboard
> + * devices as wakeup devices, which can abort hibernation/resume unexpectedly.
> + */
> +	return -EOPNOTSUPP;
> +}
> +
>  /*
>   * vmbus_suspend - Suspend a vmbus device
>   */
> @@ -938,6 +951,7 @@ static int vmbus_resume(struct device *child_device)
>  	return drv->resume(dev);
>  }
>  #else
> +#define vmbus_freeze NULL
>  #define vmbus_suspend NULL
>  #define vmbus_resume NULL
>  #endif /* CONFIG_PM_SLEEP */
> @@ -969,7 +983,7 @@ static void vmbus_device_release(struct device *device)
>   */
> 
>  static const struct dev_pm_ops vmbus_pm = {
> -	.suspend_noirq	= NULL,
> +	.suspend_noirq  = vmbus_freeze,
>  	.resume_noirq	= NULL,
>  	.freeze_noirq	= vmbus_suspend,
>  	.thaw_noirq	= vmbus_resume,

Immediately preceding this definition and initialization of
the vmbus_pm structure, there's a comment that talks
about supporting Suspend-To-Idle. Since the intent here is
to *not* support Suspend-To-Idle, that comment really
needs to be updated so it accurately reflects this change.

I can't help but also notice the bizarre situation with the
function names. The "freeze_noirq" function is named
"vmbus_suspend", and the "suspend_noirq" function is
named "vmbus_freeze". It certainly looks exactly backwards!

The "suspend_noirq" function should probably be named
"vmbus_suspend", but that name is already taken for use
in the hibernation path. I don't have a great suggestion on
how to fix this, other than to rename vmbus_suspend() to
vmbus_freeze(). That change wouldn't be that hard to do,
though then vmbus_freeze() ends up calling the "suspend"
function in struct hv_driver. Fixing the name in struct
hv_driver is harder because most VMBus drivers would need
to be updated. Sigh.

But it might be worth fixing the top-level function names
of both vmbus_suspend() and vmbus_resume() so they
don't look misused as the freeze/thaw and poweroff/restore
functions. And then the suspend_noirq function could be
named vmbus_suspend() so it makes sense.

Michael

Re: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus

[Erni Sri Satya Vennela]

On Wed, Dec 18, 2024 at 06:27:18AM +0000, Michael Kelley wrote:
> From: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Sent: Monday, December 16, 2024 8:17 PM
> > 
> > This change is specific to Hyper-V VM user.
> > If the Virtual Machine Connection window is focused,
> > a Hyper-V VM user can unintentionally touch the keyboard/mouse
> > when the VM is hibernating or resuming, and consequently the
> > hibernation or resume operation can be aborted unexpectedly.
> > Fix the issue by no longer registering the keyboard/mouse as
> > wakeup devices (see the other two patches for the
> > changes to drivers/input/serio/hyperv-keyboard.c and
> > drivers/hid/hid-hyperv.c).
> 
> One question: For the keyboard and mouse patches, it looks like the
> code change has the effect of changing the default value of
> "power/wakeup" for the keyboard and mouse device under /sys to
> be "disabled". But if a customer *wants* to enable keyboard and
> mouse wakeups, he still has the option of just writing "enabled"
> to "power/wakeup", and the wakeup behavior will be back like it
> was before these patches. So in other words, the patches only
> change the default enablement setting, and don't mess with the
> ability to generate wakeups. Or does disabling the "freeze" option
> for /sys/power/state mean that there's no scenario where wakeups
> are useful, so the enabling vs. disabling of wakeups is moot?
There is no scenario where wakeups are useful for keyboard and mouse. 
We wanted to remove them completely initially to support for 
hibernation. But based on Dimitry's suggestion we choose to opt for
default behaviour.
> 
> As you may have noticed, I've posted a patch with documentation
> about how hibernation works for Hyper-V VMs [1], and I probably
> should add a paragraph about wakeups. So I want to make sure
> I'm understanding correctly.
> 
> [1] https://lore.kernel.org/linux-hyperv/20241212231700.6977-1-mhklinux@outlook.com/
> 
> > 
> > The keyboard/mouse were registered as wakeup devices because the
> > VM needs to be woken up from the Suspend-to-Idle state after
> > a user runs "echo freeze > /sys/power/state". It seems like
> > the Suspend-to-Idle feature has no real users in practice, so
> > let's no longer support that by returning -EOPNOTSUPP if a
> > user tries to use that.
> > 
> > Fixes: 1a06d017fb3f ("Drivers: hv: vmbus: Fix Suspend-to-Idle for Generation-2 VM")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>>
> > ---
> > Changes in v4:
> > * No change
> > 
> > Changes in v3:
> > * Add "Cc: stable@vger.kernel.org" in sign-off area.
> > 
> > Changes in v2:
> > * Add "#define vmbus_freeze NULL" when CONFIG_PM_SLEEP is not
> >   enabled.
> > ---
> >  drivers/hv/vmbus_drv.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 6d89d37b069a..4df6b12bf6a1 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -900,6 +900,19 @@ static void vmbus_shutdown(struct device *child_device)
> >  }
> > 
> >  #ifdef CONFIG_PM_SLEEP
> > +/*
> > + * vmbus_freeze - Suspend-to-Idle
> > + */
> > +static int vmbus_freeze(struct device *child_device)
> > +{
> > +/*
> > + * Do not support Suspend-to-Idle ("echo freeze > /sys/power/state") as
> > + * that would require registering the Hyper-V synthetic mouse/keyboard
> > + * devices as wakeup devices, which can abort hibernation/resume unexpectedly.
> > + */
> > +	return -EOPNOTSUPP;
> > +}
> > +
> >  /*
> >   * vmbus_suspend - Suspend a vmbus device
> >   */
> > @@ -938,6 +951,7 @@ static int vmbus_resume(struct device *child_device)
> >  	return drv->resume(dev);
> >  }
> >  #else
> > +#define vmbus_freeze NULL
> >  #define vmbus_suspend NULL
> >  #define vmbus_resume NULL
> >  #endif /* CONFIG_PM_SLEEP */
> > @@ -969,7 +983,7 @@ static void vmbus_device_release(struct device *device)
> >   */
> > 
> >  static const struct dev_pm_ops vmbus_pm = {
> > -	.suspend_noirq	= NULL,
> > +	.suspend_noirq  = vmbus_freeze,
> >  	.resume_noirq	= NULL,
> >  	.freeze_noirq	= vmbus_suspend,
> >  	.thaw_noirq	= vmbus_resume,
> 
> Immediately preceding this definition and initialization of
> the vmbus_pm structure, there's a comment that talks
> about supporting Suspend-To-Idle. Since the intent here is
> to *not* support Suspend-To-Idle, that comment really
> needs to be updated so it accurately reflects this change.
> 
> I can't help but also notice the bizarre situation with the
> function names. The "freeze_noirq" function is named
> "vmbus_suspend", and the "suspend_noirq" function is
> named "vmbus_freeze". It certainly looks exactly backwards!
> 
> The "suspend_noirq" function should probably be named
> "vmbus_suspend", but that name is already taken for use
> in the hibernation path. I don't have a great suggestion on
> how to fix this, other than to rename vmbus_suspend() to
> vmbus_freeze(). That change wouldn't be that hard to do,
> though then vmbus_freeze() ends up calling the "suspend"
> function in struct hv_driver. Fixing the name in struct
> hv_driver is harder because most VMBus drivers would need
> to be updated. Sigh.
> 
> But it might be worth fixing the top-level function names
> of both vmbus_suspend() and vmbus_resume() so they
> don't look misused as the freeze/thaw and poweroff/restore
> functions. And then the suspend_noirq function could be
> named vmbus_suspend() so it makes sense.
> 
Thanks for the suggestion Micheal. I will work on it as a separate
patch.
> Michael

[PATCH v4 2/3] Input: hyperv-keyboard - disable as wakeup source

[Erni Sri Satya Vennela]

Do not enable keyboard as wakeup device since
Suspend-to-Idle feature is no longer supported.

Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
---
Changes in v4:
* Replace device_init_wakeup() with device_set_wakeup_capable()
  without reverting "Input: hyperv-keyboard - register as a wakeup source"

Changes in v3, v2:
* No change.
---
 drivers/input/serio/hyperv-keyboard.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index 0ee7505427ac..b32047f36321 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -355,7 +355,7 @@ static int hv_kbd_probe(struct hv_device *hv_dev,
 
 	serio_register_port(kbd_dev->hv_serio);
 
-	device_init_wakeup(&hv_dev->device, true);
+	device_set_wakeup_capable(&hv_dev->device, true);
 
 	return 0;
 
-- 
2.34.1

[PATCH v4 3/3] HID: hyperv: disable as wakeup source

[Erni Sri Satya Vennela]

Do not enable mouse as wakeup device since
Suspend-to-Idle feature is no longer supported.

Calling device_init_wakeup in the remove function
is unnecessary since the device is going away, and
thus won't be able to cause any wakeups under any
circumstances. Besides, the driver cleanup code
already handles the necessary cleanup.

Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
---
Changes in v4:
* Replace device_init_wakeup() with device_set_wakeup_capable
  without reverting "HID: hyperv: register as a wakeup source".
* Remove device_init_wakeup() in mousevsc_remove().
ref: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=f20b161f59b73d40043199b3bff362e656a9c673

Changes in v3, v2:
* No change
---
 drivers/hid/hid-hyperv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 0fb210e40a41..05215e79eac8 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -515,7 +515,7 @@ static int mousevsc_probe(struct hv_device *device,
 	if (ret)
 		goto probe_err2;
 
-	device_init_wakeup(&device->device, true);
+	device_set_wakeup_capable(&device->device, true);
 
 	input_dev->connected = true;
 	input_dev->init_complete = true;
@@ -539,7 +539,6 @@ static void mousevsc_remove(struct hv_device *dev)
 {
 	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
 
-	device_init_wakeup(&dev->device, false);
 	vmbus_close(dev->channel);
 	hid_hw_stop(input_dev->hid_device);
 	hid_destroy_device(input_dev->hid_device);
-- 
2.34.1