For a broken touchpad, it may take several months or longer to be fixed.
Polling mode could be a fallback solution for enthusiastic Linux users
when they have a new laptop. It also acts like a debugging feature. If
polling mode works for a broken touchpad, we can almost be certain
the root cause is related to the interrupt.
Two module parameters are added to i2c-hid,
- polling_mode: by default set to 0, i.e., polling is disabled
- polling_interval (ms): user can change this runtime parameter by
writing to /sys/module/i2c_hid/parameters/polling_interval
Signed-off-by: Coiby Xu <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 127 +++++++++++++++++++++++++++--
1 file changed, 119 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index dbd04492825d..c7af127959f6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -36,6 +36,8 @@
#include <linux/hid.h>
#include <linux/mutex.h>
#include <linux/acpi.h>
+#include <linux/kthread.h>
+#include <linux/gpio/driver.h>
#include <linux/of.h>
#include <linux/regulator/consumer.h>
@@ -60,6 +62,18 @@
#define I2C_HID_PWR_ON 0x00
#define I2C_HID_PWR_SLEEP 0x01
+/* polling mode */
+#define I2C_POLLING_DISABLED 0
+#define I2C_POLLING_GPIO_PIN 1
+#define POLLING_INTERVAL 10
+
+static u8 polling_mode;
+module_param(polling_mode, byte, 0444);
+MODULE_PARM_DESC(polling_mode, "How to poll - 0 disabled; 1 based on GPIO pin's status");
+
+static unsigned int polling_interval = 10;
+module_param(polling_interval, uint, 0644);
+MODULE_PARM_DESC(polling_interval, "Poll every {polling_interval} ms. Default to 10 ms");
/* debug option */
static bool debug;
module_param(debug, bool, 0444);
@@ -158,6 +172,8 @@ struct i2c_hid {
struct i2c_hid_platform_data pdata;
+ struct task_struct *polling_thread;
+
bool irq_wake_enabled;
struct mutex reset_lock;
};
@@ -772,7 +788,9 @@ static int i2c_hid_start(struct hid_device *hid)
i2c_hid_free_buffers(ihid);
ret = i2c_hid_alloc_buffers(ihid, bufsize);
- enable_irq(client->irq);
+
+ if (polling_mode == I2C_POLLING_DISABLED)
+ enable_irq(client->irq);
if (ret)
return ret;
@@ -814,6 +832,83 @@ struct hid_ll_driver i2c_hid_ll_driver = {
};
EXPORT_SYMBOL_GPL(i2c_hid_ll_driver);
+static int get_gpio_pin_state(struct irq_desc *irq_desc)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(&irq_desc->irq_data);
+
+ return gc->get(gc, irq_desc->irq_data.hwirq);
+}
+
+static bool interrupt_line_active(struct i2c_client *client)
+{
+ unsigned long trigger_type = irq_get_trigger_type(client->irq);
+ struct irq_desc *irq_desc = irq_to_desc(client->irq);
+
+ /*
+ * According to Windows Precsiontion Touchpad's specs
+ * https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-device-bus-connectivity,
+ * GPIO Interrupt Assertion Leve could be either ActiveLow or
+ * ActiveHigh.
+ */
+ if (trigger_type & IRQF_TRIGGER_LOW)
+ return !get_gpio_pin_state(irq_desc);
+
+ return get_gpio_pin_state(irq_desc);
+}
+
+static int i2c_hid_polling_thread(void *i2c_hid)
+{
+ struct i2c_hid *ihid = i2c_hid;
+ struct i2c_client *client = ihid->client;
+ unsigned int polling_interval_ms;
+
+ while (1) {
+ polling_interval_ms = polling_interval*1000;
+ if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
+ usleep_range(50000, 100000);
+
+ if (kthread_should_stop())
+ break;
+
+ while (interrupt_line_active(client)) {
+ i2c_hid_get_input(ihid);
+ /*
+ * keeping polling for new data at a rate of ~200Hz
+ * until the interrupt line becomes inactive
+ */
+ usleep_range(4000, 6000);
+ }
+
+ usleep_range(polling_interval_ms, polling_interval_ms);
+ }
+
+ do_exit(0);
+ return 0;
+}
+
+static int i2c_hid_init_polling(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+
+ if (!irq_get_trigger_type(client->irq)) {
+ dev_warn(&client->dev,
+ "Failed to get GPIO Interrupt Assertion Level, could not enable polling mode for %s",
+ client->name);
+ return -1;
+ }
+
+ ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid,
+ "I2C HID polling thread");
+
+ if (ihid->polling_thread) {
+ pr_info("I2C HID polling thread");
+ wake_up_process(ihid->polling_thread);
+ return 0;
+ }
+
+ return -1;
+}
+
static int i2c_hid_init_irq(struct i2c_client *client)
{
struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -997,6 +1092,16 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client,
pdata->post_power_delay_ms = val;
}
+static void free_irq_or_stop_polling(struct i2c_client *client,
+ struct i2c_hid *ihid)
+{
+
+ if (polling_mode != I2C_POLLING_DISABLED)
+ kthread_stop(ihid->polling_thread);
+ else
+ free_irq(client->irq, ihid);
+}
+
static int i2c_hid_probe(struct i2c_client *client,
const struct i2c_device_id *dev_id)
{
@@ -1090,7 +1195,11 @@ static int i2c_hid_probe(struct i2c_client *client,
if (ret < 0)
goto err_regulator;
- ret = i2c_hid_init_irq(client);
+ if (polling_mode != I2C_POLLING_DISABLED)
+ ret = i2c_hid_init_polling(ihid);
+ else
+ ret = i2c_hid_init_irq(client);
+
if (ret < 0)
goto err_regulator;
@@ -1129,7 +1238,7 @@ static int i2c_hid_probe(struct i2c_client *client,
hid_destroy_device(hid);
err_irq:
- free_irq(client->irq, ihid);
+ free_irq_or_stop_polling(client, ihid);
err_regulator:
regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
@@ -1146,7 +1255,7 @@ static int i2c_hid_remove(struct i2c_client *client)
hid = ihid->hid;
hid_destroy_device(hid);
- free_irq(client->irq, ihid);
+ free_irq_or_stop_polling(client, ihid);
if (ihid->bufsize)
i2c_hid_free_buffers(ihid);
@@ -1162,7 +1271,7 @@ static void i2c_hid_shutdown(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
- free_irq(client->irq, ihid);
+ free_irq_or_stop_polling(client, ihid);
}
#ifdef CONFIG_PM_SLEEP
@@ -1183,7 +1292,8 @@ static int i2c_hid_suspend(struct device *dev)
/* Save some power */
i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
- disable_irq(client->irq);
+ if (polling_mode == I2C_POLLING_DISABLED)
+ disable_irq(client->irq);
if (device_may_wakeup(&client->dev)) {
wake_status = enable_irq_wake(client->irq);
@@ -1216,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
if (ihid->pdata.post_power_delay_ms)
msleep(ihid->pdata.post_power_delay_ms);
- } else if (ihid->irq_wake_enabled) {
+ } else if (ihid->irq_wake_enabled && polling_mode != I2C_POLLING_DISABLED) {
wake_status = disable_irq_wake(client->irq);
if (!wake_status)
ihid->irq_wake_enabled = false;
@@ -1225,7 +1335,8 @@ static int i2c_hid_resume(struct device *dev)
wake_status);
}
- enable_irq(client->irq);
+ if (polling_mode != I2C_POLLING_DISABLED)
+ enable_irq(client->irq);
/* Instead of resetting device, simply powers the device on. This
* solves "incomplete reports" on Raydium devices 2386:3118 and
--
2.28.0
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 9bdaeff2e281a849350f5e6fb194c9eed4a7abf7 ("[PATCH] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status")
url: https://github.com/0day-ci/linux/commits/Coiby-Xu/HID-i2c-hid-add-polling-mode-based-on-connected-GPIO-chip-s-pin-status/20201009-161242
base: https://git.kernel.org/cgit/linux/kernel/git/jikos/trivial.git for-next
in testcase: suspend-stress
version:
with following parameters:
mode: mem
iterations: 10
on test machine: 4 threads Skylake with 8G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
kern :info : [ 15.209413] i2c_hid i2c-SYNA2B29:00: calling i2c_hid_resume+0x0/0x100 [i2c_hid] @ 629, parent: i2c-2
kern :info : [ 15.210766] coretemp coretemp.0: platform_pm_resume+0x0/0x40 returned 0 after 0 usecs
kern :info : [ 15.210783] intel_pmc_core intel_pmc_core.0: calling platform_pm_resume+0x0/0x40 @ 565, parent: platform
kern :info : [ 15.214954] intel_pmc_core intel_pmc_core.0: platform_pm_resume+0x0/0x40 returned 0 after 0 usecs
kern :info : [ 15.216389] i2c_hid i2c-ELAN21EF:00: calling i2c_hid_resume+0x0/0x100 [i2c_hid] @ 632, parent: i2c-6
kern :info : [ 15.279476] i2c_hid i2c-ELAN21EF:00: i2c_hid_resume+0x0/0x100 [i2c_hid] returned 0 after 60240 usecs
kern :info : [ 15.280901] input input14: calling input_dev_resume+0x0/0x40 @ 565, parent: 0018:04F3:21EF.0004
kern :info : [ 15.282271] input input14: input_dev_resume+0x0/0x40 returned 0 after 0 usecs
kern :info : [ 15.283565] input input15: calling input_dev_resume+0x0/0x40 @ 565, parent: 0018:04F3:21EF.0004
kern :info : [ 15.284939] input input15: input_dev_resume+0x0/0x40 returned 0 after 0 usecs
kern :info : [ 15.286231] input input16: calling input_dev_resume+0x0/0x40 @ 565, parent: 0018:04F3:21EF.0004
kern :info : [ 15.287630] input input16: input_dev_resume+0x0/0x40 returned 0 after 0 usecs
kern :info : [ 15.638405] asix 1-1:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0xC1E1
kern :warn : [ 16.281467] hid-rmi 0018:06CB:77C6.0003: rmi_hid_read_block: timeout elapsed
kern :warn : [ 17.304554] hid-rmi 0018:06CB:77C6.0003: rmi_hid_read_block: timeout elapsed
kern :warn : [ 18.328558] hid-rmi 0018:06CB:77C6.0003: rmi_hid_read_block: timeout elapsed
kern :warn : [ 19.352476] hid-rmi 0018:06CB:77C6.0003: rmi_hid_read_block: timeout elapsed
kern :warn : [ 20.376477] hid-rmi 0018:06CB:77C6.0003: rmi_hid_read_block: timeout elapsed
kern :err : [ 20.377756] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read current IRQ mask.
kern :err : [ 20.379150] PM: dpm_run_callback(): i2c_hid_resume+0x0/0x100 [i2c_hid] returns -11
kern :info : [ 20.380478] i2c_hid i2c-SYNA2B29:00: i2c_hid_resume+0x0/0x100 [i2c_hid] returned -11 after 5047180 usecs
kern :err : [ 20.381966] PM: Device i2c-SYNA2B29:00 failed to resume async: error -11
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml
Thanks,
Rong Chen
Hi,
I believe this patch causes I2C HID devices not to work with IRQs after resuming
from suspend.
> [...]
> #ifdef CONFIG_PM_SLEEP
> @@ -1183,7 +1292,8 @@ static int i2c_hid_suspend(struct device *dev)
> /* Save some power */
> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>
> - disable_irq(client->irq);
> + if (polling_mode == I2C_POLLING_DISABLED)
> + disable_irq(client->irq);
>
The IRQ is disabled when suspending if polling is *off*.
> if (device_may_wakeup(&client->dev)) {
> wake_status = enable_irq_wake(client->irq);
> @@ -1216,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
>
> if (ihid->pdata.post_power_delay_ms)
> msleep(ihid->pdata.post_power_delay_ms);
> - } else if (ihid->irq_wake_enabled) {
> + } else if (ihid->irq_wake_enabled && polling_mode != I2C_POLLING_DISABLED) {
As a side note, I'm not sure if the 'polling_mode != I2C_POLLING_DISABLED' part
is necessary (or that it's necessary *here*). It causes 'i2c_hid_resume' and
'i2c_hid_suspend' to be "asymmetric" which - I believe - may cause problems.
> wake_status = disable_irq_wake(client->irq);
> if (!wake_status)
> ihid->irq_wake_enabled = false;
> @@ -1225,7 +1335,8 @@ static int i2c_hid_resume(struct device *dev)
> wake_status);
> }
>
> - enable_irq(client->irq);
> + if (polling_mode != I2C_POLLING_DISABLED)
> + enable_irq(client->irq);
>
The IRQ is enabled when resuming if polling is *on*. It should be enabled if polling is *off*
in my opinion.
> [...]
Regards,
Barnabás Pőcze
Hi Barnabás,
Thank you for reviewing theis patch! I've Cced a new version to you.
On Thu, Oct 15, 2020 at 10:33:50AM +0000, Barnabás Pőcze wrote:
>Hi,
>
>I believe this patch causes I2C HID devices not to work with IRQs after resuming
>from suspend.
>
>
>> [...]
>> #ifdef CONFIG_PM_SLEEP
>> @@ -1183,7 +1292,8 @@ static int i2c_hid_suspend(struct device *dev)
>> /* Save some power */
>> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>>
>> - disable_irq(client->irq);
>> + if (polling_mode == I2C_POLLING_DISABLED)
>> + disable_irq(client->irq);
>>
>
>The IRQ is disabled when suspending if polling is *off*.
>
>
>> if (device_may_wakeup(&client->dev)) {
>> wake_status = enable_irq_wake(client->irq);
>> @@ -1216,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
>>
>> if (ihid->pdata.post_power_delay_ms)
>> msleep(ihid->pdata.post_power_delay_ms);
>> - } else if (ihid->irq_wake_enabled) {
>> + } else if (ihid->irq_wake_enabled && polling_mode != I2C_POLLING_DISABLED) {
>
>As a side note, I'm not sure if the 'polling_mode != I2C_POLLING_DISABLED' part
>is necessary (or that it's necessary *here*). It causes 'i2c_hid_resume' and
>'i2c_hid_suspend' to be "asymmetric" which - I believe - may cause problems.
>
>
>> wake_status = disable_irq_wake(client->irq);
>> if (!wake_status)
>> ihid->irq_wake_enabled = false;
>> @@ -1225,7 +1335,8 @@ static int i2c_hid_resume(struct device *dev)
>> wake_status);
>> }
>>
>> - enable_irq(client->irq);
>> + if (polling_mode != I2C_POLLING_DISABLED)
>> + enable_irq(client->irq);
>>
>
>The IRQ is enabled when resuming if polling is *on*. It should be enabled if polling is *off*
>in my opinion.
>
>
>> [...]
>
>
>Regards,
>Barnabás Pőcze
--
Best regards,
Coiby