linux-kernel - Re: [PATCH] HID: i2c-hid: Don't reset device upon system resume

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>] [day] [month] [year] [list]

Message-Id: <F02B6A3A-2584-452A-9A3F-BEF9DB0083E7@canonical.com>
Date:   Mon, 3 Sep 2018 23:54:35 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        AceLan Kao <acelan.kao@...onical.com>,
        Aaron Ma <aaron.ma@...onical.com>, linux-input@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: i2c-hid: Don't reset device upon system resume

Hi Benjamin,

I forgot to include mailing list, first time -in-reply-to user :)

at 17:39, Benjamin Tissoires <benjamin.tissoires@...hat.com> wrote:

> On Mon, Sep 3, 2018 at 11:09 AM Kai-Heng Feng
> <kai.heng.feng@...onical.com> wrote:
>> Raydium touchscreen triggers interrupt storm after system-wide suspend:
>> [ 179.085033] i2c_hid i2c-CUST0000:00: i2c_hid_get_input: incomplete
>> report (58/65535)
>>
>> According to Raydium, Windows driver does not reset the device after
>> system resume.
>>
>> The HID over I2C spec does specify a reset should be used at
>> intialization, but it doesn't specify if reset is required for system
>> suspend.
>
> Hmm, that would explain a lot, thanks for digging into this.
>
> Jiri, I think this patch should partially replace
> https://patchwork.kernel.org/patch/10583481/
>
> Kai-heng, if you do not need to re-request the HID descriptors, I
> think it should be good to squash these together to:
> - remove I2C_HID_QUIRK_RESEND_REPORT_DESCR
> - remove the re-request the HID descriptors on resume
> - merge with this patch

There's still one user uses I2C_HID_QUIRK_RESEND_REPORT_DESCR,  
USB_DEVICE_ID_SIS10FB_TOUCH.
So I'd like to have an Ack from Hans first.

I'll send a new patch with your suggestion afterward.

>
>
>
>> Tested this patch on other i2c-hid touchpanels I have and those
>> touchpanels do work after S3 without doing reset. If any regression
>> happens to other touchpanel vendors, we can use quirk for Raydium
>> devices.
>>
>> Cc: AceLan Kao <acelan.kao@...onical.com>
>> Cc: Aaron Ma <aaron.ma@...onical.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
>> ---
>>
>> Benjamin, Hans,
>>
>> I guess some vendors interpret spec this way, so hardware reset is an
>> unexpected request for their devices.
>> Can you give this patch a try?
>
> The only device I have that you should not is the Surface 3. And I am
> not so sure it goes into S3 fluently (it seems to, but I am not 100%
> sure). If you have tested on a Dell XPS 9360, I think there is not
> much more I could test.

At least for the Raydium touchscreen, the issue can be observed under both  
S2Idle and S3.
So I guess Surface 3 is still suitable for test if it can do S2Idle.

I've test this on XPS 9370 and Precision 5520, the patch doesn't break them.

Kai-Heng

>
> Cheers,
> Benjamin
>
>>  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 57126f6837bb..7567034a009f 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -1237,7 +1237,7 @@ static int i2c_hid_resume(struct device *dev)
>>         pm_runtime_enable(dev);
>>
>>         enable_irq(client->irq);
>> -       ret = i2c_hid_hwreset(client);
>> +       ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
>>         if (ret)
>>                 return ret;
>>
>> --
>> 2.17.1