Date:   Fri, 16 Oct 2020 15:00:49 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     Coiby Xu <coiby.xu@...il.com>
Cc:     "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        Helmut Stult <helmut.stult@...info.de>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

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

Powered by blists - more mailing lists