| Message ID | 20160308230323.GA4382@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 8, 2016 at 3:03 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > 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). > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Dmitry Torokhov <dtor@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> > --- > > 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.
On Tue, Mar 08, 2016 at 03:03:23PM -0800, Dmitry Torokhov wrote: > 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). > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Dmitry Torokhov <dtor@chromium.org> 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 <mika.westerberg@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 09 2016 or thereabouts, Mika Westerberg wrote: > On Tue, Mar 08, 2016 at 03:03:23PM -0800, Dmitry Torokhov wrote: > > 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). > > > > Signed-off-by: Doug Anderson <dianders@chromium.org> > > Signed-off-by: Dmitry Torokhov <dtor@chromium.org> > > 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 <mika.westerberg@linux.intel.com> Well, I have completely no way of testing this myself, and I blindly trust Mika, Dmitry and the others for doing the right thing :). Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 8 Mar 2016, Dmitry Torokhov wrote: > 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). > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Dmitry Torokhov <dtor@chromium.org> Applied to for-4.6/i2c-hid, thanks. On Wed, 9 Mar 2016, Benjamin Tissoires wrote: > 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,
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index b921693..1f62751 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -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;