Hi Kenny, Sorry for causing this regression and thank you for your fix. One small remark comment below. In the hope of getting this merged soon I'll prepare a v3 addressing this myself (keeping you as the author). On 3/31/24 8:24 PM, Kenny Levinsen wrote: > 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 acknowledgement 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 acknowledgement, 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 acknowledgement, and as reading quickly breaks the report > descriptor on the Sensel touchpad, revert to waiting for reset > acknowledgement 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") > Signed-off-by: Kenny Levinsen <kl@xxxxxx> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 2df1ab3c31cc..72d2bccf5621 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -735,9 +735,12 @@ 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; The abort_reset label here and in other places now is no longer necessary. i2c_hid_start_hwreset() (on error) and i2c_hid_finish_hwreset() (regardless of error or not) always clear I2C_HID_RESET_PENDING. And we only do "goto abort_reset;" here and in 2 other places below in a "if (ret) {}" branch, and abort_reset itself is: abort_reset: clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); if (ret) goto out; Since the reset loop now always exits with I2C_HID_RESET_PENDING cleared, the clear_bit() is not necessary after your changes and ret != 0 is always true when doing goto abort_reset so "goto abort_reset" can be replaced with "goto out" or if there is nothing to cleanup with a simple "return ret". As mentioned above I'll post a v3 with this addressed myself, so that we can hopefully get the fix upstream soonest. Regards, Hans > @@ -767,16 +770,8 @@ static int i2c_hid_parse(struct hid_device *hid) > } > } > > - /* > - * 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; >
