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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X75zL12q+FF6KBHi@kroah.com>
Date:   Wed, 25 Nov 2020 16:07:27 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Coiby Xu <coiby.xu@...il.com>
Cc:     linux-input@...r.kernel.org,
        Helmut Stult <helmut.stult@...info.de>,
        Barnabás Pőcze <pobrn@...tonmail.com>,
        Baq Domalaq <domalak@...il.com>,
        Pedro Ribeiro <pedrib@...il.com>, stable@...r.kernel.org,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] HID: i2c-hid: add polling mode based on connected
 GPIO chip's pin status

On Wed, Nov 25, 2020 at 10:10:22PM +0800, Coiby Xu wrote:
> 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.
> 
> This patch could fix touchpads of Lenovo AMD gaming laptops including
> Legion-5 15ARH05 (R7000), Legion-5P (R7000P) and IdeaPad Gaming 3
> 15ARH05.
> 
> 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}.
> 
> Note xf86-input-synaptics doesn't work well with this polling mode
> for the Synaptics touchpad. The Synaptics touchpad would often locks
> into scroll mode when using multitouch gestures [1]. One remedy is to
> decrease the polling interval.
> 
> Thanks to Barnabás's thorough review of this patch and the useful
> feedback!
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190/comments/235
> 
> Cc: <stable@...r.kernel.org>
> Cc: Barnabás Pőcze <pobrn@...tonmail.com>
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
> Signed-off-by: Coiby Xu <coiby.xu@...il.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 152 +++++++++++++++++++++++++++--
>  1 file changed, 142 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index aeff1ffb0c8b..f25503f31ccf 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,25 @@
>  #define I2C_HID_PWR_ON		0x00
>  #define I2C_HID_PWR_SLEEP	0x01
>  
> +/* polling mode */
> +#define I2C_HID_POLLING_DISABLED 0
> +#define I2C_HID_POLLING_GPIO_PIN 1
> +#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000
> +#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10
> +
> +static u8 polling_mode;
> +module_param(polling_mode, byte, 0444);
> +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status");

Module parameters are for the 1990's, they are global and horrible to
try to work with.  You should provide something on a per-device basis,
as what happens if your system requires different things here for
different devices?  You set this for all devices :(

thanks,

greg k-h