Received-SPF: pass (google.com: domain of linux-kernel+bounces-157361-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <a810561a-14f3-412e-9903-acaba7a36160@leemhuis.info>
Date: Wed, 24 Apr 2024 18:56:18 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: regression fixes sitting in subsystem git trees for a week or longer
 (was: Re: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading
 report descriptor)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jiri Kosina <jikos@kernel.org>, Douglas Anderson <dianders@chromium.org>,
 Hans de Goede <hdegoede@redhat.com>, linux-input@vger.kernel.org,
 linux-kernel@vger.kernel.org, Kenny Levinsen <kl@kl.wtf>,
 Benjamin Tissoires <benjamin.tissoires@redhat.com>,
 Linux regressions mailing list <regressions@lists.linux.dev>
References: <20240331182440.14477-1-kl@kl.wtf>
 <14d1b38e-0f11-4852-9c52-92b4bedb0a77@leemhuis.info>
 <CAO-hwJJtK2XRHK=HGaNUFb3mQhY5XbNGeCQwuAB0nmG2bjHX-Q@mail.gmail.com>
From: Thorsten Leemhuis <regressions@leemhuis.info>
Content-Language: en-US, de-DE
In-Reply-To: <CAO-hwJJtK2XRHK=HGaNUFb3mQhY5XbNGeCQwuAB0nmG2bjHX-Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Linus,

On 23.04.24 16:59, Benjamin Tissoires wrote:
> On Mon, Apr 22, 2024 at 7:11 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
>> On 31.03.24 20:24, Kenny Levinsen wrote:
>
> [previous subject: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor]
>
>>> 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?
> 
> No special reasons I guess. Neither Jiri nor I have sent a HID update
> for this rc cycle, so it's still there, waiting to be pushed.
> I've been quite busy with BPF lately and dropped the ball slightly on
> the HID maintainer side, but I'm sure we'll send the PR to Linus this
> week or the next.

out of interest: what's your stance on regression fixes sitting in
subsystem git trees for a week or longer before being mainlined?

The quoted patch is such a case. It fixes a regression caused by a
change that made it into 6.8-rc1, but the problem afaik was only
reported on 2024-03-19, e.g. ~nine days after 6.8 was out[1]; Kenny, the
author of the fix, apparently noticed and fixed the problem a bit later
independently[2]. Jiri merged a newer version of the fix on
2024-04-03[3], which was included in -next a day later -- the Thursday
before 6.9-rc3.

The fix thus would even have gotten two days of testing in -next, if
Benjamin or Jiri would have send it your way for that pre-release. But
from Benjamin's statement quoted above it seems the fix might even make
-rc6.

That obviously heavily reduces the time the fix will be tested before
6.9 is released.

It obviously also means that 6.8.y is as of now still unfixed, as the
stable team usually only applies fixes once they landed in mainline.

Which also means that even more people ran into the problem with
6.8.y[4] or mainline even after Jiri merged the patch into the hid tree
-- and maybe some of those people wasted their time on a bisection only
to find out that a fix exists.

That sounds, ehh, sub-optimal to me. Which is why I wonder what's your
stance here, as I encounter similar situations frequently[5] -- which
sometimes is kinda demotivating. :-/

Ciao, Thorsten

[1]
https://lore.kernel.org/all/a587f3f3-e0d5-4779-80a4-a9f7110b0bd2@manjaro.org/

[2] https://lore.kernel.org/all/20240331132332.6694-1-kl@kl.wtf/

[3]
https://lore.kernel.org/all/nycvar.YFH.7.76.2404031401411.20263@cbobk.fhfr.pm/

[4] https://social.lol/@major/112294923280815017

[5] This fix for example:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=master&id=afc89870ea677bd5a44516eb981f7a259b74280c
Reports:
https://lore.kernel.org/lkml/ZYhQ2-OnjDgoqjvt@wens.tw/
https://lore.kernel.org/lkml/1553a526-6f28-4a68-88a8-f35bd22d9894@linumiz.com/

> [...]
>>> 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@kl.wtf>
>>> ---
>>>  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;
>>>
>>
> 
> 
>