diff mbox series

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

Message ID 20231202224615.24818-5-hdegoede@redhat.com
State Accepted
Commit af93a167eda90192563bce64c4bb989836afac1f
Headers show
Series HID: i2c-hid: Rework wait for reset to match Windows | expand

Commit Message

Hans de Goede Dec. 2, 2023, 10:46 p.m. UTC
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 v3:
- Use goto abort_reset instead of return on i2c_hid_start_hwreset()
  failure, so that the mutex gets properly unlocked

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 | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Doug Anderson Dec. 4, 2023, 4:28 p.m. UTC | #1
Hi,

On Sat, Dec 2, 2023 at 2:46 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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 v3:
> - Use goto abort_reset instead of return on i2c_hid_start_hwreset()
>   failure, so that the mutex gets properly unlocked
>
> 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 | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)

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

Patch

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 71d742aeaf35..400c15a180b5 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -725,11 +725,10 @@  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;
+	char *rdesc = NULL, *use_override = NULL;
 	unsigned int rsize;
-	char *rdesc;
 	int ret;
 	int tries = 3;
-	char *use_override;
 
 	i2c_hid_dbg(ihid, "entering %s\n", __func__);
 
@@ -739,18 +738,15 @@  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);
 
 	if (ret)
-		return ret;
+		goto abort_reset;
 
 	use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name,
 								&rsize);
@@ -762,8 +758,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");
@@ -773,10 +769,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);