[PATCH 0/7] HID: i2c-hid: Rework wait for reset to match Windows

[PATCH 0/7] HID: i2c-hid: Rework wait for reset to match Windows

[Hans de Goede]

Hi All,

Looking at:

"2247751 - Touchpad not working on Lenovo Thinkbook 15 Gen 5"
https://bugzilla.redhat.com/show_bug.cgi?id=2247751

I thought at first that this was another touchpad needing
a I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk, further testing
has shown this seems to be an IRQ not working issue though,
so this series does not help for that bug.

The bug has made me revisit I2C_HID_QUIRK_NO_IRQ_AFTER_RESET though and it
made me look at Microsoft's i2c-hid docs again and I noticed the following:

"""
4. Issue a RESET (Host Initiated Reset) to the Device.
5. Retrieve report descriptor from the device.

Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C.
Since report descriptors are (a) static and (b) quite long, Windows 8 may
issue a request for 5 while it is waiting for a response from the device
on 4.
"""

which made me think that maybe on some touchpads the reset ack is delayed
till after the report descriptor is read ?

Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad,
for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced,
shows that about 1 ms after the report descriptor read finishes the reset
indeed does get acked.

So this series refactors the reset handling to move the waiting for
the ack to after reading the report-descriptor, so that
the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk is no longer necessary.

Julian, I've added you to the Cc because you developed the code
for touchpads where the HID report descriptors are missing and are
provided by the kernel through a DMI quirk. The behavior for these
touchpads should be unchanged, but if you can test on a laptop
with such a touchpad that would be great.

Regards,

Hans


Hans de Goede (7):
  HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()
  HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish()
    functions
  HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling
  HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the
    report-descriptor
  HID: i2c-hid: Remove I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks
  HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
  HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines

 drivers/hid/i2c-hid/i2c-hid-core.c | 155 +++++++++++++++--------------
 1 file changed, 79 insertions(+), 76 deletions(-)

-- 
2.41.0

[PATCH 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()

[Hans de Goede]

i2c_hid_hwreset() is the only caller of i2c_hid_execute_reset(),
fold the latter into the former.

This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.

No functional changes intended.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 80 +++++++++++++-----------------
 1 file changed, 34 insertions(+), 46 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2735cd585af0..12a9edb23f82 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -426,49 +426,9 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	return ret;
 }
 
-static int i2c_hid_execute_reset(struct i2c_hid *ihid)
-{
-	size_t length = 0;
-	int ret;
-
-	i2c_hid_dbg(ihid, "resetting...\n");
-
-	/* Prepare reset command. Command register goes first. */
-	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
-	length += sizeof(__le16);
-	/* Next is RESET command itself */
-	length += i2c_hid_encode_command(ihid->cmdbuf + length,
-					 I2C_HID_OPCODE_RESET, 0, 0);
-
-	set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-
-	ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
-	if (ret) {
-		dev_err(&ihid->client->dev, "failed to reset device.\n");
-		goto out;
-	}
-
-	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
-		msleep(100);
-		goto out;
-	}
-
-	i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
-	if (!wait_event_timeout(ihid->wait,
-				!test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
-				msecs_to_jiffies(5000))) {
-		ret = -ENODATA;
-		goto out;
-	}
-	i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
-
-out:
-	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-	return ret;
-}
-
 static int i2c_hid_hwreset(struct i2c_hid *ihid)
 {
+	size_t length = 0;
 	int ret;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
@@ -482,21 +442,49 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 
 	ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 	if (ret)
-		goto out_unlock;
+		goto err_unlock;
 
-	ret = i2c_hid_execute_reset(ihid);
+	/* Prepare reset command. Command register goes first. */
+	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+	length += sizeof(__le16);
+	/* Next is RESET command itself */
+	length += i2c_hid_encode_command(ihid->cmdbuf + length,
+					 I2C_HID_OPCODE_RESET, 0, 0);
+
+	set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+
+	ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
 	if (ret) {
 		dev_err(&ihid->client->dev,
 			"failed to reset device: %d\n", ret);
-		i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
-		goto out_unlock;
+		goto err_clear_reset;
 	}
 
+	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
+		msleep(100);
+		clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	}
+
+	i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
+	if (!wait_event_timeout(ihid->wait,
+				!test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
+				msecs_to_jiffies(5000))) {
+		ret = -ENODATA;
+		goto err_clear_reset;
+	}
+	i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
+
 	/* At least some SIS devices need this after reset */
 	if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET))
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 
-out_unlock:
+	mutex_unlock(&ihid->reset_lock);
+	return ret;
+
+err_clear_reset:
+	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+err_unlock:
 	mutex_unlock(&ihid->reset_lock);
 	return ret;
 }
-- 
2.41.0

Re: [PATCH 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -482,21 +442,49 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>
>         ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
>         if (ret)
> -               goto out_unlock;
> +               goto err_unlock;
>
> -       ret = i2c_hid_execute_reset(ihid);
> +       /* Prepare reset command. Command register goes first. */
> +       *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> +       length += sizeof(__le16);
> +       /* Next is RESET command itself */
> +       length += i2c_hid_encode_command(ihid->cmdbuf + length,
> +                                        I2C_HID_OPCODE_RESET, 0, 0);
> +
> +       set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +
> +       ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
>         if (ret) {
>                 dev_err(&ihid->client->dev,
>                         "failed to reset device: %d\n", ret);
> -               i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
> -               goto out_unlock;
> +               goto err_clear_reset;
>         }
>
> +       if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
> +               msleep(100);
> +               clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +       }
> +
> +       i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
> +       if (!wait_event_timeout(ihid->wait,
> +                               !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
> +                               msecs_to_jiffies(5000))) {
> +               ret = -ENODATA;
> +               goto err_clear_reset;
> +       }
> +       i2c_hid_dbg(ihid, "%s: finished.\n", __func__);

super nitty, but I wonder if your i2c_hid_dbg() message saying
"waiting" should move above the check for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET. Then you'll have a message printed
before your msleep. I guess technically you could then add an "else
if" for the second "if" statement which would make it more obvious to
the reader that the "wait_event_timeout" won't happen when the quirk
is present.

Above is just a nit, so:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()

[Hans de Goede]

Hi Douglas,

Thank you for the reviews.

On 11/6/23 19:50, Doug Anderson wrote:
> Hi,
> 
> On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> @@ -482,21 +442,49 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>>
>>         ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
>>         if (ret)
>> -               goto out_unlock;
>> +               goto err_unlock;
>>
>> -       ret = i2c_hid_execute_reset(ihid);
>> +       /* Prepare reset command. Command register goes first. */
>> +       *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
>> +       length += sizeof(__le16);
>> +       /* Next is RESET command itself */
>> +       length += i2c_hid_encode_command(ihid->cmdbuf + length,
>> +                                        I2C_HID_OPCODE_RESET, 0, 0);
>> +
>> +       set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>> +
>> +       ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
>>         if (ret) {
>>                 dev_err(&ihid->client->dev,
>>                         "failed to reset device: %d\n", ret);
>> -               i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
>> -               goto out_unlock;
>> +               goto err_clear_reset;
>>         }
>>
>> +       if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
>> +               msleep(100);
>> +               clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>> +       }
>> +
>> +       i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
>> +       if (!wait_event_timeout(ihid->wait,
>> +                               !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
>> +                               msecs_to_jiffies(5000))) {
>> +               ret = -ENODATA;
>> +               goto err_clear_reset;
>> +       }
>> +       i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
> 
> super nitty, but I wonder if your i2c_hid_dbg() message saying
> "waiting" should move above the check for
> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET. Then you'll have a message printed
> before your msleep. I guess technically you could then add an "else
> if" for the second "if" statement which would make it more obvious to
> the reader that the "wait_event_timeout" won't happen when the quirk
> is present.
> 
> Above is just a nit

I agree with you that it would be better to move the
mutex_[un]lock(&ihid->reset_lock) calls out of
i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() and into their
callers. I'm preparing a v2 with these changes now and I'll also
move the

       i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);

line to above the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET test for v2.

Regards,

Hans

[PATCH 2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions

[Hans de Goede]

Split i2c_hid_hwreset() into:

i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and
i2c_hid_finish_hwreset() which actually waits for the reset to complete.

This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.

No functional changes intended.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 12a9edb23f82..8105b84fb539 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -426,7 +426,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	return ret;
 }
 
-static int i2c_hid_hwreset(struct i2c_hid *ihid)
+static int i2c_hid_start_hwreset(struct i2c_hid *ihid)
 {
 	size_t length = 0;
 	int ret;
@@ -460,6 +460,20 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 		goto err_clear_reset;
 	}
 
+	return 0;
+
+err_clear_reset:
+	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+err_unlock:
+	mutex_unlock(&ihid->reset_lock);
+	return ret;
+}
+
+static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
+{
+	int ret = 0;
+
 	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
 		msleep(100);
 		clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
@@ -484,7 +498,6 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 err_clear_reset:
 	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
 	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
-err_unlock:
 	mutex_unlock(&ihid->reset_lock);
 	return ret;
 }
@@ -732,7 +745,9 @@ static int i2c_hid_parse(struct hid_device *hid)
 	}
 
 	do {
-		ret = i2c_hid_hwreset(ihid);
+		ret = i2c_hid_start_hwreset(ihid);
+		if (ret == 0)
+			ret = i2c_hid_finish_hwreset(ihid);
 		if (ret)
 			msleep(1000);
 	} while (tries-- > 0 && ret);
@@ -975,10 +990,13 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
 	 * However some ALPS touchpads generate IRQ storm without reset, so
 	 * let's still reset them here.
 	 */
-	if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
-		ret = i2c_hid_hwreset(ihid);
-	else
+	if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) {
+		ret = i2c_hid_start_hwreset(ihid);
+		if (ret == 0)
+			ret = i2c_hid_finish_hwreset(ihid);
+	} else {
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
+	}
 
 	if (ret)
 		return ret;
-- 
2.41.0

Re: [PATCH 2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -460,6 +460,20 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>                 goto err_clear_reset;
>         }
>
> +       return 0;

The mutex "contract" between i2c_hid_start_hwreset() and
i2c_hid_finish_hwreset() is non-obvious and, IMO, deserves a comment.
Specifically i2c_hid_start_hwreset() will grab and leave the mutex
locked if it returns 0. Then i2c_hid_finish_hwreset() expects the
mutex pre-locked and will always release it.

While reviewing later patches, I actually realized that _I think_
things would be cleaner by moving the mutex lock/unlock to the
callers. Maybe take a look at how the code looks with that?

> @@ -732,7 +745,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>         }
>
>         do {
> -               ret = i2c_hid_hwreset(ihid);
> +               ret = i2c_hid_start_hwreset(ihid);
> +               if (ret == 0)
> +                       ret = i2c_hid_finish_hwreset(ihid);
>                 if (ret)
>                         msleep(1000);

nit: it's slightly weird that two "if" tests next to each other use
different style. One compares against 0 and the other just implicitly
treats an int as a bool. I'm fine with either way, but it's nice to
keep the style the same within any given function (even better if it
can be the same throughout the driver).


> @@ -975,10 +990,13 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
>          * However some ALPS touchpads generate IRQ storm without reset, so
>          * let's still reset them here.
>          */
> -       if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
> -               ret = i2c_hid_hwreset(ihid);
> -       else
> +       if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) {
> +               ret = i2c_hid_start_hwreset(ihid);
> +               if (ret == 0)
> +                       ret = i2c_hid_finish_hwreset(ihid);

nit: Above is also a "if (ret == 0)" vs below "if (ret)"...


> +       } else {
>                 ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
> +       }
>
>         if (ret)
>                 return ret;

The above is mostly nits. I'd be OK with:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions

[Hans de Goede]

Hi,

On 11/6/23 19:53, Doug Anderson wrote:
> Hi,
> 
> On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> @@ -460,6 +460,20 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>>                 goto err_clear_reset;
>>         }
>>
>> +       return 0;
> 
> The mutex "contract" between i2c_hid_start_hwreset() and
> i2c_hid_finish_hwreset() is non-obvious and, IMO, deserves a comment.
> Specifically i2c_hid_start_hwreset() will grab and leave the mutex
> locked if it returns 0. Then i2c_hid_finish_hwreset() expects the
> mutex pre-locked and will always release it.
> 
> While reviewing later patches, I actually realized that _I think_
> things would be cleaner by moving the mutex lock/unlock to the
> callers. Maybe take a look at how the code looks with that?

I agree that moving the mutex to the callers would be better,
I've just completed this change for v2 of the series.


>> @@ -732,7 +745,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>>         }
>>
>>         do {
>> -               ret = i2c_hid_hwreset(ihid);
>> +               ret = i2c_hid_start_hwreset(ihid);
>> +               if (ret == 0)
>> +                       ret = i2c_hid_finish_hwreset(ihid);
>>                 if (ret)
>>                         msleep(1000);
> 
> nit: it's slightly weird that two "if" tests next to each other use
> different style. One compares against 0 and the other just implicitly
> treats an int as a bool. I'm fine with either way, but it's nice to
> keep the style the same within any given function (even better if it
> can be the same throughout the driver).

One of the 2 tests goes away later, so I've kept this as is for v2.

Regards,

Hans

[PATCH 3/7] HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling

[Hans de Goede]

Switch i2c_hid_parse() to goto style error handling.

This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.

Note this changes the descriptor read error path to propagate
the actual i2c_hid_read_register() error code (which is always
negative) instead of hardcoding a -EIO return.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 8105b84fb539..f029ddce4766 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -776,23 +776,21 @@ static int i2c_hid_parse(struct hid_device *hid)
 					    rdesc, rsize);
 		if (ret) {
 			hid_err(hid, "reading report descriptor failed\n");
-			kfree(rdesc);
-			return -EIO;
+			goto out;
 		}
 	}
 
 	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
 
 	ret = hid_parse_report(hid, rdesc, rsize);
+	if (ret)
+		dbg_hid("parsing report descriptor failed\n");
+
+out:
 	if (!use_override)
 		kfree(rdesc);
 
-	if (ret) {
-		dbg_hid("parsing report descriptor failed\n");
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 static int i2c_hid_start(struct hid_device *hid)
-- 
2.41.0

Re: [PATCH 3/7] HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Switch i2c_hid_parse() to goto style error handling.
>
> This is a preparation patch for removing the need for
> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
> more like Windows.
>
> Note this changes the descriptor read error path to propagate
> the actual i2c_hid_read_register() error code (which is always
> negative) instead of hardcoding a -EIO return.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor

[Hans de Goede]

A recent bug made me look at Microsoft's i2c-hid docs again
and I noticed the following:

"""
4. Issue a RESET (Host Initiated Reset) to the Device.
5. Retrieve report descriptor from the device.

Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C.
Since report descriptors are (a) static and (b) quite long, Windows 8 may
issue a request for 5 while it is waiting for a response from the device
on 4.
"""

Which made me think that maybe on some touchpads the reset ack is delayed
till after the report descriptor is read ?

Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad,
for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced,
shows that about 1 ms after the report descriptor read finishes the reset
indeed does get acked.

Move the waiting for the ack to after reading the report-descriptor,
so that the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk is no longer necessary
(on this model).

While at it drop the dbg_hid() for a malloc failure, malloc failures
already get logged extensively by malloc itself.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2247751
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index f029ddce4766..3bd0c3d77d99 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -502,6 +502,12 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
 	return ret;
 }
 
+static void i2c_hid_abort_hwreset(struct i2c_hid *ihid)
+{
+	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	mutex_unlock(&ihid->reset_lock);
+}
+
 static void i2c_hid_get_input(struct i2c_hid *ihid)
 {
 	u16 size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
@@ -746,8 +752,6 @@ static int i2c_hid_parse(struct hid_device *hid)
 
 	do {
 		ret = i2c_hid_start_hwreset(ihid);
-		if (ret == 0)
-			ret = i2c_hid_finish_hwreset(ihid);
 		if (ret)
 			msleep(1000);
 	} while (tries-- > 0 && ret);
@@ -763,9 +767,8 @@ static int i2c_hid_parse(struct hid_device *hid)
 		i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
 	} else {
 		rdesc = kzalloc(rsize, GFP_KERNEL);
-
 		if (!rdesc) {
-			dbg_hid("couldn't allocate rdesc memory\n");
+			i2c_hid_abort_hwreset(ihid);
 			return -ENOMEM;
 		}
 
@@ -776,10 +779,21 @@ static int i2c_hid_parse(struct hid_device *hid)
 					    rdesc, rsize);
 		if (ret) {
 			hid_err(hid, "reading report descriptor failed\n");
+			i2c_hid_abort_hwreset(ihid);
 			goto out;
 		}
 	}
 
+	/*
+	 * Windows directly reads the report-descriptor after sending reset
+	 * and then waits for resets completion afterwards. Some touchpads
+	 * actually wait for the report-descriptor to be read before signalling
+	 * reset completion.
+	 */
+	ret = i2c_hid_finish_hwreset(ihid);
+	if (ret)
+		goto out;
+
 	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
 
 	ret = hid_parse_report(hid, rdesc, rsize);
-- 
2.41.0

Re: [PATCH 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -746,8 +752,6 @@ static int i2c_hid_parse(struct hid_device *hid)
>
>         do {
>                 ret = i2c_hid_start_hwreset(ihid);
> -               if (ret == 0)
> -                       ret = i2c_hid_finish_hwreset(ihid);

The mutex contract (talked about in a previous patch) is a little more
confusing now. ;-) Feels like it needs a comment somewhere in here so
the reader of the code understands that the reset_mutex might or might
not be locked here.

...or would it make more sense for the caller to just handle the mutex
to make it more obvious. The "I2C_HID_QUIRK_RESET_ON_RESUME" code
would need to grab the mutex too, but that really doesn't seem
terrible. In fact, I suspect it cleans up your error handling and
makes everything cleaner?


>                 if (ret)
>                         msleep(1000);
>         } while (tries-- > 0 && ret);
> @@ -763,9 +767,8 @@ static int i2c_hid_parse(struct hid_device *hid)
>                 i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
>         } else {
>                 rdesc = kzalloc(rsize, GFP_KERNEL);
> -
>                 if (!rdesc) {
> -                       dbg_hid("couldn't allocate rdesc memory\n");
> +                       i2c_hid_abort_hwreset(ihid);
>                         return -ENOMEM;
>                 }
>
> @@ -776,10 +779,21 @@ static int i2c_hid_parse(struct hid_device *hid)
>                                             rdesc, rsize);
>                 if (ret) {
>                         hid_err(hid, "reading report descriptor failed\n");
> +                       i2c_hid_abort_hwreset(ihid);
>                         goto out;
>                 }
>         }
>
> +       /*
> +        * Windows directly reads the report-descriptor after sending reset
> +        * and then waits for resets completion afterwards. Some touchpads
> +        * actually wait for the report-descriptor to be read before signalling
> +        * reset completion.
> +        */
> +       ret = i2c_hid_finish_hwreset(ihid);
> +       if (ret)
> +               goto out;

Given your new understanding, I wonder if you should start reading the
report descriptor even if "use_override" is set? You'd throw away what
you read but maybe it would be important to make the touchscreen
properly de-assert reset? I guess this is the subject of the next
patch...

Also: I guess there's the assumption that the touchscreens needing the
other caller of the reset functions (I2C_HID_QUIRK_RESET_ON_RESUME)
never need to read the report like this?

-Doug

Re: [PATCH 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor

[Hans de Goede]

Hi,

On 11/6/23 19:53, Doug Anderson wrote:
> Hi,
> 
> On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> @@ -746,8 +752,6 @@ static int i2c_hid_parse(struct hid_device *hid)
>>
>>         do {
>>                 ret = i2c_hid_start_hwreset(ihid);
>> -               if (ret == 0)
>> -                       ret = i2c_hid_finish_hwreset(ihid);
> 
> The mutex contract (talked about in a previous patch) is a little more
> confusing now. ;-) Feels like it needs a comment somewhere in here so
> the reader of the code understands that the reset_mutex might or might
> not be locked here.
> 
> ...or would it make more sense for the caller to just handle the mutex
> to make it more obvious. The "I2C_HID_QUIRK_RESET_ON_RESUME" code
> would need to grab the mutex too, but that really doesn't seem
> terrible. In fact, I suspect it cleans up your error handling and
> makes everything cleaner?

I agree that moving the mutex to the callers would be better,
I've just completed this change for v2 of the series.

>>                 if (ret)
>>                         msleep(1000);
>>         } while (tries-- > 0 && ret);
>> @@ -763,9 +767,8 @@ static int i2c_hid_parse(struct hid_device *hid)
>>                 i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
>>         } else {
>>                 rdesc = kzalloc(rsize, GFP_KERNEL);
>> -
>>                 if (!rdesc) {
>> -                       dbg_hid("couldn't allocate rdesc memory\n");
>> +                       i2c_hid_abort_hwreset(ihid);
>>                         return -ENOMEM;
>>                 }
>>
>> @@ -776,10 +779,21 @@ static int i2c_hid_parse(struct hid_device *hid)
>>                                             rdesc, rsize);
>>                 if (ret) {
>>                         hid_err(hid, "reading report descriptor failed\n");
>> +                       i2c_hid_abort_hwreset(ihid);>>                         goto out;
>>                 }
>>         }
>>
>> +       /*
>> +        * Windows directly reads the report-descriptor after sending reset
>> +        * and then waits for resets completion afterwards. Some touchpads
>> +        * actually wait for the report-descriptor to be read before signalling
>> +        * reset completion.
>> +        */
>> +       ret = i2c_hid_finish_hwreset(ihid);
>> +       if (ret)
>> +               goto out;
> 
> Given your new understanding, I wonder if you should start reading the
> report descriptor even if "use_override" is set? You'd throw away what
> you read but maybe it would be important to make the touchscreen
> properly de-assert reset? I guess this is the subject of the next
> patch...

Right, for the devices where use_override gets set
the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET is also always set, so
i2c-hid-core basically does not expect reset-complete to be signalled
via IRQ on these devices.

Since these devices are all kinda weird I have chosen to just preserve
the old behavior (1) there to avoid regressions

1) just doing a msleep(100) instead of waiting for the IRQ

> Also: I guess there's the assumption that the touchscreens needing the
> other caller of the reset functions (I2C_HID_QUIRK_RESET_ON_RESUME)
> never need to read the report like this?

That is correct, only a few devices need to read the report like this
for the reset complete IRQ to happen and since I2C_HID_QUIRK_RESET_ON_RESUME
is only set on a few devices and we know it works there already the
assumption indeed is that on those devices the reading of the report
after reset is not necessary.

Regards,

Hans

[PATCH 5/7] HID: i2c-hid: Remove I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks

[Hans de Goede]

Now that i2c-hid-core waits for the reset after reading the HID report
descriptor, the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks appear to
no longer be necessary (confirmed on one of the quirked models).

Except on laptops where the HID report descriptor is missing and is
provided by a DMI quirk instead. On these models the HID report descriptor
is not read at all, so moving the wait for reset is a no-op. These laptops
have 0911:5288 touchpads for which I2C_HID_QUIRK_NO_IRQ_AFTER_RESET was
set, keep the existing behavior there.

There might still be devices which really do not ack the reset
at all, so to avoid this causing regressions also:

1. Change the wait for ack timeout from an error into a warning; and
2. Lower reset timeout from the very long 5s to 1s which should be plenty

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 3bd0c3d77d99..df5577fc73c5 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -45,7 +45,6 @@
 
 /* quirks to control the device */
 #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
-#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(1)
 #define I2C_HID_QUIRK_BOGUS_IRQ			BIT(4)
 #define I2C_HID_QUIRK_RESET_ON_RESUME		BIT(5)
 #define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(6)
@@ -122,12 +121,6 @@ static const struct i2c_hid_quirks {
 } i2c_hid_quirks[] = {
 	{ USB_VENDOR_ID_WEIDA, HID_ANY_ID,
 		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
-	{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
-		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
-	{ I2C_VENDOR_ID_ITE, I2C_DEVICE_ID_ITE_VOYO_WINPAD_A15,
-		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
-	{ I2C_VENDOR_ID_RAYDIUM, I2C_PRODUCT_ID_RAYDIUM_3118,
-		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
 	{ USB_VENDOR_ID_ALPS_JP, HID_ANY_ID,
 		 I2C_HID_QUIRK_RESET_ON_RESUME },
 	{ I2C_VENDOR_ID_SYNAPTICS, I2C_PRODUCT_ID_SYNAPTICS_SYNA2393,
@@ -470,11 +463,11 @@ static int i2c_hid_start_hwreset(struct i2c_hid *ihid)
 	return ret;
 }
 
-static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
+static int i2c_hid_finish_hwreset(struct i2c_hid *ihid, bool no_irq_after_reset)
 {
 	int ret = 0;
 
-	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
+	if (no_irq_after_reset) {
 		msleep(100);
 		clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
 	}
@@ -482,9 +475,9 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
 	i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
 	if (!wait_event_timeout(ihid->wait,
 				!test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
-				msecs_to_jiffies(5000))) {
-		ret = -ENODATA;
-		goto err_clear_reset;
+				msecs_to_jiffies(1000))) {
+		dev_warn(&ihid->client->dev, "device did not ack reset within 1000 ms\n");
+		clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
 	}
 	i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
 
@@ -494,12 +487,6 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
 
 	mutex_unlock(&ihid->reset_lock);
 	return ret;
-
-err_clear_reset:
-	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
-	mutex_unlock(&ihid->reset_lock);
-	return ret;
 }
 
 static void i2c_hid_abort_hwreset(struct i2c_hid *ihid)
@@ -736,6 +723,7 @@ static int i2c_hid_parse(struct hid_device *hid)
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct i2c_hid_desc *hdesc = &ihid->hdesc;
+	bool no_irq_after_reset = false;
 	unsigned int rsize;
 	char *rdesc;
 	int ret;
@@ -764,6 +752,7 @@ static int i2c_hid_parse(struct hid_device *hid)
 
 	if (use_override) {
 		rdesc = use_override;
+		no_irq_after_reset = true;
 		i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
 	} else {
 		rdesc = kzalloc(rsize, GFP_KERNEL);
@@ -790,7 +779,7 @@ static int i2c_hid_parse(struct hid_device *hid)
 	 * actually wait for the report-descriptor to be read before signalling
 	 * reset completion.
 	 */
-	ret = i2c_hid_finish_hwreset(ihid);
+	ret = i2c_hid_finish_hwreset(ihid, no_irq_after_reset);
 	if (ret)
 		goto out;
 
@@ -1005,7 +994,7 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
 	if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) {
 		ret = i2c_hid_start_hwreset(ihid);
 		if (ret == 0)
-			ret = i2c_hid_finish_hwreset(ihid);
+			ret = i2c_hid_finish_hwreset(ihid, false);
 	} else {
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 	}
-- 
2.41.0

Re: [PATCH 5/7] HID: i2c-hid: Remove I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>

Seems OK to me assuming it's not somehow better to read/throw away the
report on devices needing the override, as discussed in a previous
patch.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 6/7] HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk

[Hans de Goede]

Re-trying the power-on command on failure on all devices should
not be a problem, drop the I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
and simply retry power-on on all devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index df5577fc73c5..ff2659bf5e57 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -44,7 +44,6 @@
 #include "i2c-hid.h"
 
 /* quirks to control the device */
-#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
 #define I2C_HID_QUIRK_BOGUS_IRQ			BIT(4)
 #define I2C_HID_QUIRK_RESET_ON_RESUME		BIT(5)
 #define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(6)
@@ -119,8 +118,6 @@ static const struct i2c_hid_quirks {
 	__u16 idProduct;
 	__u32 quirks;
 } i2c_hid_quirks[] = {
-	{ USB_VENDOR_ID_WEIDA, HID_ANY_ID,
-		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
 	{ USB_VENDOR_ID_ALPS_JP, HID_ANY_ID,
 		 I2C_HID_QUIRK_RESET_ON_RESUME },
 	{ I2C_VENDOR_ID_SYNAPTICS, I2C_PRODUCT_ID_SYNAPTICS_SYNA2393,
@@ -388,8 +385,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	 * The call will get a return value (EREMOTEIO) but device will be
 	 * triggered and activated. After that, it goes like a normal device.
 	 */
-	if (power_state == I2C_HID_PWR_ON &&
-	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV) {
+	if (power_state == I2C_HID_PWR_ON) {
 		ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
 
 		/* Device was already activated */
-- 
2.41.0

Re: [PATCH 6/7] HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Re-trying the power-on command on failure on all devices should
> not be a problem, drop the I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
> and simply retry power-on on all devices.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 7/7] HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines

[Hans de Goede]

The quirks variable and the I2C_HID_QUIRK_ defines are never used /
exported outside of the i2c-hid code renumber them to start at
BIT(0) again.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index ff2659bf5e57..2a4c8657cc7e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -44,10 +44,10 @@
 #include "i2c-hid.h"
 
 /* quirks to control the device */
-#define I2C_HID_QUIRK_BOGUS_IRQ			BIT(4)
-#define I2C_HID_QUIRK_RESET_ON_RESUME		BIT(5)
-#define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(6)
-#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET	BIT(7)
+#define I2C_HID_QUIRK_BOGUS_IRQ			BIT(0)
+#define I2C_HID_QUIRK_RESET_ON_RESUME		BIT(1)
+#define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(2)
+#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET	BIT(3)
 
 /* Command opcodes */
 #define I2C_HID_OPCODE_RESET			0x01
-- 
2.41.0

Re: [PATCH 7/7] HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines

[Doug Anderson]

Hi,

On Sat, Nov 4, 2023 at 4:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The quirks variable and the I2C_HID_QUIRK_ defines are never used /
> exported outside of the i2c-hid code renumber them to start at
> BIT(0) again.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 0/7] HID: i2c-hid: Rework wait for reset to match Windows

[Julian Sax]

Am 04.11.23 um 12:17 schrieb Hans de Goede:
> Julian, I've added you to the Cc because you developed the code
> for touchpads where the HID report descriptors are missing and are
> provided by the kernel through a DMI quirk. The behavior for these
> touchpads should be unchanged, but if you can test on a laptop
> with such a touchpad that would be great.
>

Hi Hans,

I just tested the touchpad with these patches, everything still works fine.


Regards,

Julian

Re: [PATCH 0/7] HID: i2c-hid: Rework wait for reset to match Windows

[Hans de Goede]

Hi,

On 11/8/23 21:41, Julian Sax wrote:
> 
> Am 04.11.23 um 12:17 schrieb Hans de Goede:
>> Julian, I've added you to the Cc because you developed the code
>> for touchpads where the HID report descriptors are missing and are
>> provided by the kernel through a DMI quirk. The behavior for these
>> touchpads should be unchanged, but if you can test on a laptop
>> with such a touchpad that would be great.
>>
> 
> Hi Hans,
> 
> I just tested the touchpad with these patches, everything still works fine.

That is good to hear, thank you for testing.

Regards,

Hans