linux-kernel - Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

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: <CAO-hwJ+OQAsJkJj2YRGOE=8NcXhS=XWwdfGSaLjY9Bdvcfn7BQ@mail.gmail.com>
Date:   Thu, 17 Jan 2019 13:35:32 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>,
        Hans de Goede <hdegoede@...hat.com>, anisse@...ier.eu
Cc:     Jiri Kosina <jikos@...nel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

On Thu, Jan 17, 2019 at 12:41 PM Kai-Heng Feng
<kai.heng.feng@...onical.com> wrote:
>
>
>
> > On Jan 17, 2019, at 16:06, Benjamin Tissoires <benjamin.tissoires@...hat.com> wrote:
> >
> > On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
> > <kai.heng.feng@...onical.com> wrote:
> [snipped]
> >> Goodix says the firmware needs at least 60ms to fully respond ON and
> >> SLEEP command.
> >
> > I was about to say that this is not conformant to the specification.
> > But looking at 7.2.8 SET_POWER of
> > https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
> >
> > They say:
> > "The DEVICE must ensure that it transitions to the HOST specified
> > Power State in under 1 second. "
> > But in the RESPONSE paragraph: "The DEVICE shall not respond back
> > after receiving the command. The DEVICE is mandated to enter that
> > power state imminently."
> >
> > My interpretation was always that the device has to be in a ready
> > state for new commands "immediately".
> > However, the first sentence may suggest that the driver would block
> > any command to the device before the 1 second timestamp.
> >
> >>
> >> I’ll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
> >> touchpanels.
> >
> > We already have a I2C_HID_QUIRK_DELAY_AFTER_SLEEP quirk that delay
> > operations after sleep by 20ms. Maybe we can keep the runtime PM but
> > use the same timers to not confuse the hardware.
>
> Yes, but wouldn’t use pm_*_autosuspend() helpers can both solve the issue
> and make the code more cleaner?

You are probably correct :)

I am not too familiar with the details of the runtime PM API, but if
you can make this a barrier to prevent the host to call too many
suspends/resumes in a row, that would be nice.
And we might be able to ditch I2C_HID_QUIRK_DELAY_AFTER_SLEEP and
I2C_HID_QUIRK_NO_RUNTIME_PM.

Adding the involved people in the thread.

Cheers,
Benjamin