qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.28k stars 39.38k forks source link

[Bug] 50-qmk.rules udev rule breaks game controllers #24222

Open gudvinr opened 3 months ago

gudvinr commented 3 months ago

Describe the Bug

When you install this file according to the docs, you end up with game controllers (non qmk based, regular game controllers) not working anymore.

I've seen this behaviour with xbox series bluetooth controllers and xpadneo driver.

Once you remove qmk udev rule and reload, controller works again.

It is noticeable as apps not being able to receive controller inputs. You press keys, move sticks, etc and there is no input in the apps. Hovewer, evtest still shows input data.

It doesn't matter if QMK keyboard (or any input device other than gamepad itself) is connected to the system.

Keyboard Used

irrelevant

Link to product page (if applicable)

also irrelevant

Operating System

Arch Linux

qmk doctor Output

not using qmk cli

Is AutoHotKey / Karabiner installed

Other keyboard-related software installed

none

Additional Context

No response

fauxpark commented 3 months ago

This is probably due to this rule:

KERNEL=="hidraw*", MODE="0660", GROUP="plugdev", TAG+="uaccess", TAG+="udev-acl"

Which is required for qmk console. As far as I'm aware it's not possible to make this any more specific, since QMK devices do not have a fixed vendor or product ID.

sigprof commented 3 months ago

It could be possible to detect the QMK console devices automatically by the usage code for the HID application collection (but the detection is not guaranteed to be 100% precise, because it involves HID usage codes from the vendor-specific range, and those codes are not really registered anywhere, so anyone else could pick the same codes to mean something completely different). But doing that requires installing a helper program to parse the HID descriptors: https://github.com/qmk/qmk_udev

gudvinr commented 3 months ago

As far as I'm aware it's not possible to make this any more specific, since QMK devices do not have a fixed vendor or product ID.

By QMK devices you mean devices that already run QMK firmware and use hidraw to communicate with PC?
If yes, then you don't really need to match by VID/PID because udev allows matching using any of the device (or parent device) attributes (see http://www.reactivated.net/writing_udev_rules.html#sysfsmatch).

I am not sure about what exactly can be used but maybe configuration or interface descriptor names can be used. I believe you can change product name without changing VID/PID too. It could be possible to use some custom attributes that I am not aware of.

fauxpark commented 3 months ago

There are no other useful attributes to filter on. QMK does not set string descriptors for anything other than manufacturer, product and serial number, all of which are variable based on the particular board. The only real solution would be what sigprof described - having udev run an external program to inspect the HID usage page/usage pair, but as mentioned it is not very reliable or convenient.

I'm not really sure why exactly this udev rule is causing this issue - I'm not that familiar with udev. But perhaps there is some other way around it.

gudvinr commented 3 months ago

Is it really necessary to have such permissive rules in the first place?

From what I see, it seems like these only used for something called hid_listen which appears to be a debugging tool

For me it seems like it's not very relevant for an average user and only needed for developers and troubleshooting.

In this case, adding a rule that basically allows rawdogging hid devices by any user seems too much for potential benefits. Not only that but apparently this also breaks 3rd party code that has no relation to QMK at all.

This can be solved by either mentioning to run hid_listen under sudo or changing access only for specific devices that you want hid_listen to use with

You don't need raw access to all devices anyway so hid_listen or some wrapper can ask for rights elevation and change permissions one time and/or add udev rule for exact device. While QMK has variety of supported devices, end user only needs to access their devices.

fauxpark commented 3 months ago

hid_listen is nothing to do with QMK. https://www.pjrc.com/teensy/hid_listen.html

This is about the CLI subcommand qmk console which is a more polished, QMK-specific version. Since it is a part of the CLI, it should be expected to "just work".

gudvinr commented 3 months ago

First of all, it won't "just work" out if the box because somehow you need to have udev rules installed and you can't install udev rules without elevated privileges. Doesn't matter if you do it manually or by installing package from package manager

And second, "just work" doesn't mean that app absolutely can't elevate privileges only for actions that, well, need elevated privileges.

There are plenty of apps that can be used by regular user and only require using sudo for parts of it. Notable example of that is package manager. Virtually all PMs will require you to run as root to install packages and work just fine when you only need to run query operations.
Same with systemd and its *ctl apps

tzarc commented 3 months ago

It seems you’re quite passionate about this issue — I’d suggest working out what you think is acceptable given our general usability constraints as well (i.e. qmk console) and raise a corresponding PR.

In its current form the rules work for 99% of the use cases we’ve come across, and unfortunately it seems you’re one of the 1%. If you can improve the situation to be 99.9% vs. 0.1%, then great! As it’s generally working, any changes to the rules are currently extremely low priority from our team perspective, which is why we’re suggesting perhaps you’re in a better position to take action.

Alternatively, the udev rules mismatching only a client-side warning, so you’re free to remove whichever unused entries if you don’t wish to improve the situation for everyone.

gudvinr commented 3 months ago

It seems you’re quite passionate about this issue

Only thing I care about is my system "just working".
Thing is, that these rules break other programs. I wouldn't care if it was minor inconvenience contained inside QMK but it isn't.

xpadneo even has checkbox in their issue template because of how viral this is. People even make workarounds when they bump into this simply because they don't question why this situation even exist.

You can't argue in this case "maybe it's them and not us" because 60-xpadneo.rules contains very narrow set of rules.

you’re free to remove whichever unused entries if you don’t wish to improve the situation for everyone.

I already removed all rules except ones that I use to flash firmware but only after spending significant amount of time investigating why game controller doesn't work.

As it’s generally working, any changes to the rules are currently extremely low priority

If you are reluctant to the changes, warning could be added that this rule can cause conflicts with other devices.
As a comment in rules file, in qmk doctor and in FAQ, where documentation about qmk console/hid_listen suggests adding this.

It won't fix the issue but at least people who follow your suggestions will know why their other devices don't work as expected.

As far as I'm aware it's not possible to make this any more specific, since QMK devices do not have a fixed vendor or product ID.

OpenRGB does this by compiling udev rules dynamically using information about all supported devices.

In its current form the rules work for 99% of the use cases we’ve come across

https://docs.qmk.fm/cli doesn't mention any distributive that has these rules by default.
Only one I found is arch and they use rules from qmk/qmk_udev which has this particular string commented.

QMK cli itself, while installed from pip, won't have these rules by default anyway. So I don't know where you get your numbers from but it's a big question if majority of people even install these rules.

sigprof commented 3 months ago

The main annoyance of qmk/qmk_udev is that it requires a compiled helper, so you can't just throw some udev rules into the system configuration.

It could be possible to simulate what that helper does with some commonly available tools (although this would need lots of testing on more weird systems — e.g., the ones using eudev, or the busybox implementation of tools instead of coreutils):

# hidraw devices
## supporting rules
## note: udev limits environment variables to 512 bytes
SUBSYSTEM=="hid", PROGRAM="/bin/sh -c 'od -v -w255 -N255 -An -tx1 \"$$0\" | tr -dc 0-9a-f' %S%p/report_descriptor", ENV{HID_REPORT_DESCRIPTOR}="%c"
KERNEL=="hidraw*", IMPORT{parent}="HID_REPORT_DESCRIPTOR"
## VIA: application collection with usage page 0xff60, usage 0x61
KERNEL=="hidraw*", ENV{HID_REPORT_DESCRIPTOR}=="0660ff0961a101*", TAG+="uaccess"
## HID console: application collection with usage page 0xff31, usage 0x74
KERNEL=="hidraw*", ENV{HID_REPORT_DESCRIPTOR}=="0631ff0974a101*", TAG+="uaccess"

The /bin/sh -c weirdness is needed because normally udev invokes PROGRAM without going through the shell, and piping through tr -dc 0-9a-f is needed to remove all spaces from the string (the presence of spaces apparently breaks the subsequent IMPORT{parent}). I'm not sure whether hexdump -v -e "/1 \"%02x\"" would be available everywhere. Another reason to go through the shell is to make the rule work in NixOS, where the only thing which is actually located in /bin is /bin/sh, and any other tools are either supposed to be called by their full store paths (which can't be determined outside of NixOS configs), or found in $PATH (which udev does not do by itself, so going through the shell is needed).

The matching like that is not 100% generic (there are infinitely many ways in which the same application collection may be represented in the HID report descriptor), but should work with existing QMK versions. However, any HID report descriptor modifications in QMK may break compatibility (e.g., if someone would want to support 16-bit values in RAW_USAGE_ID, the existing 0x09 0x61 8-bit tag would need to be replaced with a 16-bit tag, like 0x0a 0x61 0x00, which would need a new pattern to match).

t-8ch commented 2 months ago

@gudvinr As you are on Arch it should be enough to install the qmk package which already provides rules based on the udev helper. (Disclaimer: I'm the author)

grasegger commented 2 months ago

This is probably due to this rule:

KERNEL=="hidraw*", MODE="0660", GROUP="plugdev", TAG+="uaccess", TAG+="udev-acl"

Which is required for qmk console. As far as I'm aware it's not possible to make this any more specific, since QMK devices do not have a fixed vendor or product ID.

This could be encanced to make sure to skip the problematic vid though. Of the top of my head, using https://www.freedesktop.org/software/systemd/man/latest/udev.html and https://devicehunt.com/view/type/usb/vendor/045E as a reference, this could eliviate the problem, no?

KERNEL=="hidraw*", VID!="045E", MODE="0660", GROUP="plugdev", TAG+="uaccess", TAG+="udev-acl"