On Tue, Apr 2, 2024, at 7:10 AM, Hans de Goede wrote: > From: Kenny Levinsen <kl@xxxxxx> > > 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@xxxxxxxxxxxxxxx > Signed-off-by: Kenny Levinsen <kl@xxxxxx> > Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@xxxxxx > [hdegoede@xxxxxxxxxx Drop no longer necessary abort_reset error exit > path] > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > 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@xxxxxxxxx>
