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

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

[Hans de Goede]

Hi All,

Here is v2 of my i2c-hid series reworking how the i2c-hid-core waits
for reset to complete.

Further testing on the laptop for which I2C_HID_QUIRK_NO_IRQ_AFTER_RESET
was first introduced, shows that reading the report descriptor before
waiting for the reset helps with the missing reset IRQ, but it only helps
some of the time. About 50% of the time the reset still does not get
acked properly.

Therefor for this v2 I have dropped the patch dropping
the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks and I've demoted
this series to a RFC. It is basically ready for merging, but first
the question if this should be applied at all should be discussed:

Advantages of merging this series:

1. Reading descriptors before waiting for reset does make the reset ack IRQ
   happen some of the time, so it does seem to improve things somewhat and
   maybe it does fully fix the issue on some other models

2. This should reduce the probe time of the i2c-hid driver

Disadvantages of merging this series:

1. Non 0 chance of this causing regressions

Changes in v2:
- Drop the patch dropping the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks
- Add a patch changing a missing reset ack from an error into a warning
- Move the mutex_[un]lock(&ihid->reset_lock) calls out of
  i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() and into their
  callers, as suggested by Douglas Anderson

Original v1 cover-letter:

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.

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: Turn missing reset ack into a warning
  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 | 134 +++++++++++++++--------------
 1 file changed, 70 insertions(+), 64 deletions(-)

-- 
2.41.0

[RFC v2 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.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move the i2c_hid_dbg(... "%s: waiting...\n" ...) to above the
  msleep(100) for the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 81 +++++++++++++-----------------
 1 file changed, 35 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..74dd145275f1 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,50 @@ 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;
 	}
 
+	i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
+
+	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
+		msleep(100);
+		clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	}
+
+	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: [RFC v2 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()

[Doug Anderson]

Hi,

On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -482,21 +442,50 @@ 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;
>         }
>
> +       i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
> +
> +       if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
> +               msleep(100);
> +               clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +       }
> +
> +       if (!wait_event_timeout(ihid->wait,

optional: as mentioned in v1, I probably would have made the above an
"else if" to make it clear that it's not ever true if you ran the
"I2C_HID_QUIRK_NO_IRQ_AFTER_RESET" logic.

[RFC v2 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>
---
Changes in v2:
-Move the mutex_[un]lock(&ihid->reset_lock) calls from
 i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() to the callers
 to make the locking more clear
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 38 ++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 74dd145275f1..607ed9b7ba1b 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;
@@ -438,11 +438,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 	 * being reset. Otherwise we may lose the reset complete
 	 * interrupt.
 	 */
-	mutex_lock(&ihid->reset_lock);
+	lockdep_assert_held(&ihid->reset_lock);
 
 	ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 	if (ret)
-		goto err_unlock;
+		return ret;
 
 	/* Prepare reset command. Command register goes first. */
 	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
@@ -460,6 +460,18 @@ 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);
+	return ret;
+}
+
+static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
+{
+	int ret = 0;
+
 	i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
 
 	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
@@ -479,14 +491,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 	if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET))
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 
-	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;
 }
 
@@ -733,7 +742,11 @@ static int i2c_hid_parse(struct hid_device *hid)
 	}
 
 	do {
-		ret = i2c_hid_hwreset(ihid);
+		mutex_lock(&ihid->reset_lock);
+		ret = i2c_hid_start_hwreset(ihid);
+		if (ret == 0)
+			ret = i2c_hid_finish_hwreset(ihid);
+		mutex_unlock(&ihid->reset_lock);
 		if (ret)
 			msleep(1000);
 	} while (tries-- > 0 && ret);
@@ -976,10 +989,15 @@ 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) {
+		mutex_lock(&ihid->reset_lock);
+		ret = i2c_hid_start_hwreset(ihid);
+		if (ret == 0)
+			ret = i2c_hid_finish_hwreset(ihid);
+		mutex_unlock(&ihid->reset_lock);
+	} else {
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
+	}
 
 	if (ret)
 		return ret;
-- 
2.41.0

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

[Doug Anderson]

Hi,

On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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>
> ---
> Changes in v2:
> -Move the mutex_[un]lock(&ihid->reset_lock) calls from
>  i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() to the callers
>  to make the locking more clear
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 38 ++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 10 deletions(-)

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

[RFC v2 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.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
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 607ed9b7ba1b..5e535480fed7 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -775,23 +775,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

[RFC v2 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 reading the report descriptor before waiting for the reset
helps with the missing reset IRQ. Now the reset does get acked properly,
but the ack sometimes still does not happen unfortunately.

Still moving the wait for ack to after reading the report-descriptor,
is probably a good idea, both to make i2c-hid's behavior closer to
Windows as well as to speed up probing i2c-hid devices.

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>
---
Changes in v2:
- Adjust commit message to note that moving the wait-for-reset
  to after reading thr report-descriptor only partially fixes
  the missing reset IRQ problem
- Adjust for the reset_lock now being taken in the callers of
  i2c_hid_start_hwreset() / i2c_hid_finish_hwreset()
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 5e535480fed7..48582c75a51b 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -741,12 +741,9 @@ static int i2c_hid_parse(struct hid_device *hid)
 		return -EINVAL;
 	}
 
+	mutex_lock(&ihid->reset_lock);
 	do {
-		mutex_lock(&ihid->reset_lock);
 		ret = i2c_hid_start_hwreset(ihid);
-		if (ret == 0)
-			ret = i2c_hid_finish_hwreset(ihid);
-		mutex_unlock(&ihid->reset_lock);
 		if (ret)
 			msleep(1000);
 	} while (tries-- > 0 && ret);
@@ -764,8 +761,8 @@ static int i2c_hid_parse(struct hid_device *hid)
 		rdesc = kzalloc(rsize, GFP_KERNEL);
 
 		if (!rdesc) {
-			dbg_hid("couldn't allocate rdesc memory\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto abort_reset;
 		}
 
 		i2c_hid_dbg(ihid, "asking HID report descriptor\n");
@@ -775,10 +772,23 @@ static int i2c_hid_parse(struct hid_device *hid)
 					    rdesc, rsize);
 		if (ret) {
 			hid_err(hid, "reading report descriptor failed\n");
-			goto out;
+			goto abort_reset;
 		}
 	}
 
+	/*
+	 * 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);
+abort_reset:
+	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	mutex_unlock(&ihid->reset_lock);
+	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: [RFC v2 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor

[Doug Anderson]

Hi,

On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -741,12 +741,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>                 return -EINVAL;
>         }
>
> +       mutex_lock(&ihid->reset_lock);
>         do {
> -               mutex_lock(&ihid->reset_lock);
>                 ret = i2c_hid_start_hwreset(ihid);
> -               if (ret == 0)
> -                       ret = i2c_hid_finish_hwreset(ihid);
> -               mutex_unlock(&ihid->reset_lock);
>                 if (ret)
>                         msleep(1000);
>         } while (tries-- > 0 && ret);

Right after this loop, you have:

if (ret)
  return ret;

That will return with the mutex held. It needs to be a "goto
abort_reset". You'd also need to init `use_override` then, I think.

I'll also note that it seems awkward that
`clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)` is scattered in so
many places for error handling, but I couldn't really find a better
way to do it. :-P

-Doug

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

[Hans de Goede]

Hi Doug,

Thank you for the reviews.

On 11/20/23 23:07, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> @@ -741,12 +741,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>>                 return -EINVAL;
>>         }
>>
>> +       mutex_lock(&ihid->reset_lock);
>>         do {
>> -               mutex_lock(&ihid->reset_lock);
>>                 ret = i2c_hid_start_hwreset(ihid);
>> -               if (ret == 0)
>> -                       ret = i2c_hid_finish_hwreset(ihid);
>> -               mutex_unlock(&ihid->reset_lock);
>>                 if (ret)
>>                         msleep(1000);
>>         } while (tries-- > 0 && ret);
> 
> Right after this loop, you have:
> 
> if (ret)
>   return ret;
> 
> That will return with the mutex held. It needs to be a "goto
> abort_reset". You'd also need to init `use_override` then, I think.

Ah, good catch, I will fix this for the next version.

Assuming there will be a next version. Did you read the cover-letter
part about the moving of the wait for reset to after the descriptor
read not fixing the missing reset ack 100% but rather only 50% or
so of the time ?

And do you have any opinion on if we should still move forward with
this patch-set or not ?

> I'll also note that it seems awkward that
> `clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)` is scattered in so
> many places for error handling, but I couldn't really find a better
> way to do it. :-P

I guess we could just no clear it? Only the wait for reset
wait_event_timeout() cares about its value and if we run that
a second time then the bit will be set to 1 again before calling
it anyways...    Not sure I like my own suggestion here, but
it is an option. I'm afraid it may bite us later thogh if we
ever decide to check for the bit in another place.

Regards,

Hans

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

[Doug Anderson]

Hi,

On Tue, Nov 21, 2023 at 1:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> > Right after this loop, you have:
> >
> > if (ret)
> >   return ret;
> >
> > That will return with the mutex held. It needs to be a "goto
> > abort_reset". You'd also need to init `use_override` then, I think.
>
> Ah, good catch, I will fix this for the next version.
>
> Assuming there will be a next version. Did you read the cover-letter
> part about the moving of the wait for reset to after the descriptor
> read not fixing the missing reset ack 100% but rather only 50% or
> so of the time ?
>
> And do you have any opinion on if we should still move forward with
> this patch-set or not ?

I'd tend to leave it to your judgement. I have a bias towards landing
it because it improves probe speed in a way that matches what the spec
suggests and, IMO, probe speed is important. It also has the potential
to avoid the need for quirks on some devices, even if it didn't work
out that way for the device you tested with.

The only downside you have listed is the potential for regressions,
but that's something that's a risk for nearly any change. This doesn't
feel like an excessively risky change to me and, as you've pointed
out, it's documented in the spec. If someone reports a regression then
it seems like we should address it as it comes...


> > I'll also note that it seems awkward that
> > `clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)` is scattered in so
> > many places for error handling, but I couldn't really find a better
> > way to do it. :-P
>
> I guess we could just no clear it? Only the wait for reset
> wait_event_timeout() cares about its value and if we run that
> a second time then the bit will be set to 1 again before calling
> it anyways...    Not sure I like my own suggestion here, but
> it is an option. I'm afraid it may bite us later thogh if we
> ever decide to check for the bit in another place.

Yeah, I didn't have any great ideas either and I think it's fine as
you have it. It just bothered me as I was reviewing and so I figured
I'd mention it in case some brilliant idea occurred to you. ;-)

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

[Hans de Goede]

Hi,

On 11/21/23 16:25, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 21, 2023 at 1:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> Right after this loop, you have:
>>>
>>> if (ret)
>>>   return ret;
>>>
>>> That will return with the mutex held. It needs to be a "goto
>>> abort_reset". You'd also need to init `use_override` then, I think.
>>
>> Ah, good catch, I will fix this for the next version.
>>
>> Assuming there will be a next version. Did you read the cover-letter
>> part about the moving of the wait for reset to after the descriptor
>> read not fixing the missing reset ack 100% but rather only 50% or
>> so of the time ?
>>
>> And do you have any opinion on if we should still move forward with
>> this patch-set or not ?
> 
> I'd tend to leave it to your judgement. I have a bias towards landing
> it because it improves probe speed in a way that matches what the spec
> suggests and, IMO, probe speed is important.

I'm tending towards still merging this myself too. So when I've some
time I'll address your remarks and post a non RFC v3.

Regards,

Hans

[RFC v2 5/7] HID: i2c-hid: Turn missing reset ack into a warning

[Hans de Goede]

On all i2c-hid devices seen sofar the reset-ack either works, or the hw is
somehow buggy and does not (always) ack the reset properly, yet it still
works fine.

Lower the very long reset timeout to 1 second which should be plenty
and change the reset not getting acked from an error into a warning.

This results in a bit cleaner code and avoids the need to add more
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future.

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

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 48582c75a51b..96fb5aafc1fa 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -481,9 +481,9 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
 
 	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__);
 
@@ -492,11 +492,6 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
 		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 
 	return ret;
-
-err_clear_reset:
-	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
-	return ret;
 }
 
 static void i2c_hid_get_input(struct i2c_hid *ihid)
-- 
2.41.0

Re: [RFC v2 5/7] HID: i2c-hid: Turn missing reset ack into a warning

[Doug Anderson]

Hi,

On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On all i2c-hid devices seen sofar the reset-ack either works, or the hw is
> somehow buggy and does not (always) ack the reset properly, yet it still
> works fine.
>
> Lower the very long reset timeout to 1 second which should be plenty
> and change the reset not getting acked from an error into a warning.
>
> This results in a bit cleaner code and avoids the need to add more
> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)

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

[RFC v2 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.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
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 96fb5aafc1fa..3bdfd3e89de5 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_NO_IRQ_AFTER_RESET	BIT(1)
 #define I2C_HID_QUIRK_BOGUS_IRQ			BIT(4)
 #define I2C_HID_QUIRK_RESET_ON_RESUME		BIT(5)
@@ -120,8 +119,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 },
 	{ 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,
@@ -395,8 +392,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

[RFC v2 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.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 3bdfd3e89de5..151d5a5c87ca 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -44,11 +44,11 @@
 #include "i2c-hid.h"
 
 /* quirks to control the device */
-#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)
-#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET	BIT(7)
+#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(0)
+#define I2C_HID_QUIRK_BOGUS_IRQ			BIT(1)
+#define I2C_HID_QUIRK_RESET_ON_RESUME		BIT(2)
+#define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(3)
+#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET	BIT(4)
 
 /* Command opcodes */
 #define I2C_HID_OPCODE_RESET			0x01
-- 
2.41.0