| Message ID | 20231120193313.666912-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: Rework wait for reset to match Windows | expand |
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
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
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. ;-)
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
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);
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(-)