[v2] HID: i2c-hid: disable runtime PM operations on hantick touchpad
diff mbox series

Message ID 20180912130705.17431-1-anisse@astier.eu
State New
Delegated to: Jiri Kosina
Headers show
Series
  • [v2] HID: i2c-hid: disable runtime PM operations on hantick touchpad
Related show

Commit Message

Anisse Astier Sept. 12, 2018, 1:07 p.m. UTC
This hantick HTIX5288 touchpad can quickly fall in a wrong state if
there are too many open/close operations. This will either make it stop
reporting any input, or will shift all the input reads by a few bytes,
making it impossible to decode.

Here, we never release the probed touchpad runtime pm while the driver
is loaded, which should disable all runtime pm suspend/resumes.

This fast repetition of sleep/wakeup is also more likely to happen when
using runtime PM, which is why the quirk is done there, and not for all
power downs, which would include suspend or module removal.

Signed-off-by: Anisse Astier <anisse@astier.eu>
Cc: stable@vger.kernel.org
---

Changes since v1: (thanks Hans de Goede)
 - no longer uses msleep to delay suspend, but disables runtime suspend it
   entirely
 - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk


Regards,

Anisse

 drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Benjamin Tissoires Sept. 12, 2018, 1:19 p.m. UTC | #1
On Wed, Sep 12, 2018 at 3:08 PM Anisse Astier <anisse@astier.eu> wrote:
>
> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
> there are too many open/close operations. This will either make it stop
> reporting any input, or will shift all the input reads by a few bytes,
> making it impossible to decode.
>
> Here, we never release the probed touchpad runtime pm while the driver
> is loaded, which should disable all runtime pm suspend/resumes.
>
> This fast repetition of sleep/wakeup is also more likely to happen when
> using runtime PM, which is why the quirk is done there, and not for all
> power downs, which would include suspend or module removal.
>
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> Cc: stable@vger.kernel.org
> ---
>
> Changes since v1: (thanks Hans de Goede)
>  - no longer uses msleep to delay suspend, but disables runtime suspend it
>    entirely

I like it better. Thanks both of you :)

Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk
>
>
> Regards,
>
> Anisse
>
>  drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 847b55ad0d60..9917eb2c98f3 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -48,6 +48,7 @@
>  #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV       BIT(0)
>  #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET       BIT(1)
>  #define I2C_HID_QUIRK_RESEND_REPORT_DESCR      BIT(2)
> +#define I2C_HID_QUIRK_NO_RUNTIME_PM            BIT(3)
>
>  /* flags */
>  #define I2C_HID_STARTED                0
> @@ -171,7 +172,8 @@ static const struct i2c_hid_quirks {
>         { USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>                 I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>         { I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
> -               I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
> +               I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
> +               I2C_HID_QUIRK_NO_RUNTIME_PM },
>         { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS10FB_TOUCH,
>                 I2C_HID_QUIRK_RESEND_REPORT_DESCR },
>         { 0, 0 }
> @@ -1097,7 +1099,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>                 goto err_mem_free;
>         }
>
> -       pm_runtime_put(&client->dev);
> +       if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +               pm_runtime_put(&client->dev);
> +
>         return 0;
>
>  err_mem_free:
> @@ -1124,7 +1128,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
>         struct hid_device *hid;
>
> -       pm_runtime_get_sync(&client->dev);
> +       if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +               pm_runtime_get_sync(&client->dev);
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
>         pm_runtime_put_noidle(&client->dev);
> --
> 2.11.0
>
Hans de Goede Sept. 12, 2018, 1:34 p.m. UTC | #2
Hi,

On 12-09-18 15:19, Benjamin Tissoires wrote:
> On Wed, Sep 12, 2018 at 3:08 PM Anisse Astier <anisse@astier.eu> wrote:
>>
>> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
>> there are too many open/close operations. This will either make it stop
>> reporting any input, or will shift all the input reads by a few bytes,
>> making it impossible to decode.
>>
>> Here, we never release the probed touchpad runtime pm while the driver
>> is loaded, which should disable all runtime pm suspend/resumes.
>>
>> This fast repetition of sleep/wakeup is also more likely to happen when
>> using runtime PM, which is why the quirk is done there, and not for all
>> power downs, which would include suspend or module removal.
>>
>> Signed-off-by: Anisse Astier <anisse@astier.eu>
>> Cc: stable@vger.kernel.org
>> ---
>>
>> Changes since v1: (thanks Hans de Goede)
>>   - no longer uses msleep to delay suspend, but disables runtime suspend it
>>     entirely
> 
> I like it better.

Agreed:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> Thanks both of you :)
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> >
> Cheers,
> Benjamin
> 
>>   - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk
>>
>>
>> Regards,
>>
>> Anisse
>>
>>   drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 847b55ad0d60..9917eb2c98f3 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -48,6 +48,7 @@
>>   #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV       BIT(0)
>>   #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET       BIT(1)
>>   #define I2C_HID_QUIRK_RESEND_REPORT_DESCR      BIT(2)
>> +#define I2C_HID_QUIRK_NO_RUNTIME_PM            BIT(3)
>>
>>   /* flags */
>>   #define I2C_HID_STARTED                0
>> @@ -171,7 +172,8 @@ static const struct i2c_hid_quirks {
>>          { USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>>                  I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>>          { I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
>> -               I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
>> +               I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
>> +               I2C_HID_QUIRK_NO_RUNTIME_PM },
>>          { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS10FB_TOUCH,
>>                  I2C_HID_QUIRK_RESEND_REPORT_DESCR },
>>          { 0, 0 }
>> @@ -1097,7 +1099,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>>                  goto err_mem_free;
>>          }
>>
>> -       pm_runtime_put(&client->dev);
>> +       if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
>> +               pm_runtime_put(&client->dev);
>> +
>>          return 0;
>>
>>   err_mem_free:
>> @@ -1124,7 +1128,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>>          struct i2c_hid *ihid = i2c_get_clientdata(client);
>>          struct hid_device *hid;
>>
>> -       pm_runtime_get_sync(&client->dev);
>> +       if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
>> +               pm_runtime_get_sync(&client->dev);
>>          pm_runtime_disable(&client->dev);
>>          pm_runtime_set_suspended(&client->dev);
>>          pm_runtime_put_noidle(&client->dev);
>> --
>> 2.11.0
>>
Hans de Goede Sept. 12, 2018, 1:36 p.m. UTC | #3
Hi,

On 12-09-18 15:07, Anisse Astier wrote:
> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
> there are too many open/close operations. This will either make it stop
> reporting any input, or will shift all the input reads by a few bytes,
> making it impossible to decode.
> 
> Here, we never release the probed touchpad runtime pm while the driver
> is loaded, which should disable all runtime pm suspend/resumes.
> 
> This fast repetition of sleep/wakeup is also more likely to happen when
> using runtime PM, which is why the quirk is done there, and not for all
> power downs, which would include suspend or module removal.
> 
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> Cc: stable@vger.kernel.org
> ---
> 
> Changes since v1: (thanks Hans de Goede)
>   - no longer uses msleep to delay suspend, but disables runtime suspend it
>     entirely
>   - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk

Hmm, this conflicts with my:

"HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"

patch. Jiri, how do you want to handle this ?

Regards,

Hans




> 
> 
> Regards,
> 
> Anisse
> 
>   drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 847b55ad0d60..9917eb2c98f3 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -48,6 +48,7 @@
>   #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
>   #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(1)
>   #define I2C_HID_QUIRK_RESEND_REPORT_DESCR	BIT(2)
> +#define I2C_HID_QUIRK_NO_RUNTIME_PM		BIT(3)
>   
>   /* flags */
>   #define I2C_HID_STARTED		0
> @@ -171,7 +172,8 @@ static const struct i2c_hid_quirks {
>   	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>   		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>   	{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
> -		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
> +		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
> +		I2C_HID_QUIRK_NO_RUNTIME_PM },
>   	{ USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS10FB_TOUCH,
>   		I2C_HID_QUIRK_RESEND_REPORT_DESCR },
>   	{ 0, 0 }
> @@ -1097,7 +1099,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>   		goto err_mem_free;
>   	}
>   
> -	pm_runtime_put(&client->dev);
> +	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +		pm_runtime_put(&client->dev);
> +
>   	return 0;
>   
>   err_mem_free:
> @@ -1124,7 +1128,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>   	struct i2c_hid *ihid = i2c_get_clientdata(client);
>   	struct hid_device *hid;
>   
> -	pm_runtime_get_sync(&client->dev);
> +	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +		pm_runtime_get_sync(&client->dev);
>   	pm_runtime_disable(&client->dev);
>   	pm_runtime_set_suspended(&client->dev);
>   	pm_runtime_put_noidle(&client->dev);
>
Anisse Astier Sept. 12, 2018, 3:52 p.m. UTC | #4
On Wed, Sep 12, 2018 at 03:36:08PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-09-18 15:07, Anisse Astier wrote:
> > This hantick HTIX5288 touchpad can quickly fall in a wrong state if
> > there are too many open/close operations. This will either make it stop
> > reporting any input, or will shift all the input reads by a few bytes,
> > making it impossible to decode.
> > 
> > Here, we never release the probed touchpad runtime pm while the driver
> > is loaded, which should disable all runtime pm suspend/resumes.
> > 
> > This fast repetition of sleep/wakeup is also more likely to happen when
> > using runtime PM, which is why the quirk is done there, and not for all
> > power downs, which would include suspend or module removal.
> > 
> > Signed-off-by: Anisse Astier <anisse@astier.eu>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> > Changes since v1: (thanks Hans de Goede)
> >   - no longer uses msleep to delay suspend, but disables runtime suspend it
> >     entirely
> >   - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk
> 
> Hmm, this conflicts with my:
> 
> "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
> 
> patch. Jiri, how do you want to handle this ?
> 

Jiri, this should be an easy merge, but for completeness I rebased on
top of Hans' patch and built and tested the patch below:


From: Anisse Astier <anisse@astier.eu>
Date: Wed, 12 Sep 2018 09:34:33 +0000
Subject: [PATCH] HID: i2c-hid: disable runtime PM operations on hantick
 touchpad

This hantick HTIX5288 touchpad can quickly fall in a wrong state if
there are too many open/close operations. This will either make it stop
reporting any input, or will shift all the input reads by a few bytes,
making it impossible to decode.

Here, we never release the probed touchpad runtime pm while the driver
is loaded, which should disable all runtime pm suspend/resumes.

This fast repetition of sleep/wakeup is also more likely to happen when
using runtime PM, which is why the quirk is done there, and not for all
power downs, which would include suspend or module removal.

Signed-off-by: Anisse Astier <anisse@astier.eu>
Cc: stable@vger.kernel.org
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---

Changes since v2:
 - rebased on top of Hans de Goede
   "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
   patch
 - took Benjamin and Hans' ack/reviewed-by

 drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9e1800a74bbe..4e3592e7a3f7 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -47,6 +47,7 @@
 /* quirks to control the device */
 #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
 #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(1)
+#define I2C_HID_QUIRK_NO_RUNTIME_PM		BIT(2)
 
 /* flags */
 #define I2C_HID_STARTED		0
@@ -168,7 +169,8 @@ static const struct i2c_hid_quirks {
 	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
 		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
 	{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
-		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
+		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
+		I2C_HID_QUIRK_NO_RUNTIME_PM },
 	{ 0, 0 }
 };
 
@@ -1102,7 +1104,9 @@ static int i2c_hid_probe(struct i2c_client *client,
 		goto err_mem_free;
 	}
 
-	pm_runtime_put(&client->dev);
+	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+		pm_runtime_put(&client->dev);
+
 	return 0;
 
 err_mem_free:
@@ -1127,7 +1131,8 @@ static int i2c_hid_remove(struct i2c_client *client)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid;
 
-	pm_runtime_get_sync(&client->dev);
+	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+		pm_runtime_get_sync(&client->dev);
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
Hans de Goede Sept. 12, 2018, 3:56 p.m. UTC | #5
Hi,

On 12-09-18 17:52, anisse@astier.eu wrote:
> On Wed, Sep 12, 2018 at 03:36:08PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 12-09-18 15:07, Anisse Astier wrote:
>>> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
>>> there are too many open/close operations. This will either make it stop
>>> reporting any input, or will shift all the input reads by a few bytes,
>>> making it impossible to decode.
>>>
>>> Here, we never release the probed touchpad runtime pm while the driver
>>> is loaded, which should disable all runtime pm suspend/resumes.
>>>
>>> This fast repetition of sleep/wakeup is also more likely to happen when
>>> using runtime PM, which is why the quirk is done there, and not for all
>>> power downs, which would include suspend or module removal.
>>>
>>> Signed-off-by: Anisse Astier <anisse@astier.eu>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>
>>> Changes since v1: (thanks Hans de Goede)
>>>    - no longer uses msleep to delay suspend, but disables runtime suspend it
>>>      entirely
>>>    - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk
>>
>> Hmm, this conflicts with my:
>>
>> "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
>>
>> patch. Jiri, how do you want to handle this ?
>>
> 
> Jiri, this should be an easy merge, but for completeness I rebased on
> top of Hans' patch and built and tested the patch below:

Thanks, but I think that we should consider your patch for -fixes / for
4.19, so then I would need to rebase my patch on yours, after a back
merge of -fixes into -next.

Or we could put both patches in -fixes I guess.

Anyways lets wait what Jiri has to say about this.

Regards,

Hans





> 
> 
> From: Anisse Astier <anisse@astier.eu>
> Date: Wed, 12 Sep 2018 09:34:33 +0000
> Subject: [PATCH] HID: i2c-hid: disable runtime PM operations on hantick
>   touchpad
> 
> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
> there are too many open/close operations. This will either make it stop
> reporting any input, or will shift all the input reads by a few bytes,
> making it impossible to decode.
> 
> Here, we never release the probed touchpad runtime pm while the driver
> is loaded, which should disable all runtime pm suspend/resumes.
> 
> This fast repetition of sleep/wakeup is also more likely to happen when
> using runtime PM, which is why the quirk is done there, and not for all
> power downs, which would include suspend or module removal.
> 
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> Cc: stable@vger.kernel.org
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 
> Changes since v2:
>   - rebased on top of Hans de Goede
>     "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
>     patch
>   - took Benjamin and Hans' ack/reviewed-by
> 
>   drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 9e1800a74bbe..4e3592e7a3f7 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -47,6 +47,7 @@
>   /* quirks to control the device */
>   #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
>   #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(1)
> +#define I2C_HID_QUIRK_NO_RUNTIME_PM		BIT(2)
>   
>   /* flags */
>   #define I2C_HID_STARTED		0
> @@ -168,7 +169,8 @@ static const struct i2c_hid_quirks {
>   	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>   		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>   	{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
> -		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
> +		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
> +		I2C_HID_QUIRK_NO_RUNTIME_PM },
>   	{ 0, 0 }
>   };
>   
> @@ -1102,7 +1104,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>   		goto err_mem_free;
>   	}
>   
> -	pm_runtime_put(&client->dev);
> +	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +		pm_runtime_put(&client->dev);
> +
>   	return 0;
>   
>   err_mem_free:
> @@ -1127,7 +1131,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>   	struct i2c_hid *ihid = i2c_get_clientdata(client);
>   	struct hid_device *hid;
>   
> -	pm_runtime_get_sync(&client->dev);
> +	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
> +		pm_runtime_get_sync(&client->dev);
>   	pm_runtime_disable(&client->dev);
>   	pm_runtime_set_suspended(&client->dev);
>   	pm_runtime_put_noidle(&client->dev);
>
Hans de Goede Sept. 12, 2018, 5:40 p.m. UTC | #6
Hi,

On 12-09-18 17:56, Hans de Goede wrote:
> Hi,
> 
> On 12-09-18 17:52, anisse@astier.eu wrote:
>> On Wed, Sep 12, 2018 at 03:36:08PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 12-09-18 15:07, Anisse Astier wrote:
>>>> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
>>>> there are too many open/close operations. This will either make it stop
>>>> reporting any input, or will shift all the input reads by a few bytes,
>>>> making it impossible to decode.
>>>>
>>>> Here, we never release the probed touchpad runtime pm while the driver
>>>> is loaded, which should disable all runtime pm suspend/resumes.
>>>>
>>>> This fast repetition of sleep/wakeup is also more likely to happen when
>>>> using runtime PM, which is why the quirk is done there, and not for all
>>>> power downs, which would include suspend or module removal.
>>>>
>>>> Signed-off-by: Anisse Astier <anisse@astier.eu>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>
>>>> Changes since v1: (thanks Hans de Goede)
>>>>    - no longer uses msleep to delay suspend, but disables runtime suspend it
>>>>      entirely
>>>>    - quirk is now named NO_RUNTIME_PM and OR-ed with the other Hantick quirk
>>>
>>> Hmm, this conflicts with my:
>>>
>>> "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
>>>
>>> patch. Jiri, how do you want to handle this ?
>>>
>>
>> Jiri, this should be an easy merge, but for completeness I rebased on
>> top of Hans' patch and built and tested the patch below:
> 
> Thanks, but I think that we should consider your patch for -fixes / for
> 4.19, so then I would need to rebase my patch on yours, after a back
> merge of -fixes into -next.
> 
> Or we could put both patches in -fixes I guess.

Ok, so I got test results from Philip Müller who has a
Yepo 373a which is one of the devices from:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244
https://bugzilla.kernel.org/show_bug.cgi?id=199821

So this indeed finally fixes this long standing and annoying
bug. Thank you for this Anisse!

Philiph indicated that we may add his:

Tested-by: Philip Müller <philm@manjaro.org>

This makes this patch definitely -fixes and stable material.

As for my "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
patch I've done the testing of the SIS touchscreens found on the following:

Asus T100HA
Asus T200TA
Peaq C1010
Toshiba Click Mini L9W-B

With that patch applied on top of this patch:
"HID: i2c-hid: Don't reset device upon system resume"
Which is already pending in for-4.19/fixes

So it might be best to out that in fixes too and go with the
rebased version of Anisse's patch.

Regards,

Hans


>> From: Anisse Astier <anisse@astier.eu>
>> Date: Wed, 12 Sep 2018 09:34:33 +0000
>> Subject: [PATCH] HID: i2c-hid: disable runtime PM operations on hantick
>>   touchpad
>>
>> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
>> there are too many open/close operations. This will either make it stop
>> reporting any input, or will shift all the input reads by a few bytes,
>> making it impossible to decode.
>>
>> Here, we never release the probed touchpad runtime pm while the driver
>> is loaded, which should disable all runtime pm suspend/resumes.
>>
>> This fast repetition of sleep/wakeup is also more likely to happen when
>> using runtime PM, which is why the quirk is done there, and not for all
>> power downs, which would include suspend or module removal.
>>
>> Signed-off-by: Anisse Astier <anisse@astier.eu>
>> Cc: stable@vger.kernel.org
>> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>
>> Changes since v2:
>>   - rebased on top of Hans de Goede
>>     "HID: i2c-hid: Remove RESEND_REPORT_DESCR quirk and its handling"
>>     patch
>>   - took Benjamin and Hans' ack/reviewed-by
>>
>>   drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 9e1800a74bbe..4e3592e7a3f7 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -47,6 +47,7 @@
>>   /* quirks to control the device */
>>   #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV    BIT(0)
>>   #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET    BIT(1)
>> +#define I2C_HID_QUIRK_NO_RUNTIME_PM        BIT(2)
>>   /* flags */
>>   #define I2C_HID_STARTED        0
>> @@ -168,7 +169,8 @@ static const struct i2c_hid_quirks {
>>       { USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>>           I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>>       { I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
>> -        I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
>> +        I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
>> +        I2C_HID_QUIRK_NO_RUNTIME_PM },
>>       { 0, 0 }
>>   };
>> @@ -1102,7 +1104,9 @@ static int i2c_hid_probe(struct i2c_client *client,
>>           goto err_mem_free;
>>       }
>> -    pm_runtime_put(&client->dev);
>> +    if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
>> +        pm_runtime_put(&client->dev);
>> +
>>       return 0;
>>   err_mem_free:
>> @@ -1127,7 +1131,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>>       struct i2c_hid *ihid = i2c_get_clientdata(client);
>>       struct hid_device *hid;
>> -    pm_runtime_get_sync(&client->dev);
>> +    if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
>> +        pm_runtime_get_sync(&client->dev);
>>       pm_runtime_disable(&client->dev);
>>       pm_runtime_set_suspended(&client->dev);
>>       pm_runtime_put_noidle(&client->dev);
>>
Jiri Kosina Sept. 14, 2018, 8:01 a.m. UTC | #7
On Wed, 12 Sep 2018, Anisse Astier wrote:

> This hantick HTIX5288 touchpad can quickly fall in a wrong state if
> there are too many open/close operations. This will either make it stop
> reporting any input, or will shift all the input reads by a few bytes,
> making it impossible to decode.
> 
> Here, we never release the probed touchpad runtime pm while the driver
> is loaded, which should disable all runtime pm suspend/resumes.
> 
> This fast repetition of sleep/wakeup is also more likely to happen when
> using runtime PM, which is why the quirk is done there, and not for all
> power downs, which would include suspend or module removal.
> 
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> Cc: stable@vger.kernel.org

Applied to for-4.19/fixes. I'll handle the conflict with the other patch 
(which is not -fixes material) later, once it actually appears in my tree.

Thanks,

Patch
diff mbox series

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 847b55ad0d60..9917eb2c98f3 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -48,6 +48,7 @@ 
 #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
 #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(1)
 #define I2C_HID_QUIRK_RESEND_REPORT_DESCR	BIT(2)
+#define I2C_HID_QUIRK_NO_RUNTIME_PM		BIT(3)
 
 /* flags */
 #define I2C_HID_STARTED		0
@@ -171,7 +172,8 @@  static const struct i2c_hid_quirks {
 	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
 		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
 	{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
-		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
+		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
+		I2C_HID_QUIRK_NO_RUNTIME_PM },
 	{ USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS10FB_TOUCH,
 		I2C_HID_QUIRK_RESEND_REPORT_DESCR },
 	{ 0, 0 }
@@ -1097,7 +1099,9 @@  static int i2c_hid_probe(struct i2c_client *client,
 		goto err_mem_free;
 	}
 
-	pm_runtime_put(&client->dev);
+	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+		pm_runtime_put(&client->dev);
+
 	return 0;
 
 err_mem_free:
@@ -1124,7 +1128,8 @@  static int i2c_hid_remove(struct i2c_client *client)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid;
 
-	pm_runtime_get_sync(&client->dev);
+	if (!(ihid->quirks & I2C_HID_QUIRK_NO_RUNTIME_PM))
+		pm_runtime_get_sync(&client->dev);
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);