| Message ID | 1425930287-4615-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Mon, Mar 09, 2015 at 12:44:47PM -0700, Doug Anderson wrote: > If the i2c-hid device was runtime suspended and then the system > suspended itself we'd end up disabling interrupts twice (in > i2c_hid_runtime_suspend and i2c_hid_suspend) and not reenabling them > until later when the i2c-hid device was runtime resumed. > Unfortunately the i2c_hid_resume() calls i2c_hid_hwreset() and that > only works properly if interrupts are enabled. > > We can fix this by taking the advice from "runtime_pm.txt". > Specifically we'll change i2c-hid to always resume to full power. > This only works if our parents are also resumed to full power, but > given the suggestion in "runtime_pm.txt" this seems a reasonable > assumption. > > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > Note that this was tested on a 3.14 kernel with backports. Any > testing that folks can do on ToT is appreciated. > > Changes in v2: > - Move pm_runtime_enable() higher in resume Hmm, I tried on one machine with Atmel touch panel. I modified i2c-hid.c a bit to print out when it is running suspend/resume hooks. Here is what I got: [ 7.506455] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime suspend This is the initialization and after that the device is runtime suspended. Now I put the machine to system sleep (using echo mem > /sys/power/state): [ 26.988867] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime resume [ 27.002957] i2c_hid i2c-ATML1000:00: PM: i2c-hid suspend So the PM core will kick the device out of runtime suspend before it suspends it again. Pressing power button will resume the device: [ 28.238934] i2c_hid i2c-ATML1000:00: PM: i2c-hid resume [ 29.941579] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime suspend and since the device is not being used, it will runtime suspend itself. In other words, I'm not sure if this patch is needed. It would be good if you could do similar testing using mainline kernel (if possible at all). -- 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
Mika, On Tue, Mar 10, 2015 at 5:23 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Mon, Mar 09, 2015 at 12:44:47PM -0700, Doug Anderson wrote: >> If the i2c-hid device was runtime suspended and then the system >> suspended itself we'd end up disabling interrupts twice (in >> i2c_hid_runtime_suspend and i2c_hid_suspend) and not reenabling them >> until later when the i2c-hid device was runtime resumed. >> Unfortunately the i2c_hid_resume() calls i2c_hid_hwreset() and that >> only works properly if interrupts are enabled. >> >> We can fix this by taking the advice from "runtime_pm.txt". >> Specifically we'll change i2c-hid to always resume to full power. >> This only works if our parents are also resumed to full power, but >> given the suggestion in "runtime_pm.txt" this seems a reasonable >> assumption. >> >> Signed-off-by: Doug Anderson <dianders@chromium.org> >> --- >> Note that this was tested on a 3.14 kernel with backports. Any >> testing that folks can do on ToT is appreciated. >> >> Changes in v2: >> - Move pm_runtime_enable() higher in resume > > Hmm, I tried on one machine with Atmel touch panel. I modified i2c-hid.c > a bit to print out when it is running suspend/resume hooks. Here is what > I got: > > [ 7.506455] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime suspend > > This is the initialization and after that the device is runtime > suspended. > > Now I put the machine to system sleep (using echo mem > > /sys/power/state): > > [ 26.988867] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime resume Thanks for testing! Can you do a "dump_stack()" here? I'm curious why it's deciding to runtime resume. Maybe something changed between 3.14 and ToT? > [ 27.002957] i2c_hid i2c-ATML1000:00: PM: i2c-hid suspend > > So the PM core will kick the device out of runtime suspend before it > suspends it again. > > Pressing power button will resume the device: > > [ 28.238934] i2c_hid i2c-ATML1000:00: PM: i2c-hid resume > [ 29.941579] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime suspend > > and since the device is not being used, it will runtime suspend itself. > > In other words, I'm not sure if this patch is needed. It would be good > if you could do similar testing using mainline kernel (if possible at > all). Unfortunately it's not so easy. For my platform suspend/resume is just barely functional as it is, but then I'm on a board that's further not supported upstream. If folks upstream can't reproduce the problems then there's no reason to take my patch... -Doug -- 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, Mar 10, 2015 at 09:12:36AM -0700, Doug Anderson wrote: > Thanks for testing! Can you do a "dump_stack()" here? I'm curious > why it's deciding to runtime resume. Maybe something changed between > 3.14 and ToT? Here you go: [ 26.711737] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime resume [ 26.711754] CPU: 3 PID: 123 Comm: sh Not tainted 4.0.0-rc3+ #6 [ 26.711775] ffff88007604c600 ffff88007ba77ae8 ffffffff8183966e 0000000080000000 [ 26.711791] ffff88017a83a020 ffff88007ba77b08 ffffffff816a0759 ffff88017a83a020 [ 26.711804] ffff88017a83a0ce ffff88007ba77b18 ffffffff814ba3ee ffff88007ba77b38 [ 26.711807] Call Trace: [ 26.711835] [<ffffffff8183966e>] dump_stack+0x4f/0x7b [ 26.711852] [<ffffffff816a0759>] i2c_hid_runtime_resume+0x29/0x50 [ 26.711866] [<ffffffff814ba3ee>] pm_generic_runtime_resume+0x2e/0x40 [ 26.711880] [<ffffffff81412b15>] acpi_subsys_runtime_resume+0x1f/0x23 [ 26.711892] [<ffffffff814bbeb6>] __rpm_callback+0x36/0x90 [ 26.711902] [<ffffffff814bbf36>] rpm_callback+0x26/0xa0 [ 26.711914] [<ffffffff814bd286>] rpm_resume+0x496/0x670 [ 26.711928] [<ffffffff814bd4a0>] __pm_runtime_resume+0x40/0x60 [ 26.711940] [<ffffffff81412500>] ? acpi_subsys_complete+0x1e/0x1e [ 26.711951] [<ffffffff81412515>] acpi_subsys_suspend+0x15/0x21 It's the ACPI power domain that runtime resumes the device before it suspends it for system sleep. [ 26.711964] [<ffffffff814be5ec>] dpm_run_callback+0x4c/0x150 [ 26.711978] [<ffffffff814bf17e>] __device_suspend+0xee/0x320 [ 26.711991] [<ffffffff814c0d88>] dpm_suspend+0x68/0x300 [ 26.712004] [<ffffffff814c14ff>] dpm_suspend_start+0x5f/0x70 [ 26.712019] [<ffffffff810936df>] suspend_devices_and_enter+0xbf/0x7d0 [ 26.712035] [<ffffffff8109e16f>] ? vprintk_default+0x1f/0x30 [ 26.712047] [<ffffffff810941a9>] pm_suspend+0x3b9/0x4e0 [ 26.712059] [<ffffffff81092850>] state_store+0x80/0x100 [ 26.712074] [<ffffffff8139f4df>] kobj_attr_store+0xf/0x20 [ 26.712087] [<ffffffff811f223a>] sysfs_kf_write+0x3a/0x50 [ 26.712098] [<ffffffff811f1717>] kernfs_fop_write+0x127/0x180 [ 26.712112] [<ffffffff81183207>] vfs_write+0xb7/0x200 [ 26.712124] [<ffffffff81183e66>] SyS_write+0x46/0xc0 [ 26.712140] [<ffffffff81844099>] ia32_do_call+0x13/0x13 -- 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
Mika, On Wed, Mar 11, 2015 at 4:20 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Mar 10, 2015 at 09:12:36AM -0700, Doug Anderson wrote: >> Thanks for testing! Can you do a "dump_stack()" here? I'm curious >> why it's deciding to runtime resume. Maybe something changed between >> 3.14 and ToT? > > Here you go: > > [ 26.711737] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime resume > [ 26.711754] CPU: 3 PID: 123 Comm: sh Not tainted 4.0.0-rc3+ #6 > [ 26.711775] ffff88007604c600 ffff88007ba77ae8 ffffffff8183966e 0000000080000000 > [ 26.711791] ffff88017a83a020 ffff88007ba77b08 ffffffff816a0759 ffff88017a83a020 > [ 26.711804] ffff88017a83a0ce ffff88007ba77b18 ffffffff814ba3ee ffff88007ba77b38 > [ 26.711807] Call Trace: > [ 26.711835] [<ffffffff8183966e>] dump_stack+0x4f/0x7b > [ 26.711852] [<ffffffff816a0759>] i2c_hid_runtime_resume+0x29/0x50 > [ 26.711866] [<ffffffff814ba3ee>] pm_generic_runtime_resume+0x2e/0x40 > [ 26.711880] [<ffffffff81412b15>] acpi_subsys_runtime_resume+0x1f/0x23 > [ 26.711892] [<ffffffff814bbeb6>] __rpm_callback+0x36/0x90 > [ 26.711902] [<ffffffff814bbf36>] rpm_callback+0x26/0xa0 > [ 26.711914] [<ffffffff814bd286>] rpm_resume+0x496/0x670 > [ 26.711928] [<ffffffff814bd4a0>] __pm_runtime_resume+0x40/0x60 > [ 26.711940] [<ffffffff81412500>] ? acpi_subsys_complete+0x1e/0x1e > [ 26.711951] [<ffffffff81412515>] acpi_subsys_suspend+0x15/0x21 > > It's the ACPI power domain that runtime resumes the device before it > suspends it for system sleep. OK, that explains the difference in behavior for me. I'm on an ARM board that has no ACPI, so there's no ACPI layer to runtime resume the device. At least it sounds like you confirmed that my patch doesn't break your use case, which is good. If folks want to wait until I can reproduce this problem on ToT Linux before merging then I guess we can let this patch float until I can manage to get enough other bits of the system happy with mainline. Thanks for your tests! :) -Doug -- 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 Wed, Mar 11, 2015 at 08:06:47AM -0700, Doug Anderson wrote: > Mika, > > On Wed, Mar 11, 2015 at 4:20 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Tue, Mar 10, 2015 at 09:12:36AM -0700, Doug Anderson wrote: > >> Thanks for testing! Can you do a "dump_stack()" here? I'm curious > >> why it's deciding to runtime resume. Maybe something changed between > >> 3.14 and ToT? > > > > Here you go: > > > > [ 26.711737] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime resume > > [ 26.711754] CPU: 3 PID: 123 Comm: sh Not tainted 4.0.0-rc3+ #6 > > [ 26.711775] ffff88007604c600 ffff88007ba77ae8 ffffffff8183966e 0000000080000000 > > [ 26.711791] ffff88017a83a020 ffff88007ba77b08 ffffffff816a0759 ffff88017a83a020 > > [ 26.711804] ffff88017a83a0ce ffff88007ba77b18 ffffffff814ba3ee ffff88007ba77b38 > > [ 26.711807] Call Trace: > > [ 26.711835] [<ffffffff8183966e>] dump_stack+0x4f/0x7b > > [ 26.711852] [<ffffffff816a0759>] i2c_hid_runtime_resume+0x29/0x50 > > [ 26.711866] [<ffffffff814ba3ee>] pm_generic_runtime_resume+0x2e/0x40 > > [ 26.711880] [<ffffffff81412b15>] acpi_subsys_runtime_resume+0x1f/0x23 > > [ 26.711892] [<ffffffff814bbeb6>] __rpm_callback+0x36/0x90 > > [ 26.711902] [<ffffffff814bbf36>] rpm_callback+0x26/0xa0 > > [ 26.711914] [<ffffffff814bd286>] rpm_resume+0x496/0x670 > > [ 26.711928] [<ffffffff814bd4a0>] __pm_runtime_resume+0x40/0x60 > > [ 26.711940] [<ffffffff81412500>] ? acpi_subsys_complete+0x1e/0x1e > > [ 26.711951] [<ffffffff81412515>] acpi_subsys_suspend+0x15/0x21 > > > > It's the ACPI power domain that runtime resumes the device before it > > suspends it for system sleep. > > OK, that explains the difference in behavior for me. I'm on an ARM > board that has no ACPI, so there's no ACPI layer to runtime resume the > device. That's interesting. So you have an ARM device with i2c-hid compatible device connected to it? Out of curiousity, do you use DT or some platform board file to configure the thing? > At least it sounds like you confirmed that my patch doesn't break your > use case, which is good. I actually didn't have your patch applied yet. I just wanted to verify that the existing code works. Let me know and I'll run the same test with your patch applied. -- 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 Thu, Mar 12, 2015 at 11:10:14AM +0200, Mika Westerberg wrote: > On Wed, Mar 11, 2015 at 08:06:47AM -0700, Doug Anderson wrote: > > Mika, > > > > On Wed, Mar 11, 2015 at 4:20 AM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Tue, Mar 10, 2015 at 09:12:36AM -0700, Doug Anderson wrote: > > >> Thanks for testing! Can you do a "dump_stack()" here? I'm curious > > >> why it's deciding to runtime resume. Maybe something changed between > > >> 3.14 and ToT? > > > > > > Here you go: > > > > > > [ 26.711737] i2c_hid i2c-ATML1000:00: PM: i2c-hid runtime resume > > > [ 26.711754] CPU: 3 PID: 123 Comm: sh Not tainted 4.0.0-rc3+ #6 > > > [ 26.711775] ffff88007604c600 ffff88007ba77ae8 ffffffff8183966e 0000000080000000 > > > [ 26.711791] ffff88017a83a020 ffff88007ba77b08 ffffffff816a0759 ffff88017a83a020 > > > [ 26.711804] ffff88017a83a0ce ffff88007ba77b18 ffffffff814ba3ee ffff88007ba77b38 > > > [ 26.711807] Call Trace: > > > [ 26.711835] [<ffffffff8183966e>] dump_stack+0x4f/0x7b > > > [ 26.711852] [<ffffffff816a0759>] i2c_hid_runtime_resume+0x29/0x50 > > > [ 26.711866] [<ffffffff814ba3ee>] pm_generic_runtime_resume+0x2e/0x40 > > > [ 26.711880] [<ffffffff81412b15>] acpi_subsys_runtime_resume+0x1f/0x23 > > > [ 26.711892] [<ffffffff814bbeb6>] __rpm_callback+0x36/0x90 > > > [ 26.711902] [<ffffffff814bbf36>] rpm_callback+0x26/0xa0 > > > [ 26.711914] [<ffffffff814bd286>] rpm_resume+0x496/0x670 > > > [ 26.711928] [<ffffffff814bd4a0>] __pm_runtime_resume+0x40/0x60 > > > [ 26.711940] [<ffffffff81412500>] ? acpi_subsys_complete+0x1e/0x1e > > > [ 26.711951] [<ffffffff81412515>] acpi_subsys_suspend+0x15/0x21 > > > > > > It's the ACPI power domain that runtime resumes the device before it > > > suspends it for system sleep. > > > > OK, that explains the difference in behavior for me. I'm on an ARM > > board that has no ACPI, so there's no ACPI layer to runtime resume the > > device. > > That's interesting. So you have an ARM device with i2c-hid compatible > device connected to it? Out of curiousity, do you use DT or some > platform board file to configure the thing? > > > At least it sounds like you confirmed that my patch doesn't break your > > use case, which is good. > > I actually didn't have your patch applied yet. I just wanted to verify > that the existing code works. > > Let me know and I'll run the same test with your patch applied. I did try with the patch applied and suspend/resume worked just fine. -- 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
Mika, On Thu, Mar 12, 2015 at 2:10 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > That's interesting. So you have an ARM device with i2c-hid compatible > device connected to it? Out of curiousity, do you use DT or some > platform board file to configure the thing? Using DT. That's allowed, right? (3d7d248 HID: i2c-hid: add DT bindings) -Doug -- 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 Thu, Mar 12, 2015 at 08:24:53AM -0700, Doug Anderson wrote: > Mika, > > On Thu, Mar 12, 2015 at 2:10 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > That's interesting. So you have an ARM device with i2c-hid compatible > > device connected to it? Out of curiousity, do you use DT or some > > platform board file to configure the thing? > > Using DT. That's allowed, right? (3d7d248 HID: i2c-hid: add DT bindings) Of course. I was just curious because I have only seen i2c-hid devices that are ACPI enumerated. -- 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
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index ab4dd95..9e013f6 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -1090,15 +1090,18 @@ static int i2c_hid_suspend(struct device *dev) struct hid_device *hid = ihid->hid; int ret = 0; - disable_irq(ihid->irq); - if (device_may_wakeup(&client->dev)) - enable_irq_wake(ihid->irq); + if (!pm_runtime_suspended(dev)) { + /* Save some power */ + i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); + + disable_irq(ihid->irq); + } if (hid->driver && hid->driver->suspend) ret = hid->driver->suspend(hid, PMSG_SUSPEND); - /* Save some power */ - i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); + if (device_may_wakeup(&client->dev)) + enable_irq_wake(ihid->irq); return ret; } @@ -1110,6 +1113,11 @@ static int i2c_hid_resume(struct device *dev) struct i2c_hid *ihid = i2c_get_clientdata(client); struct hid_device *hid = ihid->hid; + /* 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)
If the i2c-hid device was runtime suspended and then the system suspended itself we'd end up disabling interrupts twice (in i2c_hid_runtime_suspend and i2c_hid_suspend) and not reenabling them until later when the i2c-hid device was runtime resumed. Unfortunately the i2c_hid_resume() calls i2c_hid_hwreset() and that only works properly if interrupts are enabled. We can fix this by taking the advice from "runtime_pm.txt". Specifically we'll change i2c-hid to always resume to full power. This only works if our parents are also resumed to full power, but given the suggestion in "runtime_pm.txt" this seems a reasonable assumption. Signed-off-by: Doug Anderson <dianders@chromium.org> --- Note that this was tested on a 3.14 kernel with backports. Any testing that folks can do on ToT is appreciated. Changes in v2: - Move pm_runtime_enable() higher in resume drivers/hid/i2c-hid/i2c-hid.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)