From: Erni Sri Satya Vennela <ernis@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx/ > > 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@xxxxxxxxxxxxxxx > Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Erni Sri Satya Vennela <ernis@xxxxxxxxxxxxxxxxxxx>> > --- > Changes in v4: > * No change > > Changes in v3: > * Add "Cc: stable@xxxxxxxxxxxxxxx" 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
