[PATCH 0/3] MR6276: winebus.sys: Improve udev device information parsing.
From: Ivo Ivanov <logos128(a)gmail.com> Narrow the search for a parent subsystem to a specific devtype. For the usb subsystem in particular, only the "usb_device" devtype includes all the necessary attributes, so no need to iterate through the usb interfaces. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54838 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56987 --- dlls/winebus.sys/bus_udev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 6ec0fbd356f..5ff152fbe05 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -1138,14 +1138,14 @@ static const struct hid_device_vtbl lnxev_device_vtbl = }; #endif /* HAS_PROPER_INPUT_HEADER */ -static void get_device_subsystem_info(struct udev_device *dev, char const *subsystem, struct device_desc *desc, - int *bus) +static void get_device_subsystem_info(struct udev_device *dev, const char *subsystem, const char *devtype, + struct device_desc *desc, int *bus) { struct udev_device *parent = NULL; const char *ptr, *next, *tmp; char buffer[MAX_PATH]; - if (!(parent = udev_device_get_parent_with_subsystem_devtype(dev, subsystem, NULL))) return; + if (!(parent = udev_device_get_parent_with_subsystem_devtype(dev, subsystem, devtype))) return; if ((next = udev_device_get_sysattr_value(parent, "uevent"))) { @@ -1223,9 +1223,9 @@ static void udev_add_device(struct udev_device *dev, int fd) TRACE("udev %s syspath %s\n", debugstr_a(devnode), udev_device_get_syspath(dev)); - get_device_subsystem_info(dev, "hid", &desc, &bus); - get_device_subsystem_info(dev, "input", &desc, &bus); - get_device_subsystem_info(dev, "usb", &desc, &bus); + get_device_subsystem_info(dev, "hid", NULL, &desc, &bus); + get_device_subsystem_info(dev, "input", NULL, &desc, &bus); + get_device_subsystem_info(dev, "usb", "usb_device", &desc, &bus); if (bus == BUS_BLUETOOTH) desc.is_bluetooth = TRUE; subsystem = udev_device_get_subsystem(dev); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6276
From: Rémi Bernon <rbernon(a)codeweavers.com> Fixes an issue with the device revision (bcdDevice) not being extracted from the usb subsystem, causing the bcdHID to be extracted in error. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54838 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56987 --- dlls/winebus.sys/bus_udev.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 5ff152fbe05..f42470d4806 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -1177,13 +1177,23 @@ static void get_device_subsystem_info(struct udev_device *dev, const char *subsy if (*bus || desc->vid || desc->pid) continue; sscanf(ptr, "HID_ID=%x:%x:%x\n", bus, &desc->vid, &desc->pid); } - if (!strncmp(ptr, "PRODUCT=", 8) && *bus != BUS_BLUETOOTH) + + if (!strcmp(subsystem, "input")) { - if (desc->version) continue; - if (!strcmp(subsystem, "usb")) - sscanf(ptr, "PRODUCT=%x/%x/%x\n", &desc->vid, &desc->pid, &desc->version); - else + if (!strncmp(ptr, "PRODUCT=", 8)) + { + if (desc->version) continue; sscanf(ptr, "PRODUCT=%x/%x/%x/%x\n", bus, &desc->vid, &desc->pid, &desc->version); + } + } + + if (!strcmp(subsystem, "usb") && *bus != BUS_BLUETOOTH) + { + if (!strncmp(ptr, "PRODUCT=", 8)) + { + if (desc->version) continue; + sscanf(ptr, "PRODUCT=%x/%x/%x\n", &desc->vid, &desc->pid, &desc->version); + } } } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6276
From: Ivo Ivanov <logos128(a)gmail.com> Fixes an issue with the product name including the manufacturer in the same string. Both attributes are correctly extracted from the usb subsystem. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54838 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56987 --- dlls/winebus.sys/bus_udev.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index f42470d4806..c928e738aeb 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -1198,14 +1198,17 @@ static void get_device_subsystem_info(struct udev_device *dev, const char *subsy } } - if (!desc->manufacturer[0] && (tmp = udev_device_get_sysattr_value(dev, "manufacturer"))) - ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->manufacturer, ARRAY_SIZE(desc->manufacturer)); + if (!strcmp(subsystem, "usb") && *bus != BUS_BLUETOOTH) + { + if ((tmp = udev_device_get_sysattr_value(parent, "manufacturer"))) + ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->manufacturer, ARRAY_SIZE(desc->manufacturer)); - if (!desc->product[0] && (tmp = udev_device_get_sysattr_value(dev, "product"))) - ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->product, ARRAY_SIZE(desc->product)); + if ((tmp = udev_device_get_sysattr_value(parent, "product"))) + ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->product, ARRAY_SIZE(desc->product)); - if (!desc->serialnumber[0] && (tmp = udev_device_get_sysattr_value(dev, "serial"))) - ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->serialnumber, ARRAY_SIZE(desc->serialnumber)); + if ((tmp = udev_device_get_sysattr_value(parent, "serial"))) + ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->serialnumber, ARRAY_SIZE(desc->serialnumber)); + } } static void udev_add_device(struct udev_device *dev, int fd) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6276
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147736 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winebus.sys/bus_udev.c:1177 Task: Patch failed to apply === debian11b (build log) === error: patch failed: dlls/winebus.sys/bus_udev.c:1177 Task: Patch failed to apply
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6276
Ivo Ivanov (@logos128) commented about dlls/winebus.sys/bus_udev.c:
if (*bus || desc->vid || desc->pid) continue; sscanf(ptr, "HID_ID=%x:%x:%x\n", bus, &desc->vid, &desc->pid); } - if (!strncmp(ptr, "PRODUCT=", 8) && *bus != BUS_BLUETOOTH) + + if (!strcmp(subsystem, "input")) { - if (desc->version) continue; - if (!strcmp(subsystem, "usb")) - sscanf(ptr, "PRODUCT=%x/%x/%x\n", &desc->vid, &desc->pid, &desc->version); - else + if (!strncmp(ptr, "PRODUCT=", 8)) + { + if (desc->version) continue; sscanf(ptr, "PRODUCT=%x/%x/%x/%x\n", bus, &desc->vid, &desc->pid, &desc->version);
I've found that the 'input' subsystem returns bcdHID here instead of bcdDevice for device revision. Since we parse the 'input' subsystem first, it extracts this value for desc->version, so when the 'usb' subsystem gets parsed it skips the extraction of the correct value. For this reason I've added the check for *bus == BUS_BLUETOOTH here, so it won't extract it from here, and allows it to be extracted from the 'usb' subsystem in the next turn. Maybe an elegant solution here is to just parse the 'usb' subsystem before the 'input' in udev_add_device(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6276#note_78666
Maybe an elegant solution here is to just parse the 'usb' subsystem before the 'input' in udev_add_device().
Possibly, I'm not completely sure what amount of variation in term of available subsystems we can have. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6276#note_78676
On Wed Aug 14 14:42:10 2024 +0000, Rémi Bernon wrote:
Maybe an elegant solution here is to just parse the 'usb' subsystem before the 'input' in udev_add_device(). Possibly, I'm not completely sure what amount of variation in term of available subsystems we can have. Yes, it's better if they work properly regardless of the sequence.
Also the 'input' subsystem extracts 'bus' here for a second chance after 'HID_ID' above. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6276#note_78678
Also the 'input' subsystem extracts 'bus' here for a second chance after 'HID_ID' above.
Yes, I think this matters, so reordering would break that. What about just letting "usb" subsystem parse PRODUCT= again, still keeping the BT bus guard to avoid parsing the BT dongle IDs, regardless of whether a device version was previously found already? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6276#note_78679
What about just letting "usb" subsystem parse PRODUCT= again, still keeping the BT bus guard to avoid parsing the BT dongle IDs, regardless of whether a device version was previously found already?
Yes, this seems like a good solution. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6276#note_78680
participants (4)
-
Ivo Ivanov -
Ivo Ivanov (＠logos128)
-
Marvin -
Rémi Bernon