linux-kernel - Re: [PATCH] HID: i2c-hid: Add quirk for sleep before reset

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: <20170105112319.GA17928@mail.corp.redhat.com>
Date:   Thu, 5 Jan 2017 12:23:19 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Brendan McGrath <redmcg@...mandi.dyndns.org>
Cc:     Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Victor Vlasenko <victor.vlasenko@...gears.com>,
        Frederik Wenigwieser <frederik.wenigwieser@...il.com>
Subject: Re: [PATCH] HID: i2c-hid: Add quirk for sleep before reset

On Jan 05 2017 or thereabouts, Brendan McGrath wrote:
> Support for the Asus Touchpad was recently added. It turns out this
> device can fail initialisation (and become unusable) when the RESET
> command is sent too soon after the POWER ON command.
> 
> Unfortunately the i2c-hid specification does not specify the need for
> a delay between these two commands. But it was discovered the Windows
> driver has a 1ms delay.
> 
> As a result, this patch adds a new QUIRK to the i2c-hid module that
> allows a device to specify a specific sleep inbetween the POWER ON and
> RESET commands.
> 
> See https://github.com/vlasenko/hid-asus-dkms/issues/24 for further
> details.
> 
> Signed-off-by: Brendan McGrath <redmcg@...mandi.dyndns.org>
> ---
> I considered three approaches for this patch:
> a) add a hardcoded sleep that would affect all devices;
> b) add a quirk with a hardcoded sleep value; or
> c) add a quirk with a configurable sleep value
> 
> Each was a trade off between flexibility and the amount of code/complexity required.

I am not a big fan of having to configure the sleep value in each quirk.
I think I'd actually prefer the solution a: no quirk and a small wait
(you wait less than 5ms, I would think this as acceptable). I wouldn't
be surprised if other devices would require such timing issues, so I
wouldn't mind waiting a tiny bit for those to be working without any
quirks.

If anyone prefer not having those timeouts, we can add some quirks, but
I would then prefer solution b.

Cheers,
Benjamin

> 
> In the end I chose c) as this allowed for the most flexibility; but would
> be happy to resubmit the patch using one of the other options (or any other
> alternative).
> 
>  drivers/hid/i2c-hid/i2c-hid.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 8d53efe..fb1ebfa 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -45,6 +45,7 @@
>  
>  /* quirks to control the device */
>  #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
> +#define I2C_HID_QUIRK_SLEEP_BEFORE_RESET	BIT(1)
>  
>  /* flags */
>  #define I2C_HID_STARTED		0
> @@ -156,17 +157,26 @@ struct i2c_hid {
>  
>  	bool			irq_wake_enabled;
>  	struct mutex		reset_lock;
> +
> +	__u32			reset_usleep_low;
> +	__u32			reset_usleep_high;
>  };
>  
>  static const struct i2c_hid_quirks {
>  	__u16 idVendor;
>  	__u16 idProduct;
>  	__u32 quirks;
> +	__u32 reset_usleep_low;
> +	__u32 reset_usleep_high;
>  } i2c_hid_quirks[] = {
>  	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8752,
>  		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
>  	{ USB_VENDOR_ID_WEIDA, USB_DEVICE_ID_WEIDA_8755,
>  		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
> +	{ USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_TOUCHPAD,
> +		I2C_HID_QUIRK_SLEEP_BEFORE_RESET,
> +		 .reset_usleep_low = 750,
> +		 .reset_usleep_high = 5000 },
>  	{ 0, 0 }
>  };
>  
> @@ -177,16 +187,16 @@ static const struct i2c_hid_quirks {
>   *
>   * Returns: a u32 quirks value.
>   */
> -static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
> +static const struct i2c_hid_quirks* i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
>  {
> -	u32 quirks = 0;
> +	const struct i2c_hid_quirks *quirks = NULL;
>  	int n;
>  
>  	for (n = 0; i2c_hid_quirks[n].idVendor; n++)
>  		if (i2c_hid_quirks[n].idVendor == idVendor &&
>  		    (i2c_hid_quirks[n].idProduct == (__u16)HID_ANY_ID ||
>  		     i2c_hid_quirks[n].idProduct == idProduct))
> -			quirks = i2c_hid_quirks[n].quirks;
> +			quirks = &i2c_hid_quirks[n];
>  
>  	return quirks;
>  }
> @@ -425,6 +435,9 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  	if (ret)
>  		goto out_unlock;
>  
> +	if (ihid->quirks & I2C_HID_QUIRK_SLEEP_BEFORE_RESET)
> +		usleep_range(ihid->reset_usleep_low, ihid->reset_usleep_high);
> +
>  	i2c_hid_dbg(ihid, "resetting...\n");
>  
>  	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> @@ -1005,6 +1018,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>  	struct i2c_hid *ihid;
>  	struct hid_device *hid;
>  	__u16 hidRegister;
> +	const struct i2c_hid_quirks *quirks;
>  	struct i2c_hid_platform_data *platform_data = client->dev.platform_data;
>  
>  	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
> @@ -1091,7 +1105,15 @@ static int i2c_hid_probe(struct i2c_client *client,
>  		 client->name, hid->vendor, hid->product);
>  	strlcpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
>  
> -	ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
> +	quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
> +
> +	if (quirks)
> +		ihid->quirks = quirks->quirks;
> +
> +	if (ihid->quirks & I2C_HID_QUIRK_SLEEP_BEFORE_RESET) {
> +		ihid->reset_usleep_low = quirks->reset_usleep_low;
> +		ihid->reset_usleep_high = quirks->reset_usleep_high;
> +	}
>  
>  	ret = hid_add_device(hid);
>  	if (ret) {
> -- 
> 2.7.4
>