Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Dexuan Cui <decui@microsoft.com>
To: "dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>
Cc: KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"sashal@kernel.org" <sashal@kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Michael Kelley <mikelley@microsoft.com>
Subject: RE: [PATCH] Input: hyperv-keyboard: Add the support of hibernation
Date: Tue, 5 Nov 2019 05:18:24 +0000
Message-ID: <PU1P153MB01695CEE940C4511E3254ADCBF7E0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20191003174530.GB22365@dtor-ws>

> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
> Sent: Thursday, October 3, 2019 10:46 AM
> To: Dexuan Cui <decui@microsoft.com>
> On Thu, Oct 03, 2019 at 06:44:04AM +0000, Dexuan Cui wrote:
> > ...
> > I think I understood now: it looks the vmbus driver should implement
> > a prepare() or freeze(), which calls the hyperv_keyboard driver's
> > prepare() or freeze(), which can set the flag or disable the keyboard
> > event handling. This way we don't need the notifier.
> 
> Right. I think in practice the current suspend implementation can work
> as freeze() for the HV keyboard, because in suspend you shut off vmbus
> channel, so there should not be wakeup signals anymore. What you do not
> want is to have the current resume to be used in place of thaw(), as
> there you re-enable the vmbus channel and resume sending wakeup requests
> as you are writing out the hibernation image to storage.
> 
> I think if vmbus allowed HV keyboard driver to supply empty thaw() and
> poweroff() implementations, while using suspend() as freeze() and
> resume() as restore(), it would solve the issue for you.
> 
> Dmitry

Hi Dmitry,
Sorry for the late reply! I finally came back on this patch. :-)
After I dug more into the issues, this is my understanding now:

As I checked the code in drivers/ , it doesn't look commom for a driver to
distinguish between thaw() and restore(). Typically a driver uses the macro
SET_SYSTEM_SLEEP_PM_OPS() to define the dev_pm_ops, and the macro uses the
same function resume_fn as thaw() and restore().

BTW, the macro already uses the same function suspend_fn as suspend() and 
freeze(), and uses the same function resume_fn as resume() and restore(). And, 
it looks unusual for a driver to provide an empty thaw(), if any. If I follow your
suggestions, I'll have to fix the vmbus driver first (i.e. drivers/hv/vmbus_drv.c: 
vmbus_pm()) by manually assigning a new function vmbus_thaw() to the 
thaw() dev_pm_op, and vmbus_thaw() should call the Hyper-V keyboard 
driver's empty hv_kbd_thaw(), meaning I have to add a .thaw function
pointer to the struct hv_driver. IMHO all these changes look too big just for
the rare corner cases of the unexpected wake-up issues.

More important, even if we make the suggested changes, we actually only fix
the unexpected wakeup caused by PMSG_THAW , and there are still some corner
cases of failures (please see below).

Before any of the dev_pm_op is called, the global counter 'pm_abort_suspend'
can be already non-zero, meaning pm_wakeup_pending() is true, so
try_to_freeze_tasks() returns -EBUSY, i.e. hibernate() -> freeze_processes()
or hibernate() -> hibernation_snapshot() -> freeze_kernel_threads() fails.

When the VM boots up and tries to resume from the saved file from
disk, before the fresh new kernel's Hyper-V keyboard device is PMSG_QUIESCE'ed,
the global counter 'pm_abort_suspend' can be already non-zero (I can cause this
scenario by holding the Enter key when the kernel starts), so
pm_wakeup_pending() is true, and the below freeze_processes() or
device_suspend() can return -EBUSY and the resume process fails.

 software_resume() ->
    freeze_processes()
      pm_wakeup_clear(true) -> Note: this resets the counter to 0.
      try_to_freeze_tasks ->
        pm_wakeup_pending
    load_image_and_restore() ->
      hibernation_restore() ->
        dpm_suspend_start() ->
          dpm_suspend() ->
            device_suspend() ->
              __device_suspend() ->
                pm_wakeup_pending()

IMO on a Linux physical machine these issues should happen as well. I think
we can fix them separately. For this patch, I suggest we keep it simple like
the below:

[PATCH v2] Input: hyperv-keyboard: Add the support of hibernation

During the suspend process and resume process, if there is any keyboard
event, there is a small chance the suspend and the resume process can be
aborted because of hv_kbd_on_receive() -> pm_wakeup_hard_event().

This behavior can be avoided by disabling the Hyper-V keyboard device as
a wakeup source:

echo disabled > /sys/bus/vmbus/drivers/hyperv_keyboard/XXX/power/wakeup
(XXX is the device's GUID).

Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/input/serio/hyperv-keyboard.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index e486a8a74c40..df4e9f6f4529 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -259,6 +259,8 @@ static int hv_kbd_connect_to_vsp(struct hv_device *hv_dev)
        u32 proto_status;
        int error;

+       reinit_completion(&kbd_dev->wait_event);
+
        request = &kbd_dev->protocol_req;
        memset(request, 0, sizeof(struct synth_kbd_protocol_request));
        request->header.type = __cpu_to_le32(SYNTH_KBD_PROTOCOL_REQUEST);
@@ -380,6 +382,29 @@ static int hv_kbd_remove(struct hv_device *hv_dev)
        return 0;
 }

+static int hv_kbd_suspend(struct hv_device *hv_dev)
+{
+       vmbus_close(hv_dev->channel);
+
+       return 0;
+}
+
+static int hv_kbd_resume(struct hv_device *hv_dev)
+{
+       int ret;
+
+       ret = vmbus_open(hv_dev->channel,
+                        KBD_VSC_SEND_RING_BUFFER_SIZE,
+                        KBD_VSC_RECV_RING_BUFFER_SIZE,
+                        NULL, 0,
+                        hv_kbd_on_channel_callback,
+                        hv_dev);
+       if (ret == 0)
+               ret = hv_kbd_connect_to_vsp(hv_dev);
+
+       return ret;
+}
+
 static const struct hv_vmbus_device_id id_table[] = {
        /* Keyboard guid */
        { HV_KBD_GUID, },
@@ -393,6 +418,8 @@ static struct  hv_driver hv_kbd_drv = {
        .id_table = id_table,
        .probe = hv_kbd_probe,
        .remove = hv_kbd_remove,
+       .suspend = hv_kbd_suspend,
+       .resume = hv_kbd_resume,
        .driver = {
                .probe_type = PROBE_PREFER_ASYNCHRONOUS,
        },
--

I plan to post this as v2.

Looking forward to your comments!

Thanks,
-- Dexuan

  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-11 23:36 Dexuan Cui
2019-09-19 16:17 ` dmitry.torokhov
2019-09-21  6:56   ` Dexuan Cui
2019-09-25 19:49     ` Dexuan Cui
2019-09-28  0:31     ` dmitry.torokhov
2019-09-30 22:09       ` Dexuan Cui
2019-09-30 23:06         ` dmitry.torokhov
2019-10-03  5:35           ` Dexuan Cui
2019-10-03  6:44             ` Dexuan Cui
2019-10-03 17:45               ` dmitry.torokhov
2019-10-03 18:18                 ` Dexuan Cui
2019-11-05  5:18                 ` Dexuan Cui [this message]
2019-11-05  5:43                 ` Dexuan Cui

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PU1P153MB01695CEE940C4511E3254ADCBF7E0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM \
    --to=decui@microsoft.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=sashal@kernel.org \
    --cc=sthemmin@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git