* [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus [not found] <1734409029-10419-1-git-send-email-ernis@linux.microsoft.com> @ 2024-12-17 4:17 ` Erni Sri Satya Vennela 2024-12-17 19:27 ` Wei Liu 2024-12-18 6:27 ` Michael Kelley 0 siblings, 2 replies; 5+ messages in thread From: Erni Sri Satya Vennela @ 2024-12-17 4:17 UTC (permalink / raw) To: kys, haiyangz, wei.liu, decui, jikos, bentiss, dmitry.torokhov, linux-hyperv, linux-input, linux-kernel Cc: Erni Sri Satya Vennela, stable, Saurabh Sengar 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus 2024-12-17 4:17 ` [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus Erni Sri Satya Vennela @ 2024-12-17 19:27 ` Wei Liu 2024-12-18 6:27 ` Michael Kelley 1 sibling, 0 replies; 5+ messages in thread From: Wei Liu @ 2024-12-17 19:27 UTC (permalink / raw) To: Erni Sri Satya Vennela Cc: kys, haiyangz, wei.liu, decui, jikos, bentiss, dmitry.torokhov, linux-hyperv, linux-input, linux-kernel, stable, Saurabh Sengar 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus 2024-12-17 4:17 ` [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus Erni Sri Satya Vennela 2024-12-17 19:27 ` Wei Liu @ 2024-12-18 6:27 ` Michael Kelley 2025-01-24 16:54 ` Erni Sri Satya Vennela 1 sibling, 1 reply; 5+ messages in thread From: Michael Kelley @ 2024-12-18 6:27 UTC (permalink / raw) To: Erni Sri Satya Vennela, kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com, jikos@kernel.org, bentiss@kernel.org, dmitry.torokhov@gmail.com, linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org, Saurabh Sengar 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus 2024-12-18 6:27 ` Michael Kelley @ 2025-01-24 16:54 ` Erni Sri Satya Vennela 0 siblings, 0 replies; 5+ messages in thread From: Erni Sri Satya Vennela @ 2025-01-24 16:54 UTC (permalink / raw) To: Michael Kelley Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com, jikos@kernel.org, bentiss@kernel.org, dmitry.torokhov@gmail.com, linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Saurabh Sengar 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <1734338161-12466-1-git-send-email-ernis@linux.microsoft.com>]
* [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus [not found] <1734338161-12466-1-git-send-email-ernis@linux.microsoft.com> @ 2024-12-16 8:35 ` Erni Sri Satya Vennela 0 siblings, 0 replies; 5+ messages in thread From: Erni Sri Satya Vennela @ 2024-12-16 8:35 UTC (permalink / raw) To: ernis; +Cc: Erni Sri Satya Vennela, stable, Saurabh Sengar 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-24 16:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1734409029-10419-1-git-send-email-ernis@linux.microsoft.com>
2024-12-17 4:17 ` [PATCH v4 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus Erni Sri Satya Vennela
2024-12-17 19:27 ` Wei Liu
2024-12-18 6:27 ` Michael Kelley
2025-01-24 16:54 ` Erni Sri Satya Vennela
[not found] <1734338161-12466-1-git-send-email-ernis@linux.microsoft.com>
2024-12-16 8:35 ` Erni Sri Satya Vennela
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox