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

[Coiby Xu <coiby.xu@gmail.com>]

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