HID: i2c-hid: Fix suspend/resume when already runtime suspended
PATCH v3 1/1 HID: i2c-hid: Fix suspend/resume when already runtime suspended
On ACPI-based systems ACPI power domain code runtime resumes device before calling suspend method, which ensures that i2c-hid suspend code starts with device not in low-power state and with interrupts enabled.
On other systems, especially if device is not a part of any power domain, we may end up calling driver's system-level suspend routine while the device is runtime-suspended (with controller in presumably low power state and interrupts disabled). This will result in interrupts being essentially disabled twice, and we will only re-enable them after both system resume and runtime resume methods complete. Unfortunately i2c_hid_resume() calls i2c_hid_hwreset() and that only works properly if interrupts are enabled.
Also if device is runtime-suspended driver's suspend code may fail if it tries to issue I/O requests.
Let's fix it by runtime-resuming the device if we need to run HID driver's suspend code and also disabling interrupts only if device is not already runtime-suspended. Also on resume we mark the device as running at full power (since that is what resetting will do to it).
---
This is an uprev of a patch that Doug sent a year ago (see
https://patchwork.kernel.org/patch/5970731/), adjusted to the latest mainline. The change from v2 is that we runtime-resume the device before calling into HID driver's suspend callback.
drivers/hid/i2c-hid/i2c-hid.c | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-)
drivers/hid/i2c-hid/i2c-hid.c +31 -12
@@ -1108,13 +1108,30 @@ static int i2c_hid_suspend(struct device *dev)struct i2c_client *client =to_i2c_client (dev);struct i2c_hid *ihid =i2c_get_clientdata (client);struct hid_device *hid = ihid->hid; -int ret =0 ; +int ret;int wake_status; -if (hid->driver && hid->driver->suspend) +if (hid->driver && hid->driver->suspend) { +/* +* Wake up the device so that IO issues in +* HID driver's suspend code can succeed. +*/ + ret =pm_runtime_resume (dev); +if (ret <0 ) +return ret; + ret = hid->driver->suspend (hid,PMSG_SUSPEND ); +if (ret <0 ) +return ret; + } + +if (!pm_runtime_suspended (dev)) { +/* Save some power */ +i2c_hid_set_power (client,I2C_HID_PWR_SLEEP ); + +disable_irq (ihid->irq); + } -disable_irq (ihid->irq);if (device_may_wakeup (&client->dev)) { wake_status =enable_irq_wake (ihid->irq);if (!wake_status)
@@ -1124,10 +1141,7 @@ static int i2c_hid_suspend(struct device *dev) wake_status); } -/* Save some power */ -i2c_hid_set_power (client,I2C_HID_PWR_SLEEP ); - -return ret; +return 0 ; }static int i2c_hid_resume (struct device *dev)
@@ -1138,11 +1152,6 @@ static int i2c_hid_resume(struct device *dev)struct hid_device *hid = ihid->hid;int wake_status; -enable_irq (ihid->irq); - ret =i2c_hid_hwreset (client); -if (ret) -return ret; -if (device_may_wakeup (&client->dev) && ihid->irq_wake_enabled) { wake_status =disable_irq_wake (ihid->irq);if (!wake_status)
@@ -1152,6 +1161,16 @@ static int i2c_hid_resume(struct device *dev) wake_status); } +/* We'll resume to full power */ +pm_runtime_disable (dev); +pm_runtime_set_active (dev); +pm_runtime_enable (dev); + +enable_irq (ihid->irq); + ret =i2c_hid_hwreset (client); +if (ret) +return ret; +if (hid->driver && hid->driver->reset_resume) { ret = hid->driver->reset_resume (hid);return ret;
Signature · 5 lines
2.7.0.rc3.207.g0ac5344 -- Dmitry
Quoted 15 lines · Dmitry Torokhov
On ACPI-based systems ACPI power domain code runtime resumes device before calling suspend method, which ensures that i2c-hid suspend code starts with device not in low-power state and with interrupts enabled.
On other systems, especially if device is not a part of any power domain, we may end up calling driver's system-level suspend routine while the device is runtime-suspended (with controller in presumably low power state and interrupts disabled). This will result in interrupts being essentially disabled twice, and we will only re-enable them after both system resume and runtime resume methods complete. Unfortunately
i2c_hid_resume()callsi2c_hid_hwreset()and that only works properly if interrupts are enabled.Also if device is runtime-suspended driver's suspend code may fail if it tries to issue I/O requests.
Quoted 7 lines
Let's fix it by runtime-resuming the device if we need to run HID driver's suspend code and also disabling interrupts only if device is not already runtime-suspended. Also on resume we mark the device as running at full power (since that is what resetting will do to it).
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>
Quoted 6 lines
---
This is an uprev of a patch that Doug sent a year ago (see
https://patchwork.kernel.org/patch/5970731/), adjusted to the latest mainline. The change from v2 is that we runtime-resume the device before calling into HID driver's suspend callback.
Signature · 3 lines
Benson Leung Senior Software Engineer, Chrom* OS [email protected]
Quoted 15 lines · Dmitry Torokhov
On ACPI-based systems ACPI power domain code runtime resumes device before calling suspend method, which ensures that i2c-hid suspend code starts with device not in low-power state and with interrupts enabled.
On other systems, especially if device is not a part of any power domain, we may end up calling driver's system-level suspend routine while the device is runtime-suspended (with controller in presumably low power state and interrupts disabled). This will result in interrupts being essentially disabled twice, and we will only re-enable them after both system resume and runtime resume methods complete. Unfortunately
i2c_hid_resume()callsi2c_hid_hwreset()and that only works properly if interrupts are enabled.Also if device is runtime-suspended driver's suspend code may fail if it tries to issue I/O requests.
Quoted 7 lines
Let's fix it by runtime-resuming the device if we need to run HID driver's suspend code and also disabling interrupts only if device is not already runtime-suspended. Also on resume we mark the device as running at full power (since that is what resetting will do to it).
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>
Tested on a couple of Intel Skylake and Baytrail based systems and power management of I2C connected HID devices still seems to work just fine.
Benjamin Tissoires
Acked-byQuoted 21 lines · Mika Westerberg
Quoted 20 lines · Dmitry Torokhov
On ACPI-based systems ACPI power domain code runtime resumes device before calling suspend method, which ensures that i2c-hid suspend code starts with device not in low-power state and with interrupts enabled.
On other systems, especially if device is not a part of any power domain, we may end up calling driver's system-level suspend routine while the device is runtime-suspended (with controller in presumably low power state and interrupts disabled). This will result in interrupts being essentially disabled twice, and we will only re-enable them after both system resume and runtime resume methods complete. Unfortunately
i2c_hid_resume()callsi2c_hid_hwreset()and that only works properly if interrupts are enabled.Also if device is runtime-suspended driver's suspend code may fail if it tries to issue I/O requests.
Let's fix it by runtime-resuming the device if we need to run HID driver's suspend code and also disabling interrupts only if device is not already runtime-suspended. Also on resume we mark the device as running at full power (since that is what resetting will do to it).
Quoted 7 lines
Quoted 2 lines
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>Tested on a couple of Intel Skylake and Baytrail based systems and power management of I2C connected HID devices still seems to work just fine.
Tested-by Mika Westerberg <[email protected]>
Well, I have completely no way of testing this myself, and I blindly trust Mika, Dmitry and the others for doing the right thing :).
Cheers,
Benjamin
Quoted 15 lines · Dmitry Torokhov
On ACPI-based systems ACPI power domain code runtime resumes device before calling suspend method, which ensures that i2c-hid suspend code starts with device not in low-power state and with interrupts enabled.
On other systems, especially if device is not a part of any power domain, we may end up calling driver's system-level suspend routine while the device is runtime-suspended (with controller in presumably low power state and interrupts disabled). This will result in interrupts being essentially disabled twice, and we will only re-enable them after both system resume and runtime resume methods complete. Unfortunately
i2c_hid_resume()callsi2c_hid_hwreset()and that only works properly if interrupts are enabled.Also if device is runtime-suspended driver's suspend code may fail if it tries to issue I/O requests.
Quoted 7 lines
Let's fix it by runtime-resuming the device if we need to run HID driver's suspend code and also disabling interrupts only if device is not already runtime-suspended. Also on resume we mark the device as running at full power (since that is what resetting will do to it).
Signed-off-by Doug Anderson <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>
Applied to for-4.6/i2c-hid, thanks.
Quoted 2 lines · Benjamin Tissoires
Well, I have completely no way of testing this myself, and I blindly trust Mika, Dmitry and the others for doing the right thing :).
Welcome to the wonderful world of linux kernel maintainers! :p
Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs