Message-ID:
 <SN6PR02MB4157123D8C5C35C2B169A1E0D4052@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Wed, 18 Dec 2024 06:27:18 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Erni Sri Satya Vennela <ernis@...ux.microsoft.com>, "kys@...rosoft.com"
	<kys@...rosoft.com>, "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
	"wei.liu@...nel.org" <wei.liu@...nel.org>, "decui@...rosoft.com"
	<decui@...rosoft.com>, "jikos@...nel.org" <jikos@...nel.org>,
	"bentiss@...nel.org" <bentiss@...nel.org>, "dmitry.torokhov@...il.com"
	<dmitry.torokhov@...il.com>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "linux-input@...r.kernel.org"
	<linux-input@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "stable@...r.kernel.org" <stable@...r.kernel.org>, Saurabh Sengar
	<ssengar@...ux.microsoft.com>
Subject: RE: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for
 VMBus

From: Erni Sri Satya Vennela <ernis@...ux.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@...r.kernel.org
> Signed-off-by: Saurabh Sengar <ssengar@...ux.microsoft.com>
> Signed-off-by: Erni Sri Satya Vennela <ernis@...ux.microsoft.com>>
> ---
> Changes in v4:
> * No change
> 
> Changes in v3:
> * Add "Cc: stable@...r.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

Powered by blists - more mailing lists