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
