Message-ID: <1c53a747-f606-44e4-804c-2c0fc7c9c600@enpas.org>
Date: Tue, 23 Jul 2024 04:24:46 +0900
From: Max Staudt <max@...as.org>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: Roderick Colenbrander <thunderbird2k@...il.com>,
 Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <benjamin.tissoires@...hat.com>,
 Roderick Colenbrander <roderick.colenbrander@...y.com>,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] hid-playstation: DS4: Update rumble and lightbar
 together

On 7/23/24 01:46, Benjamin Tissoires wrote:
>> As a generalised question, how about controllers that work on
>> Android phones with kernel v6.1 (hid-sony), but not with v6.6
>> (hid-playstation), because of protocol changes that don't affect
>> first-party controllers. Do we accept upstream regressions on
>> actual, physical devices for the sake of passing downstream tests?
> 
> The upstream rule is simple: no regressions (that we know about).
> The argument that downstream tests are hard to do is not correct for 
> upstream. As a general rule of thumb, upstream doesn't care about 
> downstream at all. Regressions is all we care, and a bad test from 
> downstream is not a correct justification to reject a fix upstream.
> 
> Please understand Roderick that I am not taking side. I perfectly 
> understand the downstream challenges, but we can not refuse a
> regression fix because the new patch breaks a downstream test.
> 
> I followed the thread and Max seemed to be OK with waiting and I
> assumed it was not critical. But if we know about a regression in a
> device we supported, we should find a solution for it.

Thanks for clarifying the general rule about no regressions!

I asked in the general sense, because I needed to know how the "no 
regressions" rule works in clear-cut cases, and that the move from v6.2 
to v6.3 indeed counts as introducing regressions, and that they are 
worth fixing even if they break downstream tests.

If you're interested: The 8BitDo controller failed due to a request that 
was a warning in hid-sony, and then became a hard error in 
hid-playstation. This regression was fixed in 46089080a8e1.


Please let me clarify the current patch, since it's maybe less 
clear-cut. Do you have guidance on this as well?

With this patch, I intend to port over a behaviour from hid-sony, but I 
can (currently?) only test it on a controller which also needs a 
behaviour that appeared in the kernel with hid-playstation (it's the 
0x12 feature report that Roderick mentioned previously). So it's not a 
clear-cut regression, but still I am porting a behaviour that was there 
before.

Hence I am trying to merge the behaviour of both drivers, for maximum 
compatibility.

Since it's possible that other controllers also worked better with 
hid-sony's output reports, I've looked for third-party hints, and 
mentioned them in my code comments and commit message, to better gauge 
what the most compatible path forward may be. It seems like a middle 
ground between both drivers is the most compatible, so my patch takes 
this into account.


Where does this fall on the continuum of patches to take in or not?


And as an even more general question: Do downstream tests count as 
things that we don't break when fixing real devices?


> BTW, that's the reason why I finally managed to push the hid-tools
> tests in the seftests dir of the kernel directly. Now each kernel has
> its own set of tests, and there is no more discrepancies between
> tests and regressions that are found.

Great, thank you!



Max

Powered by blists - more mailing lists