[PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Coiby Xu]

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 or power setting.

When polling mode is enabled, an I2C device can't wake up the suspended
system since enable/disable_irq_wake is invalid for polling mode.

Three module parameters are added to i2c-hid,
    - polling_mode: by default set to 0, i.e., polling is disabled
    - polling_interval_idle_ms: the polling internal when the touchpad
      is idle, default to 10ms
    - polling_interval_active_us: the polling internal when the touchpad
      is active, default to 4000us

User can change the last two runtime polling parameter by writing to
/sys/module/i2c_hid/parameters/polling_interval_{idle_ms,active_us}.

Cc: <stable@vger.kernel.org>
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 151 ++++++++++++++++++++++++++---
 1 file changed, 135 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index dbd04492825d..0bb8075424b6 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,24 @@
 #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_active_us = 4000;
+module_param(polling_interval_active_us, uint, 0644);
+MODULE_PARM_DESC(polling_interval_active_us,
+		 "Poll every {polling_interval_active_us} us when the touchpad is active. Default to 4000 us");
+
+static unsigned int polling_interval_idle_ms = 10;
+module_param(polling_interval_idle_ms, uint, 0644);
+MODULE_PARM_DESC(polling_interval_ms,
+		 "Poll every {polling_interval_idle_ms} ms when the touchpad is idle. Default to 10 ms");
 /* debug option */
 static bool debug;
 module_param(debug, bool, 0444);
@@ -158,6 +178,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 +794,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 +838,86 @@ 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_idle;
+
+	while (1) {
+		/*
+		 * re-calculate polling_interval_idle
+		 * so the module parameters polling_interval_idle_ms can be
+		 * changed dynamically through sysfs as polling_interval_active_us
+		 */
+		polling_interval_idle = polling_interval_idle_ms * 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);
+			usleep_range(polling_interval_active_us,
+				     polling_interval_active_us + 100);
+		}
+
+		usleep_range(polling_interval_idle,
+			     polling_interval_idle + 1000);
+	}
+
+	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 +1101,15 @@ 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 +1203,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 +1246,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 +1263,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 +1279,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,15 +1300,16 @@ 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 (device_may_wakeup(&client->dev)) {
-		wake_status = enable_irq_wake(client->irq);
-		if (!wake_status)
-			ihid->irq_wake_enabled = true;
-		else
-			hid_warn(hid, "Failed to enable irq wake: %d\n",
-				wake_status);
+	if (polling_mode == I2C_POLLING_DISABLED) {
+		disable_irq(client->irq);
+		if (device_may_wakeup(&client->dev)) {
+			wake_status = enable_irq_wake(client->irq);
+			if (!wake_status)
+				ihid->irq_wake_enabled = true;
+			else
+				hid_warn(hid, "Failed to enable irq wake: %d\n",
+					 wake_status);
+		}
 	} else {
 		regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
 				       ihid->pdata.supplies);
@@ -1208,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
 	struct hid_device *hid = ihid->hid;
 	int wake_status;

-	if (!device_may_wakeup(&client->dev)) {
+	if (!device_may_wakeup(&client->dev) || polling_mode != I2C_POLLING_DISABLED) {
 		ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
 					    ihid->pdata.supplies);
 		if (ret)
@@ -1225,7 +1343,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

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Barnabás Pőcze]

Hi,

I still think that `i2c_hid_resume()` and `i2c_hid_suspend()` are asymmetric and
that might lead to issues.


> [...]
> When polling mode is enabled, an I2C device can't wake up the suspended
> system since enable/disable_irq_wake is invalid for polling mode.
>

Excuse my ignorance, but could you elaborate this because I am not sure I understand.
Aren't the two things orthogonal (polling and waking up the system)?


> [...]
>  #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

This is a very small detail, but I personally think that these defines should be
called I2C_HID_.... since they are only used here.


> +#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_active_us = 4000;
> +module_param(polling_interval_active_us, uint, 0644);
> +MODULE_PARM_DESC(polling_interval_active_us,
> +		 "Poll every {polling_interval_active_us} us when the touchpad is active. Default to 4000 us");
> +
> +static unsigned int polling_interval_idle_ms = 10;

There is a define for this value, I don't really see why you don't use it here.
And if there is a define for one value, I don't really see why there isn't one
for the other. (As far as I see `POLLING_INTERVAL` is not even used anywhere.)


> +module_param(polling_interval_idle_ms, uint, 0644);
> +MODULE_PARM_DESC(polling_interval_ms,
> +		 "Poll every {polling_interval_idle_ms} ms when the touchpad is idle. Default to 10 ms");
>  /* debug option */
>  static bool debug;
>  module_param(debug, bool, 0444);
> @@ -158,6 +178,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 +794,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 +838,86 @@ 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);
> +}

Excuse my ignorance, but I think some kind of error handling regarding the return
value of `get_gpio_pin_state()` should be present here.


> +
> +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_idle;
> +
> +	while (1) {
> +		/*
> +		 * re-calculate polling_interval_idle
> +		 * so the module parameters polling_interval_idle_ms can be
> +		 * changed dynamically through sysfs as polling_interval_active_us
> +		 */
> +		polling_interval_idle = polling_interval_idle_ms * 1000;
> +		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
> +			usleep_range(50000, 100000);
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		while (interrupt_line_active(client)) {

I realize it's quite unlikely, but can't this be a endless loop if data is coming
in at a high enough rate? Maybe the maximum number of iterations could be limited here?


> +			i2c_hid_get_input(ihid);
> +			usleep_range(polling_interval_active_us,
> +				     polling_interval_active_us + 100);
> +		}
> +
> +		usleep_range(polling_interval_idle,
> +			     polling_interval_idle + 1000);
> +	}
> +
> +	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) {

`kthread_create()` returns an errno in a pointer, so this check is incorrect. It should be

 if (!IS_ERR(ihid->polling_thread))

I think it's a bit inconsistent that in this function you do:

 if (err)
   bail

 if (!err)
   return ok

 return err

moreover, I think the errno should be propagated, so use

 return PTR_ERR(ihid->polling_thread);

for example, when bailing out.


> +		pr_info("I2C HID polling thread");
> +		wake_up_process(ihid->polling_thread);
> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
> [...]
>  #ifdef CONFIG_PM_SLEEP
> @@ -1183,15 +1300,16 @@ 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 (device_may_wakeup(&client->dev)) {
> -		wake_status = enable_irq_wake(client->irq);
> -		if (!wake_status)
> -			ihid->irq_wake_enabled = true;
> -		else
> -			hid_warn(hid, "Failed to enable irq wake: %d\n",
> -				wake_status);
> +	if (polling_mode == I2C_POLLING_DISABLED) {
> +		disable_irq(client->irq);
> +		if (device_may_wakeup(&client->dev)) {
> +			wake_status = enable_irq_wake(client->irq);
> +			if (!wake_status)
> +				ihid->irq_wake_enabled = true;
> +			else
> +				hid_warn(hid, "Failed to enable irq wake: %d\n",
> +					 wake_status);
> +		}
>  	} else {
>  		regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
>  				       ihid->pdata.supplies);
> @@ -1208,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
>  	struct hid_device *hid = ihid->hid;
>  	int wake_status;
>
> -	if (!device_may_wakeup(&client->dev)) {
> +	if (!device_may_wakeup(&client->dev) || polling_mode != I2C_POLLING_DISABLED) {
>  		ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
>  					    ihid->pdata.supplies);
>  		if (ret)
> @@ -1225,7 +1343,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);
> [...]

Before this patch, if a device cannot wake up, then the regulators are disabled
when suspending, after this patch, regulators are only disabled if polling is
used. But they are enabled if the device cannot wake up *or* polling is used.

Excuse my ignorance, but I do not understand why the following two changes are not enough:

in `i2c_hid_suspend()`:
 if (polling_mode == I2C_POLLING_DISABLED)
   disable_irq(client->irq);

in `i2c_hid_resume()`:
 if (polling_mode == I2C_POLLING_DISABLED)
   enable_irq(client->irq);


Regards,
Barnabás Pőcze

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Coiby Xu]

Hi,

Thank you for examine this patch in such a careful way!

On Fri, Oct 16, 2020 at 03:00:49PM +0000, Barnabás Pőcze wrote:
>Hi,
>
>I still think that `i2c_hid_resume()` and `i2c_hid_suspend()` are asymmetric and
>that might lead to issues.
>

Do you think this commit message is relevant to your concern?

$ git show d1c48038b849e9df0475621a52193a62424a4e87
commit d1c48038b849e9df0475621a52193a62424a4e87
     HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

     Enabling irq wake could potentially fail and calling disable_irq_wake
     after a failed call to enable_irq_wake could result in an unbalanced irq
     warning. This patch warns if enable_irq_wake fails and avoids other
     potential issues caused by calling disable_irq_wake on resume after
     enable_irq_wake failed during suspend.

So I think all cases about irq have been handled. But for the regulator
part, you are right. I made a mistake.
>
>> [...]
>> When polling mode is enabled, an I2C device can't wake up the suspended
>> system since enable/disable_irq_wake is invalid for polling mode.
>>
>
>Excuse my ignorance, but could you elaborate this because I am not sure I understand.
>Aren't the two things orthogonal (polling and waking up the system)?
>
Waking up the system depends on irq. Since we use polling, we don't set
up irq.
>
>> [...]
>>  #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
>
>This is a very small detail, but I personally think that these defines should be
>called I2C_HID_.... since they are only used here.
>
Thank you! This is absolutely a good suggestion.
>
>> +#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_active_us = 4000;
>> +module_param(polling_interval_active_us, uint, 0644);
>> +MODULE_PARM_DESC(polling_interval_active_us,
>> +		 "Poll every {polling_interval_active_us} us when the touchpad is active. Default to 4000 us");
>> +
>> +static unsigned int polling_interval_idle_ms = 10;
>
>There is a define for this value, I don't really see why you don't use it here.
>And if there is a define for one value, I don't really see why there isn't one
>for the other. (As far as I see `POLLING_INTERVAL` is not even used anywhere.)
>
Thank you for spotting this leftover issue after introducing two
parameters to control the polling interval.

Another issue is "MODULE_PARM_DESC(polling_interval_ms" should be
"MODULE_PARM_DESC(polling_interval_idle_ms" although this won't cause
real problem.
>
>> +module_param(polling_interval_idle_ms, uint, 0644);
>> +MODULE_PARM_DESC(polling_interval_ms,
>> +		 "Poll every {polling_interval_idle_ms} ms when the touchpad is idle. Default to 10 ms");
>>  /* debug option */
>>  static bool debug;
>>  module_param(debug, bool, 0444);
>> @@ -158,6 +178,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 +794,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 +838,86 @@ 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);
>> +}
>
>Excuse my ignorance, but I think some kind of error handling regarding the return
>value of `get_gpio_pin_state()` should be present here.
>
What kind of errors would you expect? It seems (struct gpio_chip *)->get
only return 0 or 1.
>
>> +
>> +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_idle;
>> +
>> +	while (1) {
>> +		/*
>> +		 * re-calculate polling_interval_idle
>> +		 * so the module parameters polling_interval_idle_ms can be
>> +		 * changed dynamically through sysfs as polling_interval_active_us
>> +		 */
>> +		polling_interval_idle = polling_interval_idle_ms * 1000;
>> +		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
>> +			usleep_range(50000, 100000);
>> +
>> +		if (kthread_should_stop())
>> +			break;
>> +
>> +		while (interrupt_line_active(client)) {
>
>I realize it's quite unlikely, but can't this be a endless loop if data is coming
>in at a high enough rate? Maybe the maximum number of iterations could be limited here?
>
If we find HID reports are constantly read and send to front-end
application like libinput, won't it help expose the problem of the I2C
HiD device?
>
>> +			i2c_hid_get_input(ihid);
>> +			usleep_range(polling_interval_active_us,
>> +				     polling_interval_active_us + 100);
>> +		}
>> +
>> +		usleep_range(polling_interval_idle,
>> +			     polling_interval_idle + 1000);
>> +	}
>> +
>> +	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) {
>
>`kthread_create()` returns an errno in a pointer, so this check is incorrect. It should be
>
> if (!IS_ERR(ihid->polling_thread))
>
Thank you for correcting my mistake!

>I think it's a bit inconsistent that in this function you do:
>
> if (err)
>   bail
>
> if (!err)
>   return ok
>
> return err
>
I'm not sure if I get you, but current pattern is

if (err)
   return err;

if (!err)
   return ok

return err

>moreover, I think the errno should be propagated, so use
>
> return PTR_ERR(ihid->polling_thread);
>
>for example, when bailing out.
>

This a good advice! Thank you
>
>> +		pr_info("I2C HID polling thread");
>> +		wake_up_process(ihid->polling_thread);
>> +		return 0;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> [...]
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -1183,15 +1300,16 @@ 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 (device_may_wakeup(&client->dev)) {
>> -		wake_status = enable_irq_wake(client->irq);
>> -		if (!wake_status)
>> -			ihid->irq_wake_enabled = true;
>> -		else
>> -			hid_warn(hid, "Failed to enable irq wake: %d\n",
>> -				wake_status);
>> +	if (polling_mode == I2C_POLLING_DISABLED) {
>> +		disable_irq(client->irq);
>> +		if (device_may_wakeup(&client->dev)) {
>> +			wake_status = enable_irq_wake(client->irq);
>> +			if (!wake_status)
>> +				ihid->irq_wake_enabled = true;
>> +			else
>> +				hid_warn(hid, "Failed to enable irq wake: %d\n",
>> +					 wake_status);
>> +		}
>>  	} else {
>>  		regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
>>  				       ihid->pdata.supplies);
>> @@ -1208,7 +1326,7 @@ static int i2c_hid_resume(struct device *dev)
>>  	struct hid_device *hid = ihid->hid;
>>  	int wake_status;
>>
>> -	if (!device_may_wakeup(&client->dev)) {
>> +	if (!device_may_wakeup(&client->dev) || polling_mode != I2C_POLLING_DISABLED) {
>>  		ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies),
>>  					    ihid->pdata.supplies);
>>  		if (ret)
>> @@ -1225,7 +1343,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);
>> [...]
>
>Before this patch, if a device cannot wake up, then the regulators are disabled
>when suspending, after this patch, regulators are only disabled if polling is
>used. But they are enabled if the device cannot wake up *or* polling is used.
>
Thank for analyzing what's wrong for me!

>Excuse my ignorance, but I do not understand why the following two changes are not enough:
>
>in `i2c_hid_suspend()`:
> if (polling_mode == I2C_POLLING_DISABLED)
>   disable_irq(client->irq);
>
>in `i2c_hid_resume()`:
> if (polling_mode == I2C_POLLING_DISABLED)
>   enable_irq(client->irq);
>
I think we shouldn't call enable/disable_irq_wake in polling mode
where we don't set up irq.
>
>Regards,
>Barnabás Pőcze

--
Best regards,
Coiby

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Barnabás Pőcze]

Hi

> [...]
> >> +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);
> >> +}
> >
> >Excuse my ignorance, but I think some kind of error handling regarding the return
> >value of `get_gpio_pin_state()` should be present here.
> >
> What kind of errors would you expect? It seems (struct gpio_chip *)->get
> only return 0 or 1.
> >

I read the code of a couple gpio chips and - I may be wrong, but - it seems they
can return an arbitrary errno.


> >> +
> >> +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_idle;
> >> +
> >> +	while (1) {
> >> +		/*
> >> +		 * re-calculate polling_interval_idle
> >> +		 * so the module parameters polling_interval_idle_ms can be
> >> +		 * changed dynamically through sysfs as polling_interval_active_us
> >> +		 */
> >> +		polling_interval_idle = polling_interval_idle_ms * 1000;
> >> +		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
> >> +			usleep_range(50000, 100000);
> >> +
> >> +		if (kthread_should_stop())
> >> +			break;
> >> +
> >> +		while (interrupt_line_active(client)) {
> >
> >I realize it's quite unlikely, but can't this be a endless loop if data is coming
> >in at a high enough rate? Maybe the maximum number of iterations could be limited here?
> >
> If we find HID reports are constantly read and send to front-end
> application like libinput, won't it help expose the problem of the I2C
> HiD device?
> >

I'm not sure I completely understand your point. The reason why I wrote what I wrote
is that this kthread could potentially could go on forever (since `kthread_should_stop()`
is not checked in the inner while loop) if the data is supplied at a high enough rate.
That's why I said, to avoid this problem, only allow a certain number of iterations
for the inner loop, to guarantee that the kthread can stop in any case.


> >> +			i2c_hid_get_input(ihid);
> >> +			usleep_range(polling_interval_active_us,
> >> +				     polling_interval_active_us + 100);
> >> +		}
> >> +
> >> +		usleep_range(polling_interval_idle,
> >> +			     polling_interval_idle + 1000);
> >> +	}
> >> +
> >> +	do_exit(0);
> >> +	return 0;
> >> +}
> [...]
> >Excuse my ignorance, but I do not understand why the following two changes are not enough:
> >
> >in `i2c_hid_suspend()`:
> > if (polling_mode == I2C_POLLING_DISABLED)
> >   disable_irq(client->irq);
> >
> >in `i2c_hid_resume()`:
> > if (polling_mode == I2C_POLLING_DISABLED)
> >   enable_irq(client->irq);
> >
> I think we shouldn't call enable/disable_irq_wake in polling mode
> where we don't set up irq.

I think I now understand what you mean. I'm not sure, but it seems logical to me
that you can enable/disable irq wake regardless whether any irq handlers are
registered or not. Therefore, I figure it makes sense to take the safe path,
and don't touch irq wake when polling, just as you did.


> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Coiby Xu]

Hi,

On Sat, Oct 17, 2020 at 01:06:14PM +0000, Barnabás Pőcze wrote:
>Hi
>
>> [...]
>> >> +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);
>> >> +}
>> >
>> >Excuse my ignorance, but I think some kind of error handling regarding the return
>> >value of `get_gpio_pin_state()` should be present here.
>> >
>> What kind of errors would you expect? It seems (struct gpio_chip *)->get
>> only return 0 or 1.
>> >
>
>I read the code of a couple gpio chips and - I may be wrong, but - it seems they
>can return an arbitrary errno.
>
I thought all GPIO chip return 0 or 1 since !!val is returned. I find
an example which could return negative value,

// drivers/gpio/gpio-wm8994.c
static int wm8994_gpio_get(struct gpio_chip *chip, unsigned offset)
{
	struct wm8994_gpio *wm8994_gpio = gpiochip_get_data(chip);
	struct wm8994 *wm8994 = wm8994_gpio->wm8994;
	int ret;

	ret = wm8994_reg_read(wm8994, WM8994_GPIO_1 + offset);
	if (ret < 0)
		return ret;

	if (ret & WM8994_GPN_LVL)
		return 1;
	else
		return 0;
}
>
>> >> +
>> >> +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_idle;
>> >> +
>> >> +	while (1) {
>> >> +		/*
>> >> +		 * re-calculate polling_interval_idle
>> >> +		 * so the module parameters polling_interval_idle_ms can be
>> >> +		 * changed dynamically through sysfs as polling_interval_active_us
>> >> +		 */
>> >> +		polling_interval_idle = polling_interval_idle_ms * 1000;
>> >> +		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
>> >> +			usleep_range(50000, 100000);
>> >> +
>> >> +		if (kthread_should_stop())
>> >> +			break;
>> >> +
>> >> +		while (interrupt_line_active(client)) {
>> >
>> >I realize it's quite unlikely, but can't this be a endless loop if data is coming
>> >in at a high enough rate? Maybe the maximum number of iterations could be limited here?
>> >
>> If we find HID reports are constantly read and send to front-end
>> application like libinput, won't it help expose the problem of the I2C
>> HiD device?
>> >
>
>I'm not sure I completely understand your point. The reason why I wrote what I wrote
>is that this kthread could potentially could go on forever (since `kthread_should_stop()`
>is not checked in the inner while loop) if the data is supplied at a high enough rate.
>That's why I said, to avoid this problem, only allow a certain number of iterations
>for the inner loop, to guarantee that the kthread can stop in any case.
>
I mean if "data is supplied at a high enough rate" does happen, this is
an abnormal case and indicates a bug. So we shouldn't cover it up. We
expect the user to report it to us.
>
>> >> +			i2c_hid_get_input(ihid);
>> >> +			usleep_range(polling_interval_active_us,
>> >> +				     polling_interval_active_us + 100);
>> >> +		}
>> >> +
>> >> +		usleep_range(polling_interval_idle,
>> >> +			     polling_interval_idle + 1000);
>> >> +	}
>> >> +
>> >> +	do_exit(0);
>> >> +	return 0;
>> >> +}
>> [...]
>> >Excuse my ignorance, but I do not understand why the following two changes are not enough:
>> >
>> >in `i2c_hid_suspend()`:
>> > if (polling_mode == I2C_POLLING_DISABLED)
>> >   disable_irq(client->irq);
>> >
>> >in `i2c_hid_resume()`:
>> > if (polling_mode == I2C_POLLING_DISABLED)
>> >   enable_irq(client->irq);
>> >
>> I think we shouldn't call enable/disable_irq_wake in polling mode
>> where we don't set up irq.
>
>I think I now understand what you mean. I'm not sure, but it seems logical to me
>that you can enable/disable irq wake regardless whether any irq handlers are
>registered or not. Therefore, I figure it makes sense to take the safe path,
>and don't touch irq wake when polling, just as you did.
>

Thank you for offering your understandings on this patch. When I'm going
to submit next version, I will add a "Signed-off-by" tag with your name
and email, does it look good to you?
>
>> [...]
>
>
>Regards,
>Barnabás Pőcze

--
Best regards,
Coiby

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Barnabás Pőcze]

> [...]
> >> >> +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);
> >> >> +}
> >> >
> >> >Excuse my ignorance, but I think some kind of error handling regarding the return
> >> >value of `get_gpio_pin_state()` should be present here.
> >> >
> >> What kind of errors would you expect? It seems (struct gpio_chip *)->get
> >> only return 0 or 1.
> >> >
> >
> >I read the code of a couple gpio chips and - I may be wrong, but - it seems they
> >can return an arbitrary errno.
> >
> I thought all GPIO chip return 0 or 1 since !!val is returned. I find
> an example which could return negative value,
>

Yes, when a function returns `int`, there is a very high chance that the return
value may be an errno.


> >
> >> >> +
> >> >> +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_idle;
> >> >> +
> >> >> +	while (1) {
> >> >> +		/*
> >> >> +		 * re-calculate polling_interval_idle
> >> >> +		 * so the module parameters polling_interval_idle_ms can be
> >> >> +		 * changed dynamically through sysfs as polling_interval_active_us
> >> >> +		 */
> >> >> +		polling_interval_idle = polling_interval_idle_ms * 1000;
> >> >> +		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
> >> >> +			usleep_range(50000, 100000);
> >> >> +
> >> >> +		if (kthread_should_stop())
> >> >> +			break;
> >> >> +
> >> >> +		while (interrupt_line_active(client)) {
> >> >
> >> >I realize it's quite unlikely, but can't this be a endless loop if data is coming
> >> >in at a high enough rate? Maybe the maximum number of iterations could be limited here?
> >> >
> >> If we find HID reports are constantly read and send to front-end
> >> application like libinput, won't it help expose the problem of the I2C
> >> HiD device?
> >> >
> >
> >I'm not sure I completely understand your point. The reason why I wrote what I wrote
> >is that this kthread could potentially could go on forever (since `kthread_should_stop()`
> >is not checked in the inner while loop) if the data is supplied at a high enough rate.
> >That's why I said, to avoid this problem, only allow a certain number of iterations
> >for the inner loop, to guarantee that the kthread can stop in any case.
> >
> I mean if "data is supplied at a high enough rate" does happen, this is
> an abnormal case and indicates a bug. So we shouldn't cover it up. We
> expect the user to report it to us.
> >

I agree in principle, but if this abnormal case ever occurs, that'll prevent
this module from being unloaded since `kthread_stop()` will hang because the
thread is "stuck" in the inner loop, never checking `kthread_should_stop()`.
That's why I think it makes sense to only allow a certain number of operations
for the inner loop, and maybe show a warning if that's exceeded:

 for (i = 0; i < max_iter && interrupt_line_active(...); i++) {
    ....
 }

 WARN_ON[CE](i == max_iter[, "data is coming in at an unreasonably high rate"]);

or something like this, where `max_iter` could possibly be some value dependent on
`polling_interval_active_us`, or even just a constant.


> >> >> +			i2c_hid_get_input(ihid);
> >> >> +			usleep_range(polling_interval_active_us,
> >> >> +				     polling_interval_active_us + 100);
> >> >> +		}
> >> >> +
> >> >> +		usleep_range(polling_interval_idle,
> >> >> +			     polling_interval_idle + 1000);
> >> >> +	}
> >> >> +
> >> >> +	do_exit(0);
> >> >> +	return 0;
> >> >> +}
> [...]
> Thank you for offering your understandings on this patch. When I'm going
> to submit next version, I will add a "Signed-off-by" tag with your name
> and email, does it look good to you?
> [...]

I'm not sure if that follows proper procedures.

 "The sign-off is a simple line at the end of the explanation for the patch, which
  certifies that you wrote it or otherwise have the right to pass it on as an
  open-source patch."[1]

I'm not the author, nor co-author, nor am I going to pass this patch on, so I don't
think that's appropriate.

Furthermore, please note that

 "[...] you may optionally add a Cc: tag to the patch. **This is the only tag which
  might be added without an explicit action by the person it names** - but it should
  indicate that this person was copied on the patch."[2]
  (emphasis mine)


Regards,
Barnabás Pőcze


[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
[2]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Barnabás Pőcze]

> [...]
> > > > > > +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_idle;
> > > > > > -
> > > > > > -   while (1) {
> > > > > > -       /*
> > > > > >
> > > > > >
> > > > > > -        * re-calculate polling_interval_idle
> > > > > >
> > > > > >
> > > > > > -        * so the module parameters polling_interval_idle_ms can be
> > > > > >
> > > > > >
> > > > > > -        * changed dynamically through sysfs as polling_interval_active_us
> > > > > >
> > > > > >
> > > > > > -        */
> > > > > >
> > > > > >
> > > > > > -       polling_interval_idle = polling_interval_idle_ms * 1000;
> > > > > >
> > > > > >
> > > > > > -       if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
> > > > > >
> > > > > >
> > > > > > -       	usleep_range(50000, 100000);
> > > > > >
> > > > > >
> > > > > > -
> > > > > > -       if (kthread_should_stop())
> > > > > >
> > > > > >
> > > > > > -       	break;
> > > > > >
> > > > > >
> > > > > > -
> > > > > > -       while (interrupt_line_active(client)) {
> > > > > >
> > > > > >
> > > > >
> > > > > I realize it's quite unlikely, but can't this be a endless loop if data is coming
> > > > > in at a high enough rate? Maybe the maximum number of iterations could be limited here?
> > > >
> > > > If we find HID reports are constantly read and send to front-end
> > > > application like libinput, won't it help expose the problem of the I2C
> > > > HiD device?
> > > >
> > > > >
> > >
> > > I'm not sure I completely understand your point. The reason why I wrote what I wrote
> > > is that this kthread could potentially could go on forever (since `kthread_should_stop()`
> > > is not checked in the inner while loop) if the data is supplied at a high enough rate.
> > > That's why I said, to avoid this problem, only allow a certain number of iterations
> > > for the inner loop, to guarantee that the kthread can stop in any case.
> >
> > I mean if "data is supplied at a high enough rate" does happen, this is
> > an abnormal case and indicates a bug. So we shouldn't cover it up. We
> > expect the user to report it to us.
> >
> > >
>
> I agree in principle, but if this abnormal case ever occurs, that'll prevent
> this module from being unloaded since `kthread_stop()` will hang because the
> thread is "stuck" in the inner loop, never checking `kthread_should_stop()`.
> That's why I think it makes sense to only allow a certain number of operations
> for the inner loop, and maybe show a warning if that's exceeded:
>
> for (i = 0; i < max_iter && interrupt_line_active(...); i++) {
> ....
> }
>
> WARN_ON[CE](i == max_iter[, "data is coming in at an unreasonably high rate"]);
>

I now realize that WARN_ON[CE] is probably not the best fit here, `hid_warn()` is possibly better.


> or something like this, where `max_iter` could possibly be some value dependent on
> `polling_interval_active_us`, or even just a constant.
> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Coiby Xu]

On Sat, Oct 17, 2020 at 02:58:13PM +0000, Barnabás Pőcze wrote:
>> [...]
>> >> >> +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);
>> >> >> +}
>> >> >
>> >> >Excuse my ignorance, but I think some kind of error handling regarding the return
>> >> >value of `get_gpio_pin_state()` should be present here.
>> >> >
>> >> What kind of errors would you expect? It seems (struct gpio_chip *)->get
>> >> only return 0 or 1.
>> >> >
>> >
>> >I read the code of a couple gpio chips and - I may be wrong, but - it seems they
>> >can return an arbitrary errno.
>> >
>> I thought all GPIO chip return 0 or 1 since !!val is returned. I find
>> an example which could return negative value,
>>
>
>Yes, when a function returns `int`, there is a very high chance that the return
>value may be an errno.
>
>
>> >
>> >> >> +
>> >> >> +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_idle;
>> >> >> +
>> >> >> +	while (1) {
>> >> >> +		/*
>> >> >> +		 * re-calculate polling_interval_idle
>> >> >> +		 * so the module parameters polling_interval_idle_ms can be
>> >> >> +		 * changed dynamically through sysfs as polling_interval_active_us
>> >> >> +		 */
>> >> >> +		polling_interval_idle = polling_interval_idle_ms * 1000;
>> >> >> +		if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
>> >> >> +			usleep_range(50000, 100000);
>> >> >> +
>> >> >> +		if (kthread_should_stop())
>> >> >> +			break;
>> >> >> +
>> >> >> +		while (interrupt_line_active(client)) {
>> >> >
>> >> >I realize it's quite unlikely, but can't this be a endless loop if data is coming
>> >> >in at a high enough rate? Maybe the maximum number of iterations could be limited here?
>> >> >
>> >> If we find HID reports are constantly read and send to front-end
>> >> application like libinput, won't it help expose the problem of the I2C
>> >> HiD device?
>> >> >
>> >
>> >I'm not sure I completely understand your point. The reason why I wrote what I wrote
>> >is that this kthread could potentially could go on forever (since `kthread_should_stop()`
>> >is not checked in the inner while loop) if the data is supplied at a high enough rate.
>> >That's why I said, to avoid this problem, only allow a certain number of iterations
>> >for the inner loop, to guarantee that the kthread can stop in any case.
>> >
>> I mean if "data is supplied at a high enough rate" does happen, this is
>> an abnormal case and indicates a bug. So we shouldn't cover it up. We
>> expect the user to report it to us.
>> >
>
>I agree in principle, but if this abnormal case ever occurs, that'll prevent
>this module from being unloaded since `kthread_stop()` will hang because the
>thread is "stuck" in the inner loop, never checking `kthread_should_stop()`.
>That's why I think it makes sense to only allow a certain number of operations
>for the inner loop, and maybe show a warning if that's exceeded:
>
> for (i = 0; i < max_iter && interrupt_line_active(...); i++) {
>    ....
> }
>
> WARN_ON[CE](i == max_iter[, "data is coming in at an unreasonably high rate"]);
>
>or something like this, where `max_iter` could possibly be some value dependent on
>`polling_interval_active_us`, or even just a constant.
>

Thank you for suggesting this approach! It seems it would add a bit of
complexity to detect this situation which could introduce other bugs.

I did a experiment of creating a kthread that will loop forever and found
the rebooting process wasn't stalled. I don't expect user to load&unload
this module. So the end user could not notice this problem so  my rationale
is invalid.

Thus I would be prefer to check `kthread_should_stop()` in the inner
while loop instead.
>
>> >> >> +			i2c_hid_get_input(ihid);
>> >> >> +			usleep_range(polling_interval_active_us,
>> >> >> +				     polling_interval_active_us + 100);
>> >> >> +		}
>> >> >> +
>> >> >> +		usleep_range(polling_interval_idle,
>> >> >> +			     polling_interval_idle + 1000);
>> >> >> +	}
>> >> >> +
>> >> >> +	do_exit(0);
>> >> >> +	return 0;
>> >> >> +}
>> [...]
>> Thank you for offering your understandings on this patch. When I'm going
>> to submit next version, I will add a "Signed-off-by" tag with your name
>> and email, does it look good to you?
>> [...]
>
>I'm not sure if that follows proper procedures.
>
> "The sign-off is a simple line at the end of the explanation for the patch, which
>  certifies that you wrote it or otherwise have the right to pass it on as an
>  open-source patch."[1]
>
>I'm not the author, nor co-author, nor am I going to pass this patch on, so I don't
>think that's appropriate.
>
>Furthermore, please note that
>
> "[...] you may optionally add a Cc: tag to the patch. **This is the only tag which
>  might be added without an explicit action by the person it names** - but it should
>  indicate that this person was copied on the patch."[2]
>  (emphasis mine)
>
You have been directly contributing to this patch because several
improvements of next version are from you. I experienced a similar
situation when submitting patches for QEMU. The only difference is
that the feedbacks on the QEMU patches were also given in the format
of patch. Or do you think a reviewed-by tag from you after you think
the next version is of production quality is a better way to credit
you?
>
>Regards,
>Barnabás Pőcze
>
>
>[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>[2]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

--
Best regards,
Coiby

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Coiby Xu]

On Sun, Oct 18, 2020 at 12:23:14PM +0000, Barnabás Pőcze wrote:
>> [...]
>> > > > > > +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_idle;
>> > > > > > -
>> > > > > > -   while (1) {
>> > > > > > -       /*
>> > > > > >
>> > > > > >
>> > > > > > -        * re-calculate polling_interval_idle
>> > > > > >
>> > > > > >
>> > > > > > -        * so the module parameters polling_interval_idle_ms can be
>> > > > > >
>> > > > > >
>> > > > > > -        * changed dynamically through sysfs as polling_interval_active_us
>> > > > > >
>> > > > > >
>> > > > > > -        */
>> > > > > >
>> > > > > >
>> > > > > > -       polling_interval_idle = polling_interval_idle_ms * 1000;
>> > > > > >
>> > > > > >
>> > > > > > -       if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
>> > > > > >
>> > > > > >
>> > > > > > -       	usleep_range(50000, 100000);
>> > > > > >
>> > > > > >
>> > > > > > -
>> > > > > > -       if (kthread_should_stop())
>> > > > > >
>> > > > > >
>> > > > > > -       	break;
>> > > > > >
>> > > > > >
>> > > > > > -
>> > > > > > -       while (interrupt_line_active(client)) {
>> > > > > >
>> > > > > >
>> > > > >
>> > > > > I realize it's quite unlikely, but can't this be a endless loop if data is coming
>> > > > > in at a high enough rate? Maybe the maximum number of iterations could be limited here?
>> > > >
>> > > > If we find HID reports are constantly read and send to front-end
>> > > > application like libinput, won't it help expose the problem of the I2C
>> > > > HiD device?
>> > > >
>> > > > >
>> > >
>> > > I'm not sure I completely understand your point. The reason why I wrote what I wrote
>> > > is that this kthread could potentially could go on forever (since `kthread_should_stop()`
>> > > is not checked in the inner while loop) if the data is supplied at a high enough rate.
>> > > That's why I said, to avoid this problem, only allow a certain number of iterations
>> > > for the inner loop, to guarantee that the kthread can stop in any case.
>> >
>> > I mean if "data is supplied at a high enough rate" does happen, this is
>> > an abnormal case and indicates a bug. So we shouldn't cover it up. We
>> > expect the user to report it to us.
>> >
>> > >
>>
>> I agree in principle, but if this abnormal case ever occurs, that'll prevent
>> this module from being unloaded since `kthread_stop()` will hang because the
>> thread is "stuck" in the inner loop, never checking `kthread_should_stop()`.
>> That's why I think it makes sense to only allow a certain number of operations
>> for the inner loop, and maybe show a warning if that's exceeded:
>>
>> for (i = 0; i < max_iter && interrupt_line_active(...); i++) {
>> ....
>> }
>>
>> WARN_ON[CE](i == max_iter[, "data is coming in at an unreasonably high rate"]);
>>
>
>I now realize that WARN_ON[CE] is probably not the best fit here, `hid_warn()` is possibly better.
>
Thank you for the suggestion!
>
>> or something like this, where `max_iter` could possibly be some value dependent on
>> `polling_interval_active_us`, or even just a constant.
>> [...]
>
>
>Regards,
>Barnabás Pőcze

--
Best regards,
Coiby

Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Barnabás Pőcze]

> [...]
> You have been directly contributing to this patch because several
> improvements of next version are from you. I experienced a similar
> situation when submitting patches for QEMU. The only difference is
> that the feedbacks on the QEMU patches were also given in the format
> of patch. Or do you think a reviewed-by tag from you after you think
> the next version is of production quality is a better way to credit
> you?

Thanks, but there is no need for any tag from me. I am no maintainer/reviewer of/for
the affected subsystem.

> [...]


Regards,
Barnabás Pőcze