| Message ID | 20240402111004.161434-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v3] HID: i2c-hid: Revert to await reset ACK before reading report descriptor | expand |
On Tue, Apr 2, 2024, at 7:10 AM, Hans de Goede wrote: > From: Kenny Levinsen <kl@kl.wtf> > > In af93a167eda9, i2c_hid_parse was changed to continue with reading the > report descriptor before waiting for reset to be acknowledged. > > This has lead to two regressions: > > 1. We fail to handle reset acknowledgment if it happens while reading > the report descriptor. The transfer sets I2C_HID_READ_PENDING, which > causes the IRQ handler to return without doing anything. > > This affects both a Wacom touchscreen and a Sensel touchpad. > > 2. On a Sensel touchpad, reading the report descriptor this quickly > after reset results in all zeroes or partial zeroes. > > The issues were observed on the Lenovo Thinkpad Z16 Gen 2. > > The change in question was made based on a Microsoft article[0] stating > that Windows 8 *may* read the report descriptor in parallel with > awaiting reset acknowledgment, intended as a slight reset performance > optimization. Perhaps they only do this if reset is not completing > quickly enough for their tastes? > > As the code is not currently ready to read registers in parallel with a > pending reset acknowledgment, and as reading quickly breaks the report > descriptor on the Sensel touchpad, revert to waiting for reset > acknowledgment before proceeding to read the report descriptor. > > [0]: > https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management > > Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to > after reading the report-descriptor") > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2271136 > Cc: stable@vger.kernel.org > Signed-off-by: Kenny Levinsen <kl@kl.wtf> > Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@kl.wtf > [hdegoede@redhat.com Drop no longer necessary abort_reset error exit > path] > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 29 ++++++++--------------------- > 1 file changed, 8 insertions(+), 21 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c > b/drivers/hid/i2c-hid/i2c-hid-core.c > index 2df1ab3c31cc..13d67d7c67b4 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -735,12 +735,15 @@ static int i2c_hid_parse(struct hid_device *hid) > mutex_lock(&ihid->reset_lock); > do { > ret = i2c_hid_start_hwreset(ihid); > - if (ret) > + if (ret == 0) > + ret = i2c_hid_finish_hwreset(ihid); > + else > msleep(1000); > } while (tries-- > 0 && ret); > + mutex_unlock(&ihid->reset_lock); > > if (ret) > - goto abort_reset; > + return ret; > > use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name, > &rsize); > @@ -750,11 +753,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) { > - ret = -ENOMEM; > - goto abort_reset; > - } > + if (!rdesc) > + return -ENOMEM; > > i2c_hid_dbg(ihid, "asking HID report descriptor\n"); > > @@ -763,23 +763,10 @@ static int i2c_hid_parse(struct hid_device *hid) > rdesc, rsize); > if (ret) { > hid_err(hid, "reading report descriptor failed\n"); > - goto abort_reset; > + 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); > -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); Tested on my Z13 G2 system and confirmed this fixes the issue. Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
On Tue, 2 Apr 2024, Mark Pearson wrote: > On Tue, Apr 2, 2024, at 7:10 AM, Hans de Goede wrote: > > From: Kenny Levinsen <kl@kl.wtf> > > > > In af93a167eda9, i2c_hid_parse was changed to continue with reading the > > report descriptor before waiting for reset to be acknowledged. > > > > This has lead to two regressions: > > > > 1. We fail to handle reset acknowledgment if it happens while reading > > the report descriptor. The transfer sets I2C_HID_READ_PENDING, which > > causes the IRQ handler to return without doing anything. > > > > This affects both a Wacom touchscreen and a Sensel touchpad. > > > > 2. On a Sensel touchpad, reading the report descriptor this quickly > > after reset results in all zeroes or partial zeroes. > > > > The issues were observed on the Lenovo Thinkpad Z16 Gen 2. > > > > The change in question was made based on a Microsoft article[0] stating > > that Windows 8 *may* read the report descriptor in parallel with > > awaiting reset acknowledgment, intended as a slight reset performance > > optimization. Perhaps they only do this if reset is not completing > > quickly enough for their tastes? > > > > As the code is not currently ready to read registers in parallel with a > > pending reset acknowledgment, and as reading quickly breaks the report > > descriptor on the Sensel touchpad, revert to waiting for reset > > acknowledgment before proceeding to read the report descriptor. > > > > [0]: > > https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management > > > > Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to > > after reading the report-descriptor") > > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2271136 > > Cc: stable@vger.kernel.org > > Signed-off-by: Kenny Levinsen <kl@kl.wtf> > > Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@kl.wtf > > [hdegoede@redhat.com Drop no longer necessary abort_reset error exit > > path] > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> [ .. snip .. ] > Tested on my Z13 G2 system and confirmed this fixes the issue. > > Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca> Applied to hid.git#for-6.9/upstream-fixes. Thanks,
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2df1ab3c31cc..13d67d7c67b4 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -735,12 +735,15 @@ static int i2c_hid_parse(struct hid_device *hid) mutex_lock(&ihid->reset_lock); do { ret = i2c_hid_start_hwreset(ihid); - if (ret) + if (ret == 0) + ret = i2c_hid_finish_hwreset(ihid); + else msleep(1000); } while (tries-- > 0 && ret); + mutex_unlock(&ihid->reset_lock); if (ret) - goto abort_reset; + return ret; use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name, &rsize); @@ -750,11 +753,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) { - ret = -ENOMEM; - goto abort_reset; - } + if (!rdesc) + return -ENOMEM; i2c_hid_dbg(ihid, "asking HID report descriptor\n"); @@ -763,23 +763,10 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc, rsize); if (ret) { hid_err(hid, "reading report descriptor failed\n"); - goto abort_reset; + 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); -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);