Bug 204191 - Force feedback not working on Logitech G920
Summary: Force feedback not working on Logitech G920
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Input Devices (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_input-devices
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-17 01:42 UTC by Sam Bazley
Modified: 2020-01-21 11:40 UTC (History)
6 users (show)

See Also:
Kernel Version: 5.2
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
dmesg after running ffcfstress (73.98 KB, text/plain)
2019-07-17 01:42 UTC, Sam Bazley
Details
USB HID descriptor parsed (3.98 KB, text/plain)
2019-09-17 22:49 UTC, Sam Bazley
Details
USB HID descriptor binary (168 bytes, image/x-tga)
2019-09-17 22:50 UTC, Sam Bazley
Details
Fix detection of Logitech G920 as hidpp device (884 bytes, patch)
2019-10-03 13:15 UTC, mikkl
Details | Diff
Fix detection of Logitech G920 as hidpp device (1.80 KB, patch)
2019-10-03 15:19 UTC, mikkl
Details | Diff
G920 fixes patch 1 (4.06 KB, patch)
2019-10-07 05:25 UTC, Andrey Smirnov
Details | Diff
G920 fixes patch 2 (15.19 KB, patch)
2019-10-07 05:25 UTC, Andrey Smirnov
Details | Diff
G920 fixes patch 3 (2.02 KB, patch)
2019-10-07 05:26 UTC, Andrey Smirnov
Details | Diff

Description Sam Bazley 2019-07-17 01:42:21 UTC
Created attachment 283757 [details]
dmesg after running ffcfstress

The wheel autocenters, but no other force feedback works. ffcfstress outputs:
ERROR: device (or driver) has no constant force feedback support [ffcfstress.c:194]

I've bisected, and found that fe3ee1ec007bf7b10d7c02814d4b8fbe7d9bb435 is the first bad commit.
Comment 1 Sam Bazley 2019-08-13 23:13:21 UTC
If it helps, the driver is treating the wheel as a generic HID, because hidpp_validate_device returns false, because hidpp_validate_report(... REPORT_ID_HIDPP_SHORT, ...) returns false, because report_length is false.
Comment 2 Xpander 2019-08-28 05:52:05 UTC
Confirming the same issue. Good work with bisecting it.
Comment 3 Danilo Pianini 2019-08-30 08:00:47 UTC
I can confirm and reproduce the issue as well.
Comment 4 Xpander 2019-08-30 08:35:39 UTC
dmesg logs with 5.2.10 and 4.19.69lts kernel

https://pastebin.com/U144bTar
Comment 5 Chris Caudle 2019-09-17 17:11:05 UTC
Has there been any progress on this?  5.2.13, 5.2.14, and 5.3 are all released now.  This has completely broken use of the Logitech steering wheel with driving/racing simulation.
Comment 6 Sam Bazley 2019-09-17 22:49:22 UTC
Created attachment 285039 [details]
USB HID descriptor parsed

I did some more digging and found that the USB HID descriptor for the wheel has a report ID for REPORT_ID_HIDPP_LONG (line 56) and REPORT_ID_HIDPP_VERY_LONG (line 69), but no report ID for REPORT_ID_HIDPP_SHORT, which is why hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT, ...) fails.
Comment 7 Sam Bazley 2019-09-17 22:50:03 UTC
Created attachment 285041 [details]
USB HID descriptor binary
Comment 8 Chris Caudle 2019-09-20 21:11:40 UTC
I am looking at the HID version 1.11 spec, but I am not familiar with it, having trouble finding my way around.
Does this seem to be a mistake in the device implementation that needs to be worked around in the driver, or a mistaken assumption in the driver that all three  REPORT_ID_HIDPP variations will always be present?
Anyone know which section of the spec would be relevant to this?  The report examples in the spec just show one section, so I don't understand how the LONG, VERY_LONG, and SHORT reports come into this.
Comment 9 Sam Bazley 2019-09-21 00:01:23 UTC
The REPORT_ID_HIDPP_* reports are from the HID++ spec. There's a couple of links on it in the readme of this repo https://github.com/cvuchener/hidpp
Comment 10 Chris Caudle 2019-09-23 16:22:42 UTC
(In reply to Sam Bazley from comment #9)
> The REPORT_ID_HIDPP_* reports are from the HID++ spec. There's a couple of
> links on it in the readme of this repo https://github.com/cvuchener/hidpp

So this is a Logitech only problem, not a generic USB HID problem?  
I don't understand why this change is not reverted yet, it is a change for a logitech specific protocol and it breaks logitech controllers, or at least logitech steering wheel controllers.  
I have not pulled out my force feedback joystick recently, anyone know if those are broken as well?  Does this change just make a fancy mouse work better or something? 
I thought the kernel approval process was pretty strict about not accepting changes which break working applications.  Seems a clear case of "breaks every driving simulation game" so would get this change reverted more quickly than the two and a half months this bug report has been active.
Comment 11 mikkl 2019-10-03 13:15:44 UTC
Created attachment 285321 [details]
Fix detection of Logitech G920 as hidpp device

Apart from the missing REPORT_ID_HIDPP_SHORT which causes hidpp_validate_report() to fail there is another problem in the driver.

Supposing that the check passes there will be a null pointer dereference in g920_get_config() -> hidpp_ff_init() line 3 of this function:

static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
{
	struct hid_device *hid = hidpp->hid_dev;
        struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
        struct input_dev *dev = hidinput->input;
        ...

This is caused by the way hid_hw_start(hdev, 0) is called before g920_get_config() gets called. If I call hid_hw_start(hdev, HID_CONNECT_DEFAULT) instead there is no problem. This is actually the way it was called before when it still did work.

I have attached a patch that fixes the two problems in hid-logitech-hidpp.c. While the second part is specific to the G920 the first part with the REPORT_IDs is not and I don't know if this could brake other things.
Comment 12 mikkl 2019-10-03 15:19:29 UTC
Created attachment 285323 [details]
Fix detection of Logitech G920 as hidpp device

Sorry, the previous patch was a little premature. There seems to be something more special about the G920 (device must not be closed/stopped before returning from hidpp_probe() ?). The patch now simply restores the previously working behaviour.
Comment 13 Xpander 2019-10-05 12:42:21 UTC
Thank you. This patch works wonderfully. Applied it to kernel 5.3.2 and forcefeedback on my G920 started working again.
Comment 14 Andrey Smirnov 2019-10-07 05:25:29 UTC
Created attachment 285375 [details]
G920 fixes patch 1

AFAICT, there were actually two regression in G920 support since 4.19. I submitted the following three patches upstream to fix them:

https://lore.kernel.org/lkml/20191007051240.4410-1-andrew.smirnov@gmail.com/
Comment 15 Andrey Smirnov 2019-10-07 05:25:58 UTC
Created attachment 285377 [details]
G920 fixes patch 2
Comment 16 Andrey Smirnov 2019-10-07 05:26:17 UTC
Created attachment 285379 [details]
G920 fixes patch 3
Comment 17 Chris Caudle 2019-10-10 13:35:12 UTC
Tried the 3-patch solution with the Fedora build of 5.2.18 and seems to work correctly with my G920.
Someone at Steam mentioned that one of their kernel engineers was working on this. Is that Andrey?  If not someone should make sure they are aware of this thread, looks like the solution is good, just needs to get reviewed and accepted upstream (and let the distributions know they should backport).
I think I will submit a Fedora request to have this backported, unless there is any reason to wait.
Comment 18 Sam Bazley 2019-10-10 17:14:12 UTC
I tested the patches earlier and it seems to work properly now. Thanks Andrey :ยท)
Comment 19 Chris Caudle 2019-10-10 18:47:21 UTC
Fedora kernel dev said that Fedora would prefer to wait until this had been ack'd by upstream maintainer before including.  If possible please add a reference to any LMKL confirmation that these patches will be accepted upstream, hopefully that will be enough for Fedora to include(the idea was include a patch that could be dropped as unneeded in the future, and not have to carry these patches forever).
thanks
Comment 20 Andrey Smirnov 2019-10-10 20:25:24 UTC
@Chris Caudle

Yes, that would be me, I was asked to look at this and provided the necessary HW by the fine folks at Valve. I was aware of this thread from the discussion on Steam, although forgot to refresh this page and did not see the patch @mikkle attached (or any of their messages) before I posted my version of the fixes.

FWIW, I tagged all of my patches to be included into stable trees, so, once Greg KH picks it up, I think Fedora (which is what I personally use as well), Ubuntu or any other distribution should get them that way eventually. I'll keep this ticket updated on the status of my submission regardless.

@Sam Bazely
Good to hear! Since you are CC'd on that thread, feel free to reply to cover letter and provide a "Tested-By" tag.
Comment 21 mikkl 2019-10-11 15:53:37 UTC
Patches work for me, too. Actually Andrey's patches do some more things, i.e. auto-centering when e.g. resetting car in Dirt Rally never worked before and now it does.
Comment 22 Xpander 2020-01-21 11:24:52 UTC
Sorry for Bump on this, but its been quite some time now. Is this finally landed upstream or we still need to patch the kernel?

This is been a problem at least 3 kernel cycles already
Comment 23 Sam Bazley 2020-01-21 11:29:24 UTC
5.3.9 has the patch. If you're having issues using the wheel with Steam, you might need to run `steam --reset`, although that will reset other settings as well, so you should probably test with `jstest(-gtk)`
Comment 24 Xpander 2020-01-21 11:31:39 UTC
(In reply to Sam Bazley from comment #23)
> 5.3.9 has the patch. If you're having issues using the wheel with Steam, you
> might need to run `steam --reset`, although that will reset other settings
> as well, so you should probably test with `jstest(-gtk)`

Thanks. I have 5.4.2 patched with Andrey Smirnov patches. And it works, i was just wondering if its already upstreamed and i can discard the patches now.

if its in 5.3.9 thats nice to hear, i guess its in 5.4+ also then?
Comment 25 Sam Bazley 2020-01-21 11:40:38 UTC
(In reply to Xpander from comment #24)
> i guess its in 5.4+ also then?

Yes

Note You need to log in before you can comment on or make changes to this bug.