linux-kernel - Re: [PATCH] HID: i2c-hid: Remove SET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=VmGNB5dP5WO7=txNDScNfhCDEsfFFivXqz+PH6rt=x8g@mail.gmail.com>
Date: Mon, 8 Jan 2024 13:24:46 -0800
From: Doug Anderson <dianders@...omium.org>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc: jikos@...nel.org, benjamin.tissoires@...hat.com, 
	Maxime Ripard <mripard@...nel.org>, Thomas Weißschuh <linux@...ssschuh.net>, 
	Johan Hovold <johan+linaro@...nel.org>, Dmitry Torokhov <dmitry.torokhov@...il.com>, 
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: i2c-hid: Remove SET_POWER SLEEP on system suspend

Hi,

On Tue, Jan 2, 2024 at 4:34 PM Kai-Heng Feng
<kai.heng.feng@...onical.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@...onical.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