[PATCH 0/4] MR5331: winebus.sys: prefer hidraw for non-input hidraw devices
## Goal Make non-input hidraw devices work again. ## Description The current device backend selection policy is input-device oriented and it does not handle non-input hidraw devices correctly. Pre-9.1 versions handled all hidraw devices with hidraw backend, but since 9.1, there have been changes/additions to the backend selection policy which have caused non-input hidraw devices to stop working. ## Changes in nutshell - add `device_desc.has_hid_input_relative` which tells if the device has hid subsystem ancestor AND the hid ancestor has an input subsystem descendant; this is useful for detecting if a hidraw device is a non-input device or not (non-input hidraw devices do not have hid input relatives) - prefer hidraw backend for all such non-input hidraw devices -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5331
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> Not using hidraw backend for non-hidraw device is the expected behavior. Warning about such cases is not justified. --- dlls/winebus.sys/main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index dceba4747b8..b884162692f 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -618,9 +618,11 @@ static DWORD CALLBACK bus_main_thread(void *args) case BUS_EVENT_TYPE_DEVICE_CREATED: { const struct device_desc *desc = &event->device_created.desc; - if (!desc->is_hidraw != !is_hidraw_enabled(desc->vid, desc->pid)) + if (!desc->is_hidraw) + break; + if (!is_hidraw_enabled(desc->vid, desc->pid)) { - WARN("ignoring %shidraw device %04x:%04x\n", desc->is_hidraw ? "" : "non-", desc->vid, desc->pid); + WARN("ignoring hidraw device %04x:%04x\n", desc->vid, desc->pid); break; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5331
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> device_desc.has_hid_input_relative is TRUE if the device has HID subsystem ancestor AND the HID ancestor has input subsystem descendant. A simplified example subsystem hierarchy view for a standard USB HID input device: USB +--> HID +--> hidraw hidraw0 | +--> input input0 In this case, device_desc.has_hid_input_relative = TRUE for hidraw0. A simplified example subsystem hierarchy view for a non-input USB HID device (e.g. USB HID Thermometer): USB +--> HID +--> hidraw hidraw0 In this case, device_desc.has_hid_input_relative = FALSE for hidraw0. --- dlls/winebus.sys/bus_udev.c | 52 +++++++++++++++++++++++++++++++++++++ dlls/winebus.sys/unixlib.h | 5 ++-- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index f8c6ccb9060..fc1a5133423 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -1284,6 +1284,54 @@ static void hidraw_set_quirks(struct hidraw_device *impl, DWORD bus_type, WORD v impl->quirks |= QUIRK_DUALSENSE_BT; } +#ifdef HAS_PROPER_INPUT_HEADER +static BOOL has_hid_input_relative(struct udev_device *dev) +{ + struct udev_device *hid_parent; + struct udev_enumerate *enumerate; + BOOL retval = FALSE; + + hid_parent = udev_device_get_parent_with_subsystem_devtype(dev, "hid", NULL); + if (!hid_parent) + { + TRACE("Device '%s' does not have 'hid' subsystem parent.\n", udev_device_get_syspath(hid_parent)); + return FALSE; + } + + enumerate = udev_enumerate_new(udev_context); + if (!enumerate) + { + ERR("Failed to create udev enumeration object.\n"); + return FALSE; + } + + if (udev_enumerate_add_match_subsystem(enumerate, "input") < 0) + { + ERR("Failed to add subsystem 'input' match to enumeration.\n"); + goto out; + } + + if (udev_enumerate_add_match_parent(enumerate, hid_parent) < 0) + { + ERR("Failed to add parent '%s' match to enumeration.\n", udev_device_get_syspath(hid_parent)); + goto out; + } + + if (udev_enumerate_scan_devices(enumerate) < 0) + { + ERR("Failed to scan devices.\n"); + goto out; + } + + retval = udev_enumerate_get_list_entry(enumerate) != NULL; +out: + udev_enumerate_unref(enumerate); + enumerate = NULL; + + return retval; +} +#endif + static void udev_add_device(struct udev_device *dev, int fd) { struct device_desc desc = @@ -1313,6 +1361,10 @@ static void udev_add_device(struct udev_device *dev, int fd) get_device_subsystem_info(dev, "input", &desc, &bus); get_device_subsystem_info(dev, "usb", &desc, &bus); +#ifdef HAS_PROPER_INPUT_HEADER + desc.has_hid_input_relative = has_hid_input_relative(dev); +#endif + subsystem = udev_device_get_subsystem(dev); if (!strcmp(subsystem, "hidraw")) { diff --git a/dlls/winebus.sys/unixlib.h b/dlls/winebus.sys/unixlib.h index 80852696dd1..c228dd88769 100644 --- a/dlls/winebus.sys/unixlib.h +++ b/dlls/winebus.sys/unixlib.h @@ -39,6 +39,7 @@ struct device_desc UINT uid; BOOL is_gamepad; BOOL is_hidraw; + BOOL has_hid_input_relative; WCHAR manufacturer[MAX_PATH]; WCHAR product[MAX_PATH]; @@ -148,8 +149,8 @@ enum unix_funcs static inline const char *debugstr_device_desc(struct device_desc *desc) { if (!desc) return "(null)"; - return wine_dbg_sprintf("{vid %04x, pid %04x, version %04x, input %d, uid %08x, is_gamepad %u, is_hidraw %u}", - desc->vid, desc->pid, desc->version, desc->input, desc->uid, desc->is_gamepad, desc->is_hidraw); + return wine_dbg_sprintf("{vid %04x, pid %04x, version %04x, input %d, uid %08x, is_gamepad %u, is_hidraw %u, has_hid_input_relative %u}", + desc->vid, desc->pid, desc->version, desc->input, desc->uid, desc->is_gamepad, desc->is_hidraw, desc->has_hid_input_relative); } static inline BOOL is_xbox_gamepad(WORD vid, WORD pid) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5331
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> --- dlls/winebus.sys/main.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index b884162692f..1d3b07477c3 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -401,7 +401,7 @@ static DWORD check_bus_option(const WCHAR *option, DWORD default_value) return default_value; } -static BOOL is_hidraw_enabled(WORD vid, WORD pid) +static BOOL is_hidraw_enabled(const struct device_desc *const desc) { char buffer[FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data[1024])]; KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION *)buffer; @@ -410,19 +410,21 @@ static BOOL is_hidraw_enabled(WORD vid, WORD pid) UNICODE_STRING str; DWORD size; + if (!desc->is_hidraw) return FALSE; + if (check_bus_option(L"DisableHidraw", FALSE)) return FALSE; if (!check_bus_option(L"Enable SDL", 1) && check_bus_option(L"DisableInput", 0)) prefer_hidraw = TRUE; - if (is_dualshock4_gamepad(vid, pid)) prefer_hidraw = TRUE; - if (is_dualsense_gamepad(vid, pid)) prefer_hidraw = TRUE; + if (is_dualshock4_gamepad(desc->vid, desc->pid)) prefer_hidraw = TRUE; + if (is_dualsense_gamepad(desc->vid, desc->pid)) prefer_hidraw = TRUE; RtlInitUnicodeString(&str, L"EnableHidraw"); if (!NtQueryValueKey(driver_key, &str, KeyValuePartialInformation, info, sizeof(buffer) - sizeof(WCHAR), &size)) { - UINT len = swprintf(vidpid, ARRAY_SIZE(vidpid), L"%04X:%04X", vid, pid); + UINT len = swprintf(vidpid, ARRAY_SIZE(vidpid), L"%04X:%04X", desc->vid, desc->pid); size -= FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data[0]); tmp = (WCHAR *)info->Data; @@ -620,7 +622,7 @@ static DWORD CALLBACK bus_main_thread(void *args) const struct device_desc *desc = &event->device_created.desc; if (!desc->is_hidraw) break; - if (!is_hidraw_enabled(desc->vid, desc->pid)) + if (!is_hidraw_enabled(desc)) { WARN("ignoring hidraw device %04x:%04x\n", desc->vid, desc->pid); break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5331
From: Tuomas Räsänen <tuomas.rasanen(a)opinsys.fi> This makes hidraw backend to be used for all hidraw devices which are NOT input devices as well. To be more precise, hidraw devices are created by HID subsystem, but HID subsystem might also use other subsystems to create more devices as well. In the most common scenario, for an HID input device, HID causes input subsystem to create input event devices too. For such scenario, the behavior is not not changed. This commit just causes hidraw backend to be used for all NON-input hidraw devices. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56450 --- dlls/winebus.sys/main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 1d3b07477c3..fe470518217 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -414,6 +414,9 @@ static BOOL is_hidraw_enabled(const struct device_desc *const desc) if (check_bus_option(L"DisableHidraw", FALSE)) return FALSE; + if (!desc->has_hid_input_relative) + prefer_hidraw = TRUE; + if (!check_bus_option(L"Enable SDL", 1) && check_bus_option(L"DisableInput", 0)) prefer_hidraw = TRUE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5331
The issue is that we want to avoid creating duplicate devices, and when both an hidraw and non-hidraw devices are accessible for the same hardware device, we want to discard the hidraw device instead if it's not preferred. I've created https://gitlab.winehq.org/wine/wine/-/merge_requests/5352 instead, which passes the device HID usages information to the main loop, and prefer hidraw for anything that is not a game controller. And for hidraw devices, on Linux, we have to request the device report descriptor to get that information, and it can only be parsed on the PE side. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5331#note_65321
On Tue Mar 19 15:42:29 2024 +0000, Rémi Bernon wrote:
The issue is that we want to avoid creating duplicate devices, and when both an hidraw and non-hidraw devices are accessible for the same hardware device, we want to discard the hidraw device instead if it's not preferred. I've created https://gitlab.winehq.org/wine/wine/-/merge_requests/5352 instead, which passes the device HID usages information to the main loop, and prefer hidraw for anything that is not a game controller. And for hidraw devices, on Linux, we have to request the device report descriptor to get that information, and it can only be parsed on the PE side. Ok, I see, avoiding duplicates makes sense.
What I did not understand is why parsing must be on the PE side and we couldn't just call ``` winebus_call(device_remove, ¶ms);` ``` when hidraw device is not preferred and rely on the device subsystem hierarchy? And the fact that I did not understand does surprise me at all, this is the first time I'm poking Wine. But I am asking this genuinely to educate my self about Wine's internals. Anyways, I tested your branch and it seems to work and fix [the bug](https://bugs.winehq.org/show_bug.cgi?id=56450). I'll close this MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5331#note_65352
why parsing must be on the PE side
This is coming from the faily recent PE/unix separation introduction, which enforces the boundaries between the PE and unix world in Wine. It was previously much less strictly enforced and code could be shared more freely as there was no clear boundaries. Now the HID parser is implemented as the Windows hidparse.sys static library, and uses Windows APIs internally, which means that using it from the unix side it would fail to link many of its symbols. It could perhaps be made more portable, and the static library used from the unix side, but that would require more change and didn't seem necessary so far.
we couldn't just call `winebus_call(device_remove, ¶ms);` when hidraw device is not preferred and rely on the device subsystem hierarchy?
I'm not sure what you mean by that. It wasn't done when devices were ignored, but that was an oversight and I added a call now so that we also stop listening on ignored device events. If you mean, delay the moment we discard one of the devices up to the point where we have their report descriptors parsed already, and remove one of the duplicated devices, then it could perhaps have been done like that too, I only preferred to avoid having the devices exposed even briefly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5331#note_65355
On Tue Mar 19 15:55:41 2024 +0000, Rémi Bernon wrote:
why parsing must be on the PE side This is coming from the faily recent PE/unix separation introduction, which enforces the boundaries between the PE and unix world in Wine. It was previously much less strictly enforced and code could be shared more freely as there was no clear boundaries. Now the HID parser is implemented as the Windows hidparse.sys static library, and uses Windows APIs internally, which means that using it from the unix side it would fail to link many of its symbols. It could perhaps be made more portable, and the static library used from the unix side, but that would require more change and didn't seem necessary so far. we couldn't just call `winebus_call(device_remove, ¶ms);` when hidraw device is not preferred and rely on the device subsystem hierarchy? I'm not sure what you mean by that. It wasn't done when devices were ignored, but that was an oversight and I added a call now so that we also stop listening on ignored device events. If you mean, delay the moment we discard one of the devices up to the point where we have their report descriptors parsed already, and remove one of the duplicated devices, then it could perhaps have been done like that too, I only preferred to avoid having the devices exposed even briefly. The question was badly formed, mostly because of my still quite fuzzy/unclear picture of architecture and all components, but let me try rephrase it:
Because in the context of UDEV bus, when a device is created, the report descriptor is **already** parsed by hid subsystem, which in turn has created necessary devices, so why do we need to parse report descriptors and read the usage pages when we could just rely on hid subsystem's capability on doing so? If the HID device is an input device, hid asks input subsystem to a create an event device for it. If it's not, then an event device is not created. And now that I asked that, is the reason for not relying on host's hid subsystem in this regard, that we want to be in charge how usage pages are interpreted? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5331#note_65367
This merge request was closed by Tuomas Räsänen. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5331
participants (2)
-
Rémi Bernon -
Tuomas Räsänen