diff mbox series

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

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

Commit Message

Hans de Goede Nov. 4, 2023, 11:17 a.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 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(-)

Comments

Doug Anderson Nov. 6, 2023, 6:53 p.m. UTC | #1
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
Hans de Goede Nov. 17, 2023, 7:42 p.m. UTC | #2
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
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 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);