On 31.03.24 20:24, 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: Lo! Jiri, Benjamin, quick question: is there a reason why this fix for a 6.8-rc1 regression after more than two and half weeks is not yet mainlined? Or is there some good reason why we should be should be extra cautious? Side note: I noticed this due to the tracking today, but I also saw a user that recently ran into the problem the quoted fix is supposed to resolve: https://social.lol/@major/112294923280815017 Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke > 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; > @@ -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; >
