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

[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

[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