linux-kernel - Re: [PATCH] HID: i2c-hid: Add 60ms delay after 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: <bbe5fde3-e0df-053e-a30a-a55c780b1f69@redhat.com>
Date:   Mon, 10 Aug 2020 18:13:06 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>, jikos@...nel.org,
        benjamin.tissoires@...hat.com
Cc:     Pavel Balan <admin@...ma.net>,
        Daniel Playfair Cal <daniel.playfair.cal@...il.com>,
        HungNien Chen <hn.chen@...dahitech.com>,
        You-Sheng Yang <vicamo.yang@...onical.com>,
        Aaron Ma <aaron.ma@...onical.com>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: i2c-hid: Add 60ms delay after SET_POWER ON

Hi,

On 10-08-2020 16:29, Kai-Heng Feng wrote:
> Goodix touchpad fails to operate in I2C mode after system suspend.
> 
> According to the vendor, Windows is more forgiving and there's a 60ms
> delay after SET_POWER ON command.
> 
> So let's do the same here, to workaround for the touchpads that depend
> on the delay.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>

Interesting I send a very similar patch a couple of days ago,
after debugging some touchpads issues on a BMAX Y13 laptop:

https://patchwork.kernel.org/patch/11701541/

If you look at that patch you will see that if we add a
sleep on power-on to i2c_hid_set_power(), we can remove
an existing sleep after power-on from i2c_hid_hwreset().

And there is an interesting comment there which should
probably be moved (as my patch does) and corrected for the
new knowledge so that people reading the code in the future
now why the sleep is there.

Other then that we've come to the same conclusion, but
your sleep is much longer. I guess that is ok though,
are you sure we need 60ms as a minimum? Is that what goodix
said?

Regards,

Hans


> ---
>   drivers/hid/i2c-hid/i2c-hid-core.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 294c84e136d7..7b24a27fad95 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -419,6 +419,9 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>   	if (ret)
>   		dev_err(&client->dev, "failed to change power setting.\n");
>   
> +	if (!ret && power_state == I2C_HID_PWR_ON)
> +		msleep(60);
> +
>   set_pwr_exit:
>   	return ret;
>   }
>