[PATCH v2] HID: i2c-hid: Always sleep 1ms after I2C_HID_PWR_ON commands

[PATCH v2] HID: i2c-hid: Always sleep 1ms after I2C_HID_PWR_ON commands

[Hans de Goede]

Before this commit i2c_hid_parse() consists of the following steps:

1. Send power on cmd
2. usleep_range(1000, 5000)
3. Send reset cmd
4. Wait for reset to complete (device interrupt, or msleep(100))
5. Send power on cmd
6. Try to read HID descriptor

Notice how there is an usleep_range(1000, 5000) after the first power-on
command, but not after the second power-on command.

Testing has shown that at least on the BMAX Y13 laptop's i2c-hid touchpad,
not having a delay after the second power-on command causes the HID
descriptor to read as all zeros.

In case we hit this on other devices too, the descriptor being all zeros
can be recognized by the following message being logged many, many times:

hid-generic 0018:0911:5288.0002: unknown main item tag 0x0

At the same time as the BMAX Y13's touchpad issue was debugged,
Kai-Heng was working on debugging some issues with Goodix i2c-hid
touchpads. It turns out that these need a delay after a PWR_ON command
too, otherwise they stop working after a suspend/resume cycle.
According to Goodix a delay of minimal 60ms is needed.

Having multiple cases where we need a delay after sending the power-on
command, seems to indicate that we should always sleep after the power-on
command.

This commit fixes the mentioned issues by moving the existing 1ms sleep to
the i2c_hid_set_power() function and changing it to a 60ms sleep.

Cc: stable@vger.kernel.org
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208247
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Reported-and-tested-by: Andrea Borgia <andrea@borgia.bo.it>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add Kai-Heng's case, with Goodix touchpads needing a delay after PWR_ON too,
  to the commit message
- Add a Reported-by tag for Kai-Heng
- Increase the delay to 60ms
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 294c84e136d7..dbd04492825d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -420,6 +420,19 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
 		dev_err(&client->dev, "failed to change power setting.\n");
 
 set_pwr_exit:
+
+	/*
+	 * The HID over I2C specification states that if a DEVICE needs time
+	 * after the PWR_ON request, it should utilise CLOCK stretching.
+	 * However, it has been observered that the Windows driver provides a
+	 * 1ms sleep between the PWR_ON and RESET requests.
+	 * According to Goodix Windows even waits 60 ms after (other?)
+	 * PWR_ON requests. Testing has confirmed that several devices
+	 * will not work properly without a delay after a PWR_ON request.
+	 */
+	if (!ret && power_state == I2C_HID_PWR_ON)
+		msleep(60);
+
 	return ret;
 }
 
@@ -441,15 +454,6 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 	if (ret)
 		goto out_unlock;
 
-	/*
-	 * The HID over I2C specification states that if a DEVICE needs time
-	 * after the PWR_ON request, it should utilise CLOCK stretching.
-	 * However, it has been observered that the Windows driver provides a
-	 * 1ms sleep between the PWR_ON and RESET requests and that some devices
-	 * rely on this.
-	 */
-	usleep_range(1000, 5000);
-
 	i2c_hid_dbg(ihid, "resetting...\n");
 
 	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
-- 
2.28.0

Re: [PATCH v2] HID: i2c-hid: Always sleep 1ms after I2C_HID_PWR_ON commands

[Kai-Heng Feng]

Hi Hans,

> On Aug 11, 2020, at 20:59, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> Before this commit i2c_hid_parse() consists of the following steps:
> 
> 1. Send power on cmd
> 2. usleep_range(1000, 5000)
> 3. Send reset cmd
> 4. Wait for reset to complete (device interrupt, or msleep(100))
> 5. Send power on cmd
> 6. Try to read HID descriptor
> 
> Notice how there is an usleep_range(1000, 5000) after the first power-on
> command, but not after the second power-on command.
> 
> Testing has shown that at least on the BMAX Y13 laptop's i2c-hid touchpad,
> not having a delay after the second power-on command causes the HID
> descriptor to read as all zeros.
> 
> In case we hit this on other devices too, the descriptor being all zeros
> can be recognized by the following message being logged many, many times:
> 
> hid-generic 0018:0911:5288.0002: unknown main item tag 0x0
> 
> At the same time as the BMAX Y13's touchpad issue was debugged,
> Kai-Heng was working on debugging some issues with Goodix i2c-hid
> touchpads. It turns out that these need a delay after a PWR_ON command
> too, otherwise they stop working after a suspend/resume cycle.
> According to Goodix a delay of minimal 60ms is needed.
> 
> Having multiple cases where we need a delay after sending the power-on
> command, seems to indicate that we should always sleep after the power-on
> command.
> 
> This commit fixes the mentioned issues by moving the existing 1ms sleep to
> the i2c_hid_set_power() function and changing it to a 60ms sleep.

Thanks for your patch.

The subject is still "1ms" instead of "60ms".

Kai-Heng

> 
> Cc: stable@vger.kernel.org
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208247
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Reported-and-tested-by: Andrea Borgia <andrea@borgia.bo.it>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Add Kai-Heng's case, with Goodix touchpads needing a delay after PWR_ON too,
>  to the commit message
> - Add a Reported-by tag for Kai-Heng
> - Increase the delay to 60ms
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 294c84e136d7..dbd04492825d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -420,6 +420,19 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
> 		dev_err(&client->dev, "failed to change power setting.\n");
> 
> set_pwr_exit:
> +
> +	/*
> +	 * The HID over I2C specification states that if a DEVICE needs time
> +	 * after the PWR_ON request, it should utilise CLOCK stretching.
> +	 * However, it has been observered that the Windows driver provides a
> +	 * 1ms sleep between the PWR_ON and RESET requests.
> +	 * According to Goodix Windows even waits 60 ms after (other?)
> +	 * PWR_ON requests. Testing has confirmed that several devices
> +	 * will not work properly without a delay after a PWR_ON request.
> +	 */
> +	if (!ret && power_state == I2C_HID_PWR_ON)
> +		msleep(60);
> +
> 	return ret;
> }
> 
> @@ -441,15 +454,6 @@ static int i2c_hid_hwreset(struct i2c_client *client)
> 	if (ret)
> 		goto out_unlock;
> 
> -	/*
> -	 * The HID over I2C specification states that if a DEVICE needs time
> -	 * after the PWR_ON request, it should utilise CLOCK stretching.
> -	 * However, it has been observered that the Windows driver provides a
> -	 * 1ms sleep between the PWR_ON and RESET requests and that some devices
> -	 * rely on this.
> -	 */
> -	usleep_range(1000, 5000);
> -
> 	i2c_hid_dbg(ihid, "resetting...\n");
> 
> 	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
> -- 
> 2.28.0
> 

Re: [PATCH v2] HID: i2c-hid: Always sleep 1ms after I2C_HID_PWR_ON commands

[Hans de Goede]

Hi,

On 8/11/20 3:37 PM, Kai-Heng Feng wrote:
> Hi Hans,
> 
>> On Aug 11, 2020, at 20:59, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Before this commit i2c_hid_parse() consists of the following steps:
>>
>> 1. Send power on cmd
>> 2. usleep_range(1000, 5000)
>> 3. Send reset cmd
>> 4. Wait for reset to complete (device interrupt, or msleep(100))
>> 5. Send power on cmd
>> 6. Try to read HID descriptor
>>
>> Notice how there is an usleep_range(1000, 5000) after the first power-on
>> command, but not after the second power-on command.
>>
>> Testing has shown that at least on the BMAX Y13 laptop's i2c-hid touchpad,
>> not having a delay after the second power-on command causes the HID
>> descriptor to read as all zeros.
>>
>> In case we hit this on other devices too, the descriptor being all zeros
>> can be recognized by the following message being logged many, many times:
>>
>> hid-generic 0018:0911:5288.0002: unknown main item tag 0x0
>>
>> At the same time as the BMAX Y13's touchpad issue was debugged,
>> Kai-Heng was working on debugging some issues with Goodix i2c-hid
>> touchpads. It turns out that these need a delay after a PWR_ON command
>> too, otherwise they stop working after a suspend/resume cycle.
>> According to Goodix a delay of minimal 60ms is needed.
>>
>> Having multiple cases where we need a delay after sending the power-on
>> command, seems to indicate that we should always sleep after the power-on
>> command.
>>
>> This commit fixes the mentioned issues by moving the existing 1ms sleep to
>> the i2c_hid_set_power() function and changing it to a 60ms sleep.
> 
> Thanks for your patch.
> 
> The subject is still "1ms" instead of "60ms".

Oops, v3 with this fixed coming up.

Regards,

Hans




>> Cc: stable@vger.kernel.org
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208247
>> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> Reported-and-tested-by: Andrea Borgia <andrea@borgia.bo.it>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Add Kai-Heng's case, with Goodix touchpads needing a delay after PWR_ON too,
>>   to the commit message
>> - Add a Reported-by tag for Kai-Heng
>> - Increase the delay to 60ms
>> ---
>> drivers/hid/i2c-hid/i2c-hid-core.c | 22 +++++++++++++---------
>> 1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index 294c84e136d7..dbd04492825d 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -420,6 +420,19 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
>> 		dev_err(&client->dev, "failed to change power setting.\n");
>>
>> set_pwr_exit:
>> +
>> +	/*
>> +	 * The HID over I2C specification states that if a DEVICE needs time
>> +	 * after the PWR_ON request, it should utilise CLOCK stretching.
>> +	 * However, it has been observered that the Windows driver provides a
>> +	 * 1ms sleep between the PWR_ON and RESET requests.
>> +	 * According to Goodix Windows even waits 60 ms after (other?)
>> +	 * PWR_ON requests. Testing has confirmed that several devices
>> +	 * will not work properly without a delay after a PWR_ON request.
>> +	 */
>> +	if (!ret && power_state == I2C_HID_PWR_ON)
>> +		msleep(60);
>> +
>> 	return ret;
>> }
>>
>> @@ -441,15 +454,6 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>> 	if (ret)
>> 		goto out_unlock;
>>
>> -	/*
>> -	 * The HID over I2C specification states that if a DEVICE needs time
>> -	 * after the PWR_ON request, it should utilise CLOCK stretching.
>> -	 * However, it has been observered that the Windows driver provides a
>> -	 * 1ms sleep between the PWR_ON and RESET requests and that some devices
>> -	 * rely on this.
>> -	 */
>> -	usleep_range(1000, 5000);
>> -
>> 	i2c_hid_dbg(ihid, "resetting...\n");
>>
>> 	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
>> -- 
>> 2.28.0
>>
> 

Re: [PATCH v2] HID: i2c-hid: Always sleep 1ms after I2C_HID_PWR_ON commands

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.8.1, v5.7.15, v5.4.58, v4.19.139, v4.14.193, v4.9.232, v4.4.232.

v5.8.1: Build OK!
v5.7.15: Build OK!
v5.4.58: Build OK!
v4.19.139: Build OK!
v4.14.193: Build OK!
v4.9.232: Build OK!
v4.4.232: Failed to apply! Possible dependencies:
    71af01a8c85a ("HID: i2c-hid: add a simple quirk to fix device defects")
    9a327405014f ("HID: i2c-hid: Prevent sending reports from racing with device reset")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v2] HID: i2c-hid: Always sleep 1ms after I2C_HID_PWR_ON commands

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.8.2, v5.7.16, v5.4.59, v4.19.140, v4.14.193, v4.9.232, v4.4.232.

v5.8.2: Build OK!
v5.7.16: Build OK!
v5.4.59: Build OK!
v4.19.140: Build OK!
v4.14.193: Build OK!
v4.9.232: Build OK!
v4.4.232: Failed to apply! Possible dependencies:
    71af01a8c85a ("HID: i2c-hid: add a simple quirk to fix device defects")
    9a327405014f ("HID: i2c-hid: Prevent sending reports from racing with device reset")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha