[PATCH 0/3] MR6276: winebus.sys: Improve udev device information parsing.

[Rémi Bernon (＠rbernon)]

Supersedes https://gitlab.winehq.org/wine/wine/-/merge_requests/6268.

[PATCH 1/3] winebus.sys: Add devtype parameter to get_device_subsystem_info().

[Ivo Ivanov]

From: Ivo Ivanov

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);

[PATCH 2/3] winebus.sys: Always parse uevent PRODUCT= line on input subsystem.

[Rémi Bernon]

From: Rémi Bernon

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);
+                }
             }
         }
     }

[PATCH 3/3] winebus.sys: Read vendor/product/serial strings from the usb subsystem.

[Ivo Ivanov]

From: Ivo Ivanov

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)

[PATCH 3/3] winebus.sys: Read vendor/product/serial strings from the usb subsystem.

[Marvin]

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

[PATCH 0/3] MR6276: winebus.sys: Improve udev device information parsing. - approved

[Rémi Bernon (＠rbernon)]

This merge request was approved by Rémi Bernon.

[Ivo Ivanov (＠logos128)]

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().

[PATCH 0/3] MR6276: Draft: winebus.sys: Improve udev device information parsing.

[Rémi Bernon (＠rbernon)]

> 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.

[PATCH 0/3] MR6276: Draft: winebus.sys: Improve udev device information parsing.

[Ivo Ivanov (＠logos128)]

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.

[PATCH 0/3] MR6276: Draft: winebus.sys: Improve udev device information parsing.

[Rémi Bernon (＠rbernon)]

> 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?

[PATCH 0/3] MR6276: Draft: winebus.sys: Improve udev device information parsing.

[Ivo Ivanov (＠logos128)]

> 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.