| Message ID | 20240103003355.747335-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: Remove SET_POWER SLEEP on system suspend | expand |
Hi, On Tue, Jan 2, 2024 at 4:34 PM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > There's a Cirque touchpad that wakes system up without anything touched > the touchpad. The input report is empty when this happens. > The reason is stated in HID over I2C spec, 7.2.8.2: > "If the DEVICE wishes to wake the HOST from its low power state, it can > issue a wake by asserting the interrupt." > > This is fine if OS can put system back to suspend by identifying input > wakeup count stays the same on resume, like Chrome OS Dark Resume [0]. > But for regular distro such policy is lacking. > > According to commit d9f448e3d71f ("HID: i2c-hid: set power sleep before > shutdown"), SLEEP is required for shutdown, in addition to that, commit > 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") didn't > notice any power comsumption reduction from SET_POWER SLEEP, so also > remove that to avoid the device asserting the interrupt. > > [0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 2735cd585af0..dd513dc75cb9 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -957,9 +957,6 @@ static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff) > if (ret < 0) > return ret; > > - /* Save some power */ > - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); IMO this is not a good idea to do universally. Specifically: 1. There are many vendors of i2c-hid devices and many different devices per vendor. Even if your device doesn't save power in "sleep" mode, that doesn't mean it's not important for some other devices. 2. There are some boards where an i2c-hid device is powered by an "always-on" regulator. On these devices it seems like a bad idea not to be able to put the i2c-hid device into sleep mode. I'd also note that I'm really not sure what ChromeOS dark resume has to do with anything here. Dark resume is used for certain types of events that wakeup the system where we can identify that the event shouldn't turn the screen on, then we do some processing, then we go back to sleep. I'm nearly certain that a trackpad / touchscreen wakeup event would never qualify for "dark resume". If we see a trackpad/touchscreen event then we'll wakeup the system. If the system is in a state where trackpad/touchscreen events shouldn't wake us up then we disable those wakeups before going to suspend... It seems to me like the board you're testing on has some strange bug and that bug should be fixed, or (in the worst case) you should send a patch to detect this broken touchpad and disable wakeup for it. -Doug
On Tue, Jan 9, 2024 at 5:25 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jan 2, 2024 at 4:34 PM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: > > > > There's a Cirque touchpad that wakes system up without anything touched > > the touchpad. The input report is empty when this happens. > > The reason is stated in HID over I2C spec, 7.2.8.2: > > "If the DEVICE wishes to wake the HOST from its low power state, it can > > issue a wake by asserting the interrupt." > > > > This is fine if OS can put system back to suspend by identifying input > > wakeup count stays the same on resume, like Chrome OS Dark Resume [0]. > > But for regular distro such policy is lacking. > > > > According to commit d9f448e3d71f ("HID: i2c-hid: set power sleep before > > shutdown"), SLEEP is required for shutdown, in addition to that, commit > > 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") didn't > > notice any power comsumption reduction from SET_POWER SLEEP, so also > > remove that to avoid the device asserting the interrupt. > > > > [0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/hid/i2c-hid/i2c-hid-core.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > > index 2735cd585af0..dd513dc75cb9 100644 > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > > @@ -957,9 +957,6 @@ static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff) > > if (ret < 0) > > return ret; > > > > - /* Save some power */ > > - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); > > IMO this is not a good idea to do universally. Specifically: > > 1. There are many vendors of i2c-hid devices and many different > devices per vendor. Even if your device doesn't save power in "sleep" > mode, that doesn't mean it's not important for some other devices. Thanks for the response. > > 2. There are some boards where an i2c-hid device is powered by an > "always-on" regulator. On these devices it seems like a bad idea not > to be able to put the i2c-hid device into sleep mode. > So if SLEEP for system wide suspend is crucial for some i2c-hid devices, it should be kept. > > I'd also note that I'm really not sure what ChromeOS dark resume has > to do with anything here. Dark resume is used for certain types of > events that wakeup the system where we can identify that the event > shouldn't turn the screen on, then we do some processing, then we go > back to sleep. I'm nearly certain that a trackpad / touchscreen wakeup > event would never qualify for "dark resume". If we see a > trackpad/touchscreen event then we'll wakeup the system. If the system > is in a state where trackpad/touchscreen events shouldn't wake us up > then we disable those wakeups before going to suspend... Doesn't Dark Resume use wakeup count to decide whether the system should wake up or go back to suspend? For this case the input report is empty, hence wakeup count remains the same after the wakeup. I assumed Dark Resume will check the wakeup count and decide to put the system back to suspend. > > It seems to me like the board you're testing on has some strange bug > and that bug should be fixed, or (in the worst case) you should send a > patch to detect this broken touchpad and disable wakeup for it. It's desired to keep the wakeup capability, disabling wakeup isn't ideal here. I'll write a patch to use touchpad specific quirk instead of applying the change universally. Kai-Heng > > > -Doug
Hi, On Tue, Jan 9, 2024 at 11:31 PM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > > I'd also note that I'm really not sure what ChromeOS dark resume has > > to do with anything here. Dark resume is used for certain types of > > events that wakeup the system where we can identify that the event > > shouldn't turn the screen on, then we do some processing, then we go > > back to sleep. I'm nearly certain that a trackpad / touchscreen wakeup > > event would never qualify for "dark resume". If we see a > > trackpad/touchscreen event then we'll wakeup the system. If the system > > is in a state where trackpad/touchscreen events shouldn't wake us up > > then we disable those wakeups before going to suspend... > > Doesn't Dark Resume use wakeup count to decide whether the system > should wake up or go back to suspend? > For this case the input report is empty, hence wakeup count remains > the same after the wakeup. I assumed Dark Resume will check the wakeup > count and decide to put the system back to suspend. Ah, I understand now. So you're saying that the issue wouldn't be so bad (or maybe we wouldn't notice it) on systems with dark resume. However, even with dark resume we're not in a super great shape. Doing a dark resume isn't exactly a lightweight operation, since it can take a bit of time to resume the system, realize that there were no wakeup events, and then go back to sleep. I'm not a total expert on dark resume, but I believe that even with dark resume, there may also be artifacts that a user might notice (like perhaps USB devices powering up or perhaps the suspend LED on the system showing that we're not in suspend anymore). > > It seems to me like the board you're testing on has some strange bug > > and that bug should be fixed, or (in the worst case) you should send a > > patch to detect this broken touchpad and disable wakeup for it. > > It's desired to keep the wakeup capability, disabling wakeup isn't ideal here. > I'll write a patch to use touchpad specific quirk instead of applying > the change universally. Thanks! I'd also be curious if this is a problem for everyone with the Cirque touchpad or if it's board-specific. I could imagine the behavior you describe as coming about due to a missing or misconfigured pull resistor on the IRQ line. ...or perhaps a pull resistor pulling up to the wrong voltage rail... -Doug
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2735cd585af0..dd513dc75cb9 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -957,9 +957,6 @@ static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff) if (ret < 0) return ret; - /* Save some power */ - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); - disable_irq(client->irq); if (force_poweroff || !device_may_wakeup(&client->dev))
There's a Cirque touchpad that wakes system up without anything touched the touchpad. The input report is empty when this happens. The reason is stated in HID over I2C spec, 7.2.8.2: "If the DEVICE wishes to wake the HOST from its low power state, it can issue a wake by asserting the interrupt." This is fine if OS can put system back to suspend by identifying input wakeup count stays the same on resume, like Chrome OS Dark Resume [0]. But for regular distro such policy is lacking. According to commit d9f448e3d71f ("HID: i2c-hid: set power sleep before shutdown"), SLEEP is required for shutdown, in addition to that, commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") didn't notice any power comsumption reduction from SET_POWER SLEEP, so also remove that to avoid the device asserting the interrupt. [0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 3 --- 1 file changed, 3 deletions(-)