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

[Ivo Ivanov (＠logos128)]

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.

Additionally fixes an issue with the device revision (bcdDevice)
not being extracted from the usb subsystem, causing the bcdHID
to be extracted in error.

Also 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

[PATCH 1/1] 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.

Additionally fixes an issue with the device revision (bcdDevice)
not being extracted from the usb subsystem, causing the bcdHID
to be extracted in error.

Also 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 | 49 ++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index 6ec0fbd356f..71988f815dd 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -1138,14 +1138,15 @@ 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_type)
 {
     struct udev_device *parent = NULL;
     const char *ptr, *next, *tmp;
     char buffer[MAX_PATH];
+    UINT version;

-    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")))
     {
@@ -1174,28 +1175,36 @@ static void get_device_subsystem_info(struct udev_device *dev, char const *subsy
             }
             if (!strncmp(ptr, "HID_ID=", 7))
             {
-                if (*bus || desc->vid || desc->pid) continue;
-                sscanf(ptr, "HID_ID=%x:%x:%x\n", bus, &desc->vid, &desc->pid);
+                if (*bus_type || desc->vid || desc->pid) continue;
+                sscanf(ptr, "HID_ID=%x:%x:%x\n", bus_type, &desc->vid, &desc->pid);
             }
-            if (!strncmp(ptr, "PRODUCT=", 8) && *bus != BUS_BLUETOOTH)
+            if (!strncmp(ptr, "PRODUCT=", 8))
             {
                 if (desc->version) continue;
                 if (!strcmp(subsystem, "usb"))
-                    sscanf(ptr, "PRODUCT=%x/%x/%x\n", &desc->vid, &desc->pid, &desc->version);
-                else
-                    sscanf(ptr, "PRODUCT=%x/%x/%x/%x\n", bus, &desc->vid, &desc->pid, &desc->version);
+                {
+                    if (*bus_type != BUS_BLUETOOTH)
+                        sscanf(ptr, "PRODUCT=%x/%x/%x\n", &desc->vid, &desc->pid, &desc->version);
+                }
+                else if (sscanf(ptr, "PRODUCT=%x/%x/%x/%x\n", bus_type, &desc->vid, &desc->pid, &version) == 4 &&
+                         *bus_type == BUS_BLUETOOTH)
+                    desc->version = version;
             }
         }
     }

-    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_type != BUS_BLUETOOTH)
+    {
+        if ((tmp = udev_device_get_sysattr_value(parent, "manufacturer")))
+            ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->manufacturer, ARRAY_SIZE(desc->manufacturer));
+
+        if ((tmp = udev_device_get_sysattr_value(parent, "product")))
+            ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->product, ARRAY_SIZE(desc->product));

-    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, "serial")))
+            ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->serialnumber, ARRAY_SIZE(desc->serialnumber));

-    if (!desc->serialnumber[0] && (tmp = udev_device_get_sysattr_value(dev, "serial")))
-        ntdll_umbstowcs(tmp, strlen(tmp) + 1, desc->serialnumber, ARRAY_SIZE(desc->serialnumber));
+    }
 }

static void udev_add_device(struct udev_device *dev, int fd)
@@ -1207,7 +1216,7 @@ static void udev_add_device(struct udev_device *dev, int fd)
     struct base_device *impl;
     const char *subsystem;
     const char *devnode;
-    int bus = 0;
+    int bus_type = 0;

if (!(devnode = udev_device_get_devnode(dev)))
     {
@@ -1223,10 +1232,10 @@ 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);
-    if (bus == BUS_BLUETOOTH) desc.is_bluetooth = TRUE;
+    get_device_subsystem_info(dev, "hid", NULL, &desc, &bus_type);
+    get_device_subsystem_info(dev, "input", NULL, &desc, &bus_type);
+    get_device_subsystem_info(dev, "usb", "usb_device", &desc, &bus_type);
+    if (bus_type == BUS_BLUETOOTH) desc.is_bluetooth = TRUE;

subsystem = udev_device_get_subsystem(dev);
     if (!strcmp(subsystem, "hidraw"))

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

[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 tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=147717

Your paranoid android.

=== debian11b (64 bit WoW report) ===

ddraw:
ddraw4.c:3969: Test failed: Expected message 0x5, but didn't receive it.

[Rémi Bernon (＠rbernon)]

Thanks, looks good. I've opened (https://gitlab.winehq.org/wine/wine/-/merge_requests/6276) with the changes split and a couple of adjustments to some make future changes easier.

I think we could parse additional information from the "input" subsystem, as well as (possibly) use the PHYS strings to implement device container IDs (https://gitlab.winehq.org/wine/wine/-/merge_requests/535) which are needed for some applications to match between controllers and audio devices with the Dualshock controllers. As far as I can see pulseaudio exposes the same string in its device names.

[Ivo Ivanov (＠logos128)]

On Wed Aug 14 13:45:25 2024 +0000, Rémi Bernon wrote:
> Thanks, looks good. I've opened
> (https://gitlab.winehq.org/wine/wine/-/merge_requests/6276) with the
> changes split and a couple of adjustments to some make future changes easier.
> I think we could parse additional information from the "input"
> subsystem, as well as (possibly) use the PHYS strings to implement
> device container IDs
> (https://gitlab.winehq.org/wine/wine/-/merge_requests/535) which are
> needed for some applications to match between controllers and audio
> devices with the Dualshock controllers. As far as I can see pulseaudio
> exposes the same string in its device names.

I did this as part of a larger patchset implementing IOCTL_HID_GET_INDEXED_STRING in hidclass.sys -> winebus.sys. Have been using it for a year or so without any problems. Still plan to push an MR for the remaining part when time permits.
Just saw the bug reports about the product/manufacturer string issues and decided to push this patch separately.

Many apps/games need these strings to match exactly (esp. product name), to properly recognize the device and apply predefined configuration, etc.

[Rémi Bernon (＠rbernon)]

On Wed Aug 14 13:45:25 2024 +0000, Ivo Ivanov wrote:
> I did this as part of a larger patchset implementing
> IOCTL_HID_GET_INDEXED_STRING in hidclass.sys -> winebus.sys. Have been
> using it for a year or so without any problems. Still plan to push an MR
> for the remaining part when time permits.
> Just saw the bug reports about the product/manufacturer string issues
> and decided to push this patch separately.
> Many apps/games need these strings to match exactly (esp. product name),
> to properly recognize the device and apply predefined configuration, etc.

Fwiw in several cases the names simply do not match between the Linux driver and the Windows drivers. We forcefully override the device names for such cases in hidclass.sys.

[Ivo Ivanov (＠logos128)]

On Wed Aug 14 13:52:34 2024 +0000, Rémi Bernon wrote:
> Fwiw in several cases the names simply do not match between the Linux
> driver and the Windows drivers. We forcefully override the device names
> for such cases in hidclass.sys.

Thanks. Didn't know that.

I use the 'hidraw' backend for all my devices, incl. force feedback wheel, shifter, pedals, joysticks, etc., and hopefully this way the strings match better between Linux and Windows. So far didn't spot any issues. The devices get properly recognized/configured by apps and games.

[PATCH 0/1] MR6268: winebus.sys: Add devtype parameter to get_device_subsystem_info(). - closed

[Rémi Bernon (＠rbernon)]

This merge request was closed by Rémi Bernon.