New subject: [PATCH v6] winebus.sys: Watch for hid raw device addition and removal

29 Sep 2016

On 29.09.2016 20:48, Aric Stewart wrote:
...
v2: Correct poll timeout
    Style changes
v3: vtable style instead of a callback
v4: Suggestions from Sebastian Lackner
v5: Fixing handling of other native in the compare function
v6: Tweaks suggested by Sebastian Lackner
Includes an implementation of a common bus_remove_hid_device
Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
---
 dlls/winebus.sys/bus.h      |  13 +++++-
 dlls/winebus.sys/bus_udev.c | 106 +++++++++++++++++++++++++++++++++++++++++++-
 dlls/winebus.sys/main.c     |  64 +++++++++++++++++++++-----
 3 files changed, 169 insertions(+), 14 deletions(-)
v6-0001-winebus.sys-Watch-for-hid-raw-device-addition-and-r.txt

diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
index dcb50f2..719d8fd 100644
--- a/dlls/winebus.sys/bus.h
+++ b/dlls/winebus.sys/bus.h
@@ -19,8 +19,17 @@
 /* Busses */
 NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path) DECLSPEC_HIDDEN;
+/* Native device function table */
+typedef struct {
+    int(*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev);
+} platform_vtbl;
+
+void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
+DEVICE_OBJECT *find_hid_device(const platform_vtbl* vtbl, void *device) DECLSPEC_HIDDEN;
+
 /* HID Plug and Play Bus */
 NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
-DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
+DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WORD vid,
                                      WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
-                                     const GUID *class) DECLSPEC_HIDDEN;
+                                     const GUID *class, const platform_vtbl* vtbl, DWORD platform_data_size) DECLSPEC_HIDDEN;
+void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
index 1525861..b7995bc 100644
--- a/dlls/winebus.sys/bus_udev.c
+++ b/dlls/winebus.sys/bus_udev.c
@@ -26,6 +26,11 @@
 #ifdef HAVE_UNISTD_H
 # include <unistd.h>
 #endif
+
+#ifdef HAVE_SYS_POLL_H
+# include <sys/poll.h>
+#endif
+
 #ifdef HAVE_LIBUDEV_H
 # include <libudev.h>
 #endif
@@ -55,6 +60,10 @@ static const WCHAR hidraw_busidW[] = {'H','I','D','R','A','W',0};
 #include "initguid.h"
 DEFINE_GUID(GUID_DEVCLASS_HIDRAW, 0x3def44ad,0x242e,0x46e5,0x82,0x6d,0x70,0x72,0x13,0xf3,0xaa,0x81);
+struct platform_private {
+    struct udev_device *udev_device;
+};
+
 static DWORD get_sysattr_dword(struct udev_device *dev, const char *sysattr, int base)
 {
     const char *attr = udev_device_get_sysattr_value(dev, sysattr);
@@ -82,6 +91,17 @@ static WCHAR *get_sysattr_string(struct udev_device *dev, const char *sysattr)
     return dst;
 }
+static int compare_platform_device(DEVICE_OBJECT *device, void *platform_dev)
+{
+   struct udev_device *dev1 = ((struct platform_private*)get_platform_private(device))->udev_device;
There is a whitespace issue in the line above. Also, I would suggest to use a wrapper like for COM
interfaces to avoid having this conversion at multiple places. For example:

static inline platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device) { ... }
...
+    struct udev_device *dev2 = platform_dev;
+    return (strcmp(udev_device_get_syspath(dev1), udev_device_get_syspath(dev2)));
+}
+
+static const platform_vtbl hidraw_vtbl = {
+    compare_platform_device,
+};
+
 static void try_add_device(struct udev_device *dev)
 {
     DWORD vid = 0, pid = 0, version = 0;
@@ -117,12 +137,16 @@ static void try_add_device(struct udev_device *dev)
     subsystem = udev_device_get_subsystem(dev);
     if (strcmp(subsystem, "hidraw") == 0)
     {
-        device = bus_create_hid_device(udev_driver_obj, hidraw_busidW, dev, vid, pid,
-                                       version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW);
+        device = bus_create_hid_device(udev_driver_obj, hidraw_busidW, vid, pid,
+                                       version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW, &hidraw_vtbl, sizeof(struct platform_private));
     }
if (device)
+    {
+        ((struct platform_private*)get_platform_private(device))->udev_device = dev;
         udev_device_ref(dev);
udev_device_ref returns the device, so you could also do the assignment and incref in one line if you prefer. ;)
Not really an issue though.
...
+        IoInvalidateDeviceRelations(device, BusRelations);
+    }
     else
         WARN("Ignoring device %s with subsystem %s\n", debugstr_a(devnode), subsystem);
@@ -164,6 +188,77 @@ static void build_initial_deviceset(void)
     udev_enumerate_unref(enumerate);
 }
+/* This is our main even loop for reading devices */
+static DWORD CALLBACK deviceloop_thread(void *args)
+{
+    struct pollfd plfds[1];
+    struct udev_monitor *monitor = NULL;
+
+    monitor = udev_monitor_new_from_netlink(udev_context, "udev");
+    if (!monitor)
+    {
+        WARN("Unable to get udev monitor, device insertion and removal cannot be tracked\n");
+        goto exit;
+    }
+    if (udev_monitor_filter_add_match_subsystem_devtype(monitor, "hidraw", NULL) < 0)
+    {
+        WARN("Failed to add 'hidraw' subsystem to monitor\n");
+        goto exit;
+    }
+
+    if (udev_monitor_enable_receiving(monitor) < 0)
+    {
+        WARN("Failed to start monitoring\n");
+        goto exit;
+    }
+
+    plfds[0].fd = udev_monitor_get_fd(monitor);
+    plfds[0].events = POLLIN;
+    if (plfds[0].fd < 0)
+    {
+        WARN("Failed to get monitor fd\n");
+        goto exit;
+    }
+    /* Run the event loop */
+    while (1)
+    {
+        struct udev_device *dev;
+        const char* action;
+
+        if (poll(plfds, 1, -1) <= 0) continue;
+
+        TRACE("udev reports device changes\n");
+        dev = udev_monitor_receive_device(monitor);
+        if (!dev)
+        {
+            FIXME("Failed to get device that has changed\n");
+            continue;
+        }
+        action = udev_device_get_action(dev);
+        if (!action)
+            WARN("No action recieved\n");
+        else if (strcmp(action, "add") == 0)
+            try_add_device(dev);
+        else if (strcmp(action, "remove") == 0)
+        {
+            DEVICE_OBJECT *device = find_hid_device(&hidraw_vtbl, dev);
+            if (device)
+            {
+                struct udev_device *original = ((struct platform_private*)get_platform_private(device))->udev_device;
+                bus_remove_hid_device(device);
+                udev_device_unref(original);
+            }
+        }
+        udev_device_unref(dev);
+    }
+
+exit:
+    TRACE("Monitor thread exiting\n");
+    if (monitor)
+        udev_monitor_unref(monitor);
+    return 1;
+}
+
 NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry_path)
 {
     TRACE("(%p, %s)\n", driver, debugstr_w(registry_path->Buffer));
@@ -178,6 +273,13 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry
     driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
build_initial_deviceset();
+
+    if (!CreateThread(NULL, 0, deviceloop_thread, NULL, 0, NULL))
+    {
+        ERR("Unable to create udev device thread\n");
+        return STATUS_UNSUCCESSFUL;
Wouldn't it make sense to proceed anyway? The initial device set could still
be working properly (and, you are not cleaning up already added drivers here).
...
+    }
+
     return STATUS_SUCCESS;
 }
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
index 1364eab..1cb8031 100644
--- a/dlls/winebus.sys/main.c
+++ b/dlls/winebus.sys/main.c
@@ -47,13 +47,15 @@ struct pnp_device
struct device_extension
 {
-    void *native;  /* Must be the first member of the structure */
-
     WORD vid, pid;
     DWORD uid, version, index;
     BOOL is_gamepad;
     WCHAR *serial;
     const WCHAR *busid;  /* Expected to be a static constant */
+    struct pnp_device *pnp_device;
+    const platform_vtbl *vtbl;
+
+    BYTE platform_private[1];
 };
static CRITICAL_SECTION device_list_cs;
@@ -157,11 +159,11 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device)
     return dst;
 }
-DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid,
+DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, WORD vid,
                                      WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad,
-                                     const GUID *class)
+                                     const GUID *class, const platform_vtbl *vtbl, DWORD platform_data_size)
 {
-    static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p',0};
+    static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p','#','%','x',0};
     struct device_extension *ext;
     struct pnp_device *pnp_dev;
     DEVICE_OBJECT *device;
@@ -169,16 +171,18 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
     WCHAR dev_name[256];
     HDEVINFO devinfo;
     NTSTATUS status;
+    DWORD length;
-    TRACE("(%p, %s, %p, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW), native,
+    TRACE("(%p, %s, %04x, %04x, %u, %u, %s, %u, %s)\n", driver, debugstr_w(busidW),
           vid, pid, version, uid, debugstr_w(serialW), is_gamepad, debugstr_guid(class));
You forgot to add the vtbl and platform_data_size parameters to the TRACE.
...
if (!(pnp_dev = HeapAlloc(GetProcessHeap(), 0, sizeof(*pnp_dev))))
         return NULL;
-    sprintfW(dev_name, device_name_fmtW, busidW, native);
+    sprintfW(dev_name, device_name_fmtW, busidW, pnp_dev, GetTickCount());
Do you think the memory pointer (pnp_dev) alone is not sufficient? If not, it was already
a problem before because "native" is also just a memory address for UDEV.
...
RtlInitUnicodeString(&nameW, dev_name);
-    status = IoCreateDevice(driver, sizeof(*ext), &nameW, 0, 0, FALSE, &device);
+    length = FIELD_OFFSET(struct device_extension, platform_private[platform_data_size]);
+    status = IoCreateDevice(driver, length, &nameW, 0, 0, FALSE, &device);
     if (status)
     {
         FIXME("failed to create device error %x\n", status);
@@ -190,7 +194,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
/* fill out device_extension struct */
     ext = (struct device_extension *)device->DeviceExtension;
-    ext->native     = native;
     ext->vid        = vid;
     ext->pid        = pid;
     ext->uid        = uid;
@@ -199,6 +202,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
     ext->is_gamepad = is_gamepad;
     ext->serial     = strdupW(serialW);
     ext->busid      = busidW;
+    ext->pnp_device = pnp_dev;
+    ext->vtbl       = vtbl;
/* add to list of pnp devices */
     pnp_dev->device = device;
@@ -226,7 +231,6 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
     else
         ERR("failed to get ClassDevs: %x\n", GetLastError());
-    IoInvalidateDeviceRelations(device, BusRelations);
     return device;
 }
@@ -265,6 +269,46 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp)
     return status;
 }
+void *get_platform_private(DEVICE_OBJECT *device)
+{
+    struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
+    return ext->platform_private;
+}
+
+DEVICE_OBJECT *find_hid_device(const platform_vtbl* vtbl, void *device)
+{
+    struct pnp_device *dev, *ptr;
+    DEVICE_OBJECT *ret = NULL;
+
+    EnterCriticalSection(&device_list_cs);
+    LIST_FOR_EACH_ENTRY_SAFE(dev, ptr, &pnp_devset, struct pnp_device, entry)
You do no longer need the _SAFE enumeration here because the deletion is done separately.
...
+    {
+        struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension;
+        if ((vtbl == ext->vtbl) &&
+            (ext->vtbl->compare_platform_device(dev->device, device) == 0))
+        {
+            TRACE("Found as device %p\n", dev->device);
+            ret = dev->device;
+            break;
+        }
+    }
+    LeaveCriticalSection(&device_list_cs);
+    return ret;
+}
+
+void bus_remove_hid_device(DEVICE_OBJECT *device)
+{
+    struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
+    TRACE("Remove hid device %p\n", device);
+    EnterCriticalSection(&device_list_cs);
+    list_remove(&ext->pnp_device->entry);
+    LeaveCriticalSection(&device_list_cs);
+    IoInvalidateDeviceRelations(device, RemovalRelations);
+    HeapFree(GetProcessHeap(), 0, ext->serial);
+    HeapFree(GetProcessHeap(), 0, ext->pnp_device);
+    IoDeleteDevice(device);
+}
+
 NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp)
 {
     NTSTATUS status = irp->IoStatus.u.Status;


    

Re: [PATCH v6] winebus.sys: Watch for hid raw device addition and removal

Sebastian Lackner

Aric Stewart

Alexandre Julliard

tags

participants (3)