Re: [PATCH v3] winebus.sys: Watch for hid raw device addition and removal
On 19.09.2016 15:35, Aric Stewart wrote:
v2: Correct poll timeout Style changes v3: vtable style instead of a callback
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/winebus.sys/bus.h | 11 +++++- dlls/winebus.sys/bus_udev.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- dlls/winebus.sys/main.c | 52 ++++++++++++++++++++++++- 3 files changed, 152 insertions(+), 3 deletions(-)
Would you mind sharing some of the follow up patches, so that I can check if the vtbl solution really looks better than the previous approach?
v3-0004-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..cf10e80 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_native_device)(DEVICE_OBJECT *device, DEVICE_OBJECT *other); +} platform_vtbl; + +void *get_native_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; +DEVICE_OBJECT *find_hid_device(const WCHAR* busidW, void *native) 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, WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, - const GUID *class) DECLSPEC_HIDDEN; + const GUID *class, platform_vtbl* vtbl) 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..35b198a 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 @@ -82,6 +87,17 @@ static WCHAR *get_sysattr_string(struct udev_device *dev, const char *sysattr) return dst; }
+static int compare_native_device(DEVICE_OBJECT *device, DEVICE_OBJECT *other) +{ + struct udev_device *dev1 = get_native_device(device); + struct udev_device *dev2 = get_native_device(other); + return (strcmp(udev_device_get_syspath(dev1), udev_device_get_syspath(dev2))); +} + +static platform_vtbl hidraw_vtbl = {
This should be made const.
+ compare_native_device, +}; + static void try_add_device(struct udev_device *dev) { DWORD vid = 0, pid = 0, version = 0; @@ -118,7 +134,7 @@ static void try_add_device(struct udev_device *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); + version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW, &hidraw_vtbl); }
if (device) @@ -164,6 +180,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_busidW, dev); + if (device) + { + struct udev_device *original = get_native_device(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 +265,9 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
build_initial_deviceset(); + + CreateThread(NULL, 0, deviceloop_thread, NULL, 0, NULL);
It would be better to check the result here (at least to print an ERR or something like that).
+ return STATUS_SUCCESS; }
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 1364eab..c7418e3 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -54,6 +54,7 @@ struct device_extension BOOL is_gamepad; WCHAR *serial; const WCHAR *busid; /* Expected to be a static constant */ + platform_vtbl *vtbl; };
static CRITICAL_SECTION device_list_cs; @@ -159,7 +160,7 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device)
DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, - const GUID *class) + const GUID *class, platform_vtbl *vtbl) { static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p',0}; struct device_extension *ext; @@ -199,6 +200,7 @@ 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->vtbl = vtbl;
/* add to list of pnp devices */ pnp_dev->device = device; @@ -265,6 +267,54 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp) return status; }
+void *get_native_device(DEVICE_OBJECT *device) +{ + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; + return ext->native; +} + +DEVICE_OBJECT *find_hid_device(const WCHAR* busidW, void *native) +{ + 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) + { + struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension; + if ((strcmpW(busidW, ext->busid) == 0) && + (ext->vtbl->compare_native_device(dev->device, native) == 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 pnp_device *dev, *ptr; + TRACE("Remove hid device %p\n", device); + EnterCriticalSection(&device_list_cs); + LIST_FOR_EACH_ENTRY_SAFE(dev, ptr, &pnp_devset, struct pnp_device, entry) + { + struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension; + if (device == dev->device) + { + list_remove(&dev->entry); + IoInvalidateDeviceRelations(dev->device, RemovalRelations); + HeapFree(GetProcessHeap(), 0, ext->serial); + IoDeleteDevice(dev->device); + HeapFree(GetProcessHeap(), 0, dev); + break; + } + } + LeaveCriticalSection(&device_list_cs); +} + NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) { NTSTATUS status = irp->IoStatus.u.Status;
On 9/21/16 5:59 AM, Sebastian Lackner wrote:
On 19.09.2016 15:35, Aric Stewart wrote:
v2: Correct poll timeout Style changes v3: vtable style instead of a callback
Signed-off-by: Aric Stewart <aric(a)codeweavers.com> --- dlls/winebus.sys/bus.h | 11 +++++- dlls/winebus.sys/bus_udev.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- dlls/winebus.sys/main.c | 52 ++++++++++++++++++++++++- 3 files changed, 152 insertions(+), 3 deletions(-)
Would you mind sharing some of the follow up patches, so that I can check if the vtbl solution really looks better than the previous approach?
Sure, Here is the full patchset for hidraw. https://www.codeweavers.com/xfer/aric/HID/Hid_Patches_20160921.tgz access-code: sLcm7EZ7 I did not include the iohid and linux event patches in that given set as I know they still need cleaning up and I am working on that. Also many of the later patches probably still need some cleaning up. -aric
v3-0004-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..cf10e80 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_native_device)(DEVICE_OBJECT *device, DEVICE_OBJECT *other); +} platform_vtbl; + +void *get_native_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN; +DEVICE_OBJECT *find_hid_device(const WCHAR* busidW, void *native) 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, WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, - const GUID *class) DECLSPEC_HIDDEN; + const GUID *class, platform_vtbl* vtbl) 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..35b198a 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 @@ -82,6 +87,17 @@ static WCHAR *get_sysattr_string(struct udev_device *dev, const char *sysattr) return dst; }
+static int compare_native_device(DEVICE_OBJECT *device, DEVICE_OBJECT *other) +{ + struct udev_device *dev1 = get_native_device(device); + struct udev_device *dev2 = get_native_device(other); + return (strcmp(udev_device_get_syspath(dev1), udev_device_get_syspath(dev2))); +} + +static platform_vtbl hidraw_vtbl = {
This should be made const.
+ compare_native_device, +}; + static void try_add_device(struct udev_device *dev) { DWORD vid = 0, pid = 0, version = 0; @@ -118,7 +134,7 @@ static void try_add_device(struct udev_device *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); + version, 0, serial, FALSE, &GUID_DEVCLASS_HIDRAW, &hidraw_vtbl); }
if (device) @@ -164,6 +180,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_busidW, dev); + if (device) + { + struct udev_device *original = get_native_device(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 +265,9 @@ NTSTATUS WINAPI udev_driver_init(DRIVER_OBJECT *driver, UNICODE_STRING *registry driver->MajorFunction[IRP_MJ_PNP] = common_pnp_dispatch;
build_initial_deviceset(); + + CreateThread(NULL, 0, deviceloop_thread, NULL, 0, NULL);
It would be better to check the result here (at least to print an ERR or something like that).
+ return STATUS_SUCCESS; }
diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 1364eab..c7418e3 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -54,6 +54,7 @@ struct device_extension BOOL is_gamepad; WCHAR *serial; const WCHAR *busid; /* Expected to be a static constant */ + platform_vtbl *vtbl; };
static CRITICAL_SECTION device_list_cs; @@ -159,7 +160,7 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device)
DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW, void *native, WORD vid, WORD pid, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, - const GUID *class) + const GUID *class, platform_vtbl *vtbl) { static const WCHAR device_name_fmtW[] = {'\\','D','e','v','i','c','e','\\','%','s','#','%','p',0}; struct device_extension *ext; @@ -199,6 +200,7 @@ 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->vtbl = vtbl;
/* add to list of pnp devices */ pnp_dev->device = device; @@ -265,6 +267,54 @@ static NTSTATUS handle_IRP_MN_QUERY_ID(DEVICE_OBJECT *device, IRP *irp) return status; }
+void *get_native_device(DEVICE_OBJECT *device) +{ + struct device_extension *ext = (struct device_extension*)device->DeviceExtension; + return ext->native; +} + +DEVICE_OBJECT *find_hid_device(const WCHAR* busidW, void *native) +{ + 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) + { + struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension; + if ((strcmpW(busidW, ext->busid) == 0) && + (ext->vtbl->compare_native_device(dev->device, native) == 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 pnp_device *dev, *ptr; + TRACE("Remove hid device %p\n", device); + EnterCriticalSection(&device_list_cs); + LIST_FOR_EACH_ENTRY_SAFE(dev, ptr, &pnp_devset, struct pnp_device, entry) + { + struct device_extension *ext = (struct device_extension*)dev->device->DeviceExtension; + if (device == dev->device) + { + list_remove(&dev->entry); + IoInvalidateDeviceRelations(dev->device, RemovalRelations); + HeapFree(GetProcessHeap(), 0, ext->serial); + IoDeleteDevice(dev->device); + HeapFree(GetProcessHeap(), 0, dev); + break; + } + } + LeaveCriticalSection(&device_list_cs); +} + NTSTATUS WINAPI common_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) { NTSTATUS status = irp->IoStatus.u.Status;
participants (2)
-
Aric Stewart -
Sebastian Lackner