| Message ID | 20180912102213.12518-1-anisse@astier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: slower runtime PM operations on hantick touchpad | expand |
Hello Anisse, On 12-09-18 12:22, 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 synchronously block the the runtime suspend by 2.5 seconds; > this was found experimentally, and sleeping less will not result in a > non-working touchpad. > > This quirk will also result in a non-responsive touchpad for 2.5s if > userspace closes, then opens a device quickly, but this was deemed an > appropriate compromise versus a non-working touchpad. > > 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 > --- > > This patch feels a bit hack-ish, but I don't really know how to debug this > issue further. > > I'm looking forward to your feedback. Interesting, what laptop are you seeing this on, we've had lots of bug reports about this touchpad, mainly see: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244 But we've been unable to get to the bottom of this, so I'm wondering if you are using one of the machines mentioned in that bug report and thus if you are seeing the same problem. I don't think your msleep(2500) is a good solution though, I think we just need to disable runtime pm all together on these devices, can you try changing the pm_runtime_put(&client->dev); return 0; Lines at the end of i2c_hid_probe() to: if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)) pm_runtime_put(&client->dev); And in i2c_hid_remove() change: struct hid_device *hid; pm_runtime_get_sync(&client->dev); pm_runtime_disable(&client->dev); to: struct hid_device *hid; if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)) pm_runtime_get_sync(&client->dev); pm_runtime_disable(&client->dev); And then drop this bit: > @@ -1259,6 +1262,10 @@ static int i2c_hid_resume(struct device *dev) > static int i2c_hid_runtime_suspend(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > + struct i2c_hid *ihid = i2c_get_clientdata(client); > + > + if (ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM) > + msleep(2500); > > i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); > disable_irq(client->irq); From your patch and see if that also fixes things for you (I would expect it will). If that also fixes things, please also rename the quirk from I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM to I2C_HID_QUIRK_NO_RUNTIME_PM Also there already is an existing quirk for the hantick touchpad which is necessary on at least some devices. So you need to or ('|') your quirk flag together with the existing I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk for these devices And then submit a v2 upstream. Regards, Hans
Hi Hans, Thanks for taking the time to review this patch. On Wed, Sep 12, 2018 at 02:10:18PM +0200, Hans de Goede wrote: > Hello Anisse, > > On 12-09-18 12:22, 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 synchronously block the the runtime suspend by 2.5 seconds; > > this was found experimentally, and sleeping less will not result in a > > non-working touchpad. > > > > This quirk will also result in a non-responsive touchpad for 2.5s if > > userspace closes, then opens a device quickly, but this was deemed an > > appropriate compromise versus a non-working touchpad. > > > > 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 > > --- > > > > This patch feels a bit hack-ish, but I don't really know how to debug this > > issue further. > > > > I'm looking forward to your feedback. > > Interesting, what laptop are you seeing this on, we've had lots of This an OEM's Gemini Lake laptop with a Celeron N4000, I don't think it has shipped with a recognizable name. DMI information is basically "Default string" and the infamous "To Be Filled By O.E.M". > bug reports about this touchpad, mainly see: > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244 > > But we've been unable to get to the bottom of this, so I'm wondering > if you are using one of the machines mentioned in that bug report and > thus if you are seeing the same problem. > > I don't think your msleep(2500) is a good solution though, I think > we just need to disable runtime pm all together on these devices, That's actually the first thing I did, I just removed the SET_RUNTIME_PM_OPS ; but I didn't want to go nuclear and disable it entirely. It might be the best option though, I don't think this can really have a big impact on battery life. > can you try changing the > > pm_runtime_put(&client->dev); > return 0; > > Lines at the end of i2c_hid_probe() to: > > if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)) > pm_runtime_put(&client->dev); > > And in i2c_hid_remove() change: > > struct hid_device *hid; > > pm_runtime_get_sync(&client->dev); > pm_runtime_disable(&client->dev); > > to: > > struct hid_device *hid; > > if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)) > pm_runtime_get_sync(&client->dev); > > pm_runtime_disable(&client->dev); > > And then drop this bit: > > > @@ -1259,6 +1262,10 @@ static int i2c_hid_resume(struct device *dev) > > static int i2c_hid_runtime_suspend(struct device *dev) > > { > > struct i2c_client *client = to_i2c_client(dev); > > + struct i2c_hid *ihid = i2c_get_clientdata(client); > > + > > + if (ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM) > > + msleep(2500); > > i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); > > disable_irq(client->irq); > > From your patch and see if that also fixes things for you > (I would expect it will). I updated to try your approach. Note that there are other pm refcounting calls in open/close, but they should be no-ops since the counter never drops to 0 now, right ? > > If that also fixes things, please also rename the quirk from > I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM to I2C_HID_QUIRK_NO_RUNTIME_PM > > Also there already is an existing quirk for the hantick > touchpad which is necessary on at least some devices. So you need > to or ('|') your quirk flag together with the existing > I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk for these devices > > And then submit a v2 upstream. I'll send v2 in reply to this message. From quick testing it seems to work. Thanks, Anisse
Hi, On 12-09-18 14:55, Anisse Astier wrote: > Hi Hans, > > Thanks for taking the time to review this patch. > > On Wed, Sep 12, 2018 at 02:10:18PM +0200, Hans de Goede wrote: >> Hello Anisse, >> >> On 12-09-18 12:22, 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 synchronously block the the runtime suspend by 2.5 seconds; >>> this was found experimentally, and sleeping less will not result in a >>> non-working touchpad. >>> >>> This quirk will also result in a non-responsive touchpad for 2.5s if >>> userspace closes, then opens a device quickly, but this was deemed an >>> appropriate compromise versus a non-working touchpad. >>> >>> 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 >>> --- >>> >>> This patch feels a bit hack-ish, but I don't really know how to debug this >>> issue further. >>> >>> I'm looking forward to your feedback. >> >> Interesting, what laptop are you seeing this on, we've had lots of > > This an OEM's Gemini Lake laptop with a Celeron N4000, I don't think it > has shipped with a recognizable name. DMI information is basically > "Default string" and the infamous "To Be Filled By O.E.M". Ok. >> bug reports about this touchpad, mainly see: >> >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244 >> >> But we've been unable to get to the bottom of this, so I'm wondering >> if you are using one of the machines mentioned in that bug report and >> thus if you are seeing the same problem. >> >> I don't think your msleep(2500) is a good solution though, I think >> we just need to disable runtime pm all together on these devices, > > That's actually the first thing I did, I just removed the > SET_RUNTIME_PM_OPS ; but I didn't want to go nuclear and disable it > entirely. It might be the best option though, I don't think this can > really have a big impact on battery life. No, when running a graphical user session the touchpad evdev device node will be open all the time, so runtime pm will not do much. And I expect these laptops to pretty much always run a graphical user session. > >> can you try changing the >> >> pm_runtime_put(&client->dev); >> return 0; >> >> Lines at the end of i2c_hid_probe() to: >> >> if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)) >> pm_runtime_put(&client->dev); >> >> And in i2c_hid_remove() change: >> >> struct hid_device *hid; >> >> pm_runtime_get_sync(&client->dev); >> pm_runtime_disable(&client->dev); >> >> to: >> >> struct hid_device *hid; >> >> if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)) >> pm_runtime_get_sync(&client->dev); >> >> pm_runtime_disable(&client->dev); >> >> And then drop this bit: >> >>> @@ -1259,6 +1262,10 @@ static int i2c_hid_resume(struct device *dev) >>> static int i2c_hid_runtime_suspend(struct device *dev) >>> { >>> struct i2c_client *client = to_i2c_client(dev); >>> + struct i2c_hid *ihid = i2c_get_clientdata(client); >>> + >>> + if (ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM) >>> + msleep(2500); >>> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); >>> disable_irq(client->irq); >> >> From your patch and see if that also fixes things for you >> (I would expect it will). > > I updated to try your approach. Note that there are other pm refcounting > calls in open/close, but they should be no-ops since the counter never > drops to 0 now, right ? Right. >> If that also fixes things, please also rename the quirk from >> I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM to I2C_HID_QUIRK_NO_RUNTIME_PM >> >> Also there already is an existing quirk for the hantick >> touchpad which is necessary on at least some devices. So you need >> to or ('|') your quirk flag together with the existing >> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk for these devices >> >> And then submit a v2 upstream. > > I'll send v2 in reply to this message. From quick testing it seems to > work. Thanks. If this really fixes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244 You've made a lot of people very happy! I've added a comment to that bug asking people to test. Regards, Hans
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 847b55ad0d60..f894d9225df6 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_SLOW_DOWN_RUNTIME_PM BIT(3) /* flags */ #define I2C_HID_STARTED 0 @@ -172,6 +173,8 @@ static const struct i2c_hid_quirks { I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV }, { I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288, I2C_HID_QUIRK_NO_IRQ_AFTER_RESET }, + { I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288, + I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM }, { USB_VENDOR_ID_SIS_TOUCH, USB_DEVICE_ID_SIS10FB_TOUCH, I2C_HID_QUIRK_RESEND_REPORT_DESCR }, { 0, 0 } @@ -1259,6 +1262,10 @@ static int i2c_hid_resume(struct device *dev) static int i2c_hid_runtime_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); + struct i2c_hid *ihid = i2c_get_clientdata(client); + + if (ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM) + msleep(2500); i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); disable_irq(client->irq);
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 synchronously block the the runtime suspend by 2.5 seconds; this was found experimentally, and sleeping less will not result in a non-working touchpad. This quirk will also result in a non-responsive touchpad for 2.5s if userspace closes, then opens a device quickly, but this was deemed an appropriate compromise versus a non-working touchpad. 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 --- This patch feels a bit hack-ish, but I don't really know how to debug this issue further. I'm looking forward to your feedback. Regards, Anisse drivers/hid/i2c-hid/i2c-hid.c | 7 +++++++ 1 file changed, 7 insertions(+)