| Message ID | 1726176470-13133-2-git-send-email-ernis@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Disable Suspend-to-Idle in Hyper-V and Fix Hibernation Interruptions | expand |
On Thu, Sep 12, 2024 at 02:27:48PM -0700, Erni Sri Satya Vennela wrote: > 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. > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> Can we have a fixes tag ? My vote is to backport this upto this commit atleast: https://lore.kernel.org/all/1586663435-36243-1-git-send-email-decui@microsoft.com/ - Saurabh > --- > drivers/hv/vmbus_drv.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 965d2a4efb7e..4efd8856392f 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 > */ > @@ -969,7 +982,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
On 9/13/2024 2:57 AM, Erni Sri Satya Vennela wrote: > 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. > Maybe it would be good to capture here the kind of VMs that this is going to be not supported - HyperV based VMs. You mentioned it in cover letter, but it would be good to add it here as well, as cover letter does not go to git log. Also, the subject suggests that we are disabling suspend-to-idle for vmbus specifically, but from commit description, it suggests that "suspend to idle" feature itself is no longer supported on these particular VMs. Please rephrase it based on what exactly we are trying to do here. IIUC, we are now returning an error (EOPNOTSUPP) in vmbus driver callback, which insures that we don't support Suspend-to-Idle in these VMs. > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> > --- > drivers/hv/vmbus_drv.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 965d2a4efb7e..4efd8856392f 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 > */ > @@ -969,7 +982,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, I am not sure if this is OK or how it works, but this naming scheme seems a bit confusing to me - *suspend* -> vmbus_*freeze* *freeze* -> vmbus_*suspend* and we are removing support for "freeze" by returning EOPNOTSUPP in suspend callback. I'll try to understand more on this, but just see if its OK. > .thaw_noirq = vmbus_resume, Regards, Naman
Hi Erni, kernel test robot noticed the following build errors: [auto build test ERROR on hid/for-next] [also build test ERROR on dtor-input/next dtor-input/for-linus linus/master v6.11-rc7 next-20240913] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Erni-Sri-Satya-Vennela/Drivers-hv-vmbus-Disable-Suspend-to-Idle-for-VMBus/20240913-053127 base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next patch link: https://lore.kernel.org/r/1726176470-13133-2-git-send-email-ernis%40linux.microsoft.com patch subject: [PATCH 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus config: x86_64-randconfig-161-20240913 (https://download.01.org/0day-ci/archive/20240914/202409140042.imFE8dSL-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140042.imFE8dSL-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409140042.imFE8dSL-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/hv/vmbus_drv.c:985:20: error: use of undeclared identifier 'vmbus_freeze' 985 | .suspend_noirq = vmbus_freeze, | ^ drivers/hv/vmbus_drv.c:1913:42: warning: shift count >= width of type [-Wshift-count-overflow] 1913 | dma_set_mask(&child_device_obj->device, DMA_BIT_MASK(64)); | ^~~~~~~~~~~~~~~~ include/linux/dma-mapping.h:77:54: note: expanded from macro 'DMA_BIT_MASK' 77 | #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1)) | ^ ~~~ 1 warning and 1 error generated. vim +/vmbus_freeze +985 drivers/hv/vmbus_drv.c 973 974 /* 975 * Note: we must use the "noirq" ops: see the comment before vmbus_bus_pm. 976 * 977 * suspend_noirq/resume_noirq are set to NULL to support Suspend-to-Idle: we 978 * shouldn't suspend the vmbus devices upon Suspend-to-Idle, otherwise there 979 * is no way to wake up a Generation-2 VM. 980 * 981 * The other 4 ops are for hibernation. 982 */ 983 984 static const struct dev_pm_ops vmbus_pm = { > 985 .suspend_noirq = vmbus_freeze, 986 .resume_noirq = NULL, 987 .freeze_noirq = vmbus_suspend, 988 .thaw_noirq = vmbus_resume, 989 .poweroff_noirq = vmbus_suspend, 990 .restore_noirq = vmbus_resume, 991 }; 992
Hi Erni, kernel test robot noticed the following build errors: [auto build test ERROR on hid/for-next] [also build test ERROR on dtor-input/next dtor-input/for-linus linus/master v6.11-rc7 next-20240913] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Erni-Sri-Satya-Vennela/Drivers-hv-vmbus-Disable-Suspend-to-Idle-for-VMBus/20240913-053127 base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next patch link: https://lore.kernel.org/r/1726176470-13133-2-git-send-email-ernis%40linux.microsoft.com patch subject: [PATCH 1/3] Drivers: hv: vmbus: Disable Suspend-to-Idle for VMBus config: i386-randconfig-003-20240913 (https://download.01.org/0day-ci/archive/20240914/202409140137.5jZxplAp-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140137.5jZxplAp-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409140137.5jZxplAp-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/hv/vmbus_drv.c:985:27: error: 'vmbus_freeze' undeclared here (not in a function); did you mean 'vmbus_remove'? 985 | .suspend_noirq = vmbus_freeze, | ^~~~~~~~~~~~ | vmbus_remove vim +985 drivers/hv/vmbus_drv.c 973 974 /* 975 * Note: we must use the "noirq" ops: see the comment before vmbus_bus_pm. 976 * 977 * suspend_noirq/resume_noirq are set to NULL to support Suspend-to-Idle: we 978 * shouldn't suspend the vmbus devices upon Suspend-to-Idle, otherwise there 979 * is no way to wake up a Generation-2 VM. 980 * 981 * The other 4 ops are for hibernation. 982 */ 983 984 static const struct dev_pm_ops vmbus_pm = { > 985 .suspend_noirq = vmbus_freeze, 986 .resume_noirq = NULL, 987 .freeze_noirq = vmbus_suspend, 988 .thaw_noirq = vmbus_resume, 989 .poweroff_noirq = vmbus_suspend, 990 .restore_noirq = vmbus_resume, 991 }; 992
On Fri, Sep 13, 2024 at 12:49:27PM +0530, Naman Jain wrote: > > > On 9/13/2024 2:57 AM, Erni Sri Satya Vennela wrote: > >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. > > > > Maybe it would be good to capture here the kind of VMs that this is > going to be not supported - HyperV based VMs. You mentioned it in cover > letter, but it would be good to add it here as well, as cover letter > does not go to git log. > Sure, I'll specify this in the next version of the patch. > Also, the subject suggests that we are disabling suspend-to-idle for > vmbus specifically, but from commit description, it suggests that > "suspend to idle" feature itself is no longer supported on these > particular VMs. Please rephrase it based on what exactly we are trying > to do here. IIUC, we are now returning an error (EOPNOTSUPP) in vmbus > driver callback, which insures that we don't support Suspend-to-Idle in > these VMs. > Yes, that's correct. > >Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > >Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> > >--- > > drivers/hv/vmbus_drv.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > >index 965d2a4efb7e..4efd8856392f 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 > > */ > >@@ -969,7 +982,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, > > I am not sure if this is OK or how it works, but this naming scheme > seems a bit confusing to me - > *suspend* -> vmbus_*freeze* > *freeze* -> vmbus_*suspend* > and we are removing support for "freeze" by returning EOPNOTSUPP in > suspend callback. AFAIU, suspend_noirq is used for Suspend-to-Idle operation and we use "freeze > /sys/power/state" for the same. Maybe the naming convention comes that way. The key point to understand here is suspend_noirq prepares the machine to low power state and freeze_noirq prepares the machine for hibernation (saving state to disk). > > I'll try to understand more on this, but just see if its OK. > > > .thaw_noirq = vmbus_resume, > > Regards, > Naman Yes, thaw_noirq is to restore devices from hibernation state.
[+linux-pm, Rafael, Len, Pavel] On Thu, Sep 12, 2024 at 02:27:48PM -0700, Erni Sri Satya Vennela wrote: > 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. > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com> > --- > drivers/hv/vmbus_drv.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 965d2a4efb7e..4efd8856392f 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 > */ > @@ -969,7 +982,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 > >
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index 965d2a4efb7e..4efd8856392f 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 */ @@ -969,7 +982,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,