[PATCH 0/4] MR413: ntoskrnl: Enumerate child devices on a separate thread.
From: Zebediah Figura <zfigura(a)codeweavers.com> Do not rely on IoInvalidateDeviceRelations() to initialize the device; it's supposed to be asynchronous. --- dlls/wineusb.sys/wineusb.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/wineusb.sys/wineusb.c b/dlls/wineusb.sys/wineusb.c index 0b8af43a3dc..396591db451 100644 --- a/dlls/wineusb.sys/wineusb.c +++ b/dlls/wineusb.sys/wineusb.c @@ -255,11 +255,7 @@ static void add_unix_device(struct unix_device *unix_device) device->device_obj = device_obj; device->unix_device = unix_device; InitializeListHead(&device->irp_list); - - EnterCriticalSection(&wineusb_cs); - list_add_tail(&device_list, &device->entry); device->removed = FALSE; - LeaveCriticalSection(&wineusb_cs); device->class = device_desc.bDeviceClass; device->subclass = device_desc.bDeviceSubClass; @@ -268,6 +264,10 @@ static void add_unix_device(struct unix_device *unix_device) device->product = device_desc.idProduct; device->revision = device_desc.bcdDevice; + EnterCriticalSection(&wineusb_cs); + list_add_tail(&device_list, &device->entry); + LeaveCriticalSection(&wineusb_cs); + if (!(ret = libusb_get_active_config_descriptor(libusb_device, &config_desc))) { /* Create new devices for interfaces of composite devices. -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/413
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver_pnp.c | 65 +++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 972cc61c6df..a3e7329ae72 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -41,10 +41,7 @@ static UNICODE_STRING control_symlink, bus_symlink; static DRIVER_OBJECT *driver_obj; static DEVICE_OBJECT *bus_fdo, *bus_pdo; -static DWORD remove_device_count; -static DWORD surprise_removal_count; -static DWORD query_remove_device_count; -static DWORD cancel_remove_device_count; +static unsigned int remove_device_count, surprise_removal_count, query_remove_device_count, cancel_remove_device_count; struct irp_queue { @@ -100,6 +97,7 @@ struct device UNICODE_STRING child_symlink; DEVICE_POWER_STATE power_state; struct irp_queue irp_queue; + HANDLE plug_event, plug_event2; }; static struct list device_list = LIST_INIT(device_list); @@ -255,6 +253,7 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) case IRP_MN_START_DEVICE: { + static const LARGE_INTEGER wait_time = {.QuadPart = -500 * 10000}; POWER_STATE state = {.DeviceState = PowerDeviceD0}; NTSTATUS status; @@ -271,6 +270,12 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) state = PoSetPowerState(device_obj, DevicePowerState, state); todo_wine ok(state.DeviceState == device->power_state, "got previous state %u\n", state.DeviceState); device->power_state = PowerDeviceD0; + + status = ZwWaitForSingleObject(device->plug_event, TRUE, &wait_time); + todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + status = ZwSetEvent(device->plug_event2, NULL); + ok(!status, "Failed to set event, status %#lx.\n", status); + ret = STATUS_SUCCESS; break; } @@ -288,6 +293,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) { IoSetDeviceInterfaceState(&device->child_symlink, FALSE); RtlFreeUnicodeString(&device->child_symlink); + ZwClose(device->plug_event); + ZwClose(device->plug_event2); irp->IoStatus.Status = STATUS_SUCCESS; IoCompleteRequest(irp, IO_NO_INCREMENT); IoDeleteDevice(device->device_obj); @@ -351,10 +358,21 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) } case IRP_MN_SURPRISE_REMOVAL: + { + static const LARGE_INTEGER wait_time = {.QuadPart = -500 * 10000}; + NTSTATUS status; + surprise_removal_count++; irp_queue_clear(&device->irp_queue); + + status = ZwWaitForSingleObject(device->plug_event, TRUE, &wait_time); + ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + status = ZwSetEvent(device->plug_event2, NULL); + ok(!status, "Failed to set event, status %#lx.\n", status); + ret = STATUS_SUCCESS; break; + } case IRP_MN_QUERY_REMOVE_DEVICE: query_remove_device_count++; @@ -584,7 +602,9 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) case IOCTL_WINETEST_BUS_ADD_CHILD: { + static const LARGE_INTEGER wait_time = {.QuadPart = -500 * 10000}; DEVICE_OBJECT *device_obj; + OBJECT_ATTRIBUTES attr; UNICODE_STRING string; struct device *device; NTSTATUS status; @@ -605,6 +625,11 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) device->device_obj = device_obj; device->id = id; device->removed = FALSE; + InitializeObjectAttributes(&attr, NULL, OBJ_KERNEL_HANDLE, NULL, NULL); + status = ZwCreateEvent(&device->plug_event, EVENT_ALL_ACCESS, &attr, SynchronizationEvent, FALSE); + ok(!status, "Failed to create event, status %#lx.\n", status); + status = ZwCreateEvent(&device->plug_event2, EVENT_ALL_ACCESS, &attr, SynchronizationEvent, FALSE); + ok(!status, "Failed to create event, status %#lx.\n", status); ExAcquireFastMutex(&driver_lock); list_add_tail(&device_list, &device->entry); @@ -613,13 +638,26 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) device_obj->Flags &= ~DO_DEVICE_INITIALIZING; IoInvalidateDeviceRelations(bus_pdo, BusRelations); + IoInvalidateDeviceRelations(bus_pdo, BusRelations); + + /* Synchronize both ways, to show that the bus invalidation happens + * completely asynchronously and that neither thread blocks waiting + * for the other. */ + + status = ZwSetEvent(device->plug_event, NULL); + ok(!status, "Failed to set event, status %#lx.\n", status); + status = ZwWaitForSingleObject(device->plug_event2, TRUE, &wait_time); + ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); return STATUS_SUCCESS; } case IOCTL_WINETEST_BUS_REMOVE_CHILD: { + static const LARGE_INTEGER wait_time = {.QuadPart = -500 * 10000}; + HANDLE plug_event = NULL, plug_event2 = NULL; struct device *device; + NTSTATUS status; int id; if (stack->Parameters.DeviceIoControl.InputBufferLength < sizeof(int)) @@ -631,6 +669,8 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) { if (device->id == id) { + plug_event = device->plug_event; + plug_event2 = device->plug_event2; list_remove(&device->entry); device->removed = TRUE; break; @@ -638,10 +678,23 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) } ExReleaseFastMutex(&driver_lock); + IoInvalidateDeviceRelations(bus_pdo, BusRelations); IoInvalidateDeviceRelations(bus_pdo, BusRelations); - /* The actual removal might be asynchronous; we can't test that the - * device is gone here. */ + /* Synchronize both ways, to show that the bus invalidation happens + * completely asynchronously and that neither thread blocks waiting + * for the other. */ + + status = ZwSetEvent(plug_event, NULL); + todo_wine ok(!status, "Failed to set event, status %#lx.\n", status); + status = ZwWaitForSingleObject(plug_event2, TRUE, &wait_time); + todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + ok(surprise_removal_count == 1, "Got %u surprise removal events.\n", surprise_removal_count); + /* We shouldn't get IRP_MN_REMOVE_DEVICE until all user-space + * handles to the device are closed (and the user-space thread is + * currently blocked in this ioctl and won't close its handle + * yet.) */ + todo_wine ok(!remove_device_count, "Got %u remove events.\n", remove_device_count); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/413
From: Zebediah Figura <zfigura(a)codeweavers.com> wineusb enumerates new devices and completes IRPs from the same thread (as if it were an IRQ thread). On the other hand, the Hauppauge cx231xx USB driver performs I/O from within IRP_MN_START_DEVICE. If the wineusb "IRQ" thread is currently trying to report a new device, it won't be able to report I/O completion, resulting in a deadlock. Although wineusb could be modified to use more threads, these tests show that calling IoInvalidateDeviceRelations() should not block in this situation. --- dlls/ntoskrnl.exe/tests/driver_pnp.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index a3e7329ae72..4b269f00bfc 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -275,6 +275,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); status = ZwSetEvent(device->plug_event2, NULL); ok(!status, "Failed to set event, status %#lx.\n", status); + status = ZwWaitForSingleObject(device->plug_event, TRUE, &wait_time); + todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); ret = STATUS_SUCCESS; break; @@ -648,6 +650,10 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) ok(!status, "Failed to set event, status %#lx.\n", status); status = ZwWaitForSingleObject(device->plug_event2, TRUE, &wait_time); ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + /* IoInvalidateDeviceRelations() here shouldn't deadlock either. */ + IoInvalidateDeviceRelations(bus_pdo, BusRelations); + status = ZwSetEvent(device->plug_event, NULL); + ok(!status, "Failed to set event, status %#lx.\n", status); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/413
From: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl_private.h | 1 + dlls/ntoskrnl.exe/pnp.c | 40 +++++++++++++++++++++++++++- dlls/ntoskrnl.exe/tests/driver_pnp.c | 10 +++---- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl_private.h b/dlls/ntoskrnl.exe/ntoskrnl_private.h index c736a9805a0..ef1fa99057c 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl_private.h +++ b/dlls/ntoskrnl.exe/ntoskrnl_private.h @@ -22,6 +22,7 @@ #define __WINE_NTOSKRNL_PRIVATE_H #include <stdarg.h> +#include <stdbool.h> #include "ntstatus.h" #define WIN32_NO_STATUS #include "windef.h" diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 5d9ca2dca38..71c03586897 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -38,6 +38,12 @@ DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0); WINE_DEFAULT_DEBUG_CHANNEL(plugplay); +DECLARE_CRITICAL_SECTION(invalidated_devices_cs); +static CONDITION_VARIABLE invalidated_devices_cv = CONDITION_VARIABLE_INIT; + +static DEVICE_OBJECT **invalidated_devices; +static size_t invalidated_devices_count; + static inline const char *debugstr_propkey( const DEVPROPKEY *id ) { if (!id) return "(null)"; @@ -468,8 +474,14 @@ void WINAPI IoInvalidateDeviceRelations( DEVICE_OBJECT *device_object, DEVICE_RE switch (type) { case BusRelations: - handle_bus_relations( device_object ); + EnterCriticalSection( &invalidated_devices_cs ); + invalidated_devices = realloc( invalidated_devices, + (invalidated_devices_count + 1) * sizeof(*invalidated_devices) ); + invalidated_devices[invalidated_devices_count++] = device_object; + LeaveCriticalSection( &invalidated_devices_cs ); + WakeConditionVariable( &invalidated_devices_cv ); break; + default: FIXME("Unhandled relation %#x.\n", type); break; @@ -1086,6 +1098,30 @@ static NTSTATUS WINAPI pnp_manager_driver_entry( DRIVER_OBJECT *driver, UNICODE_ return STATUS_SUCCESS; } +static DWORD CALLBACK device_enum_thread_proc(void *arg) +{ + for (;;) + { + DEVICE_OBJECT *device; + + EnterCriticalSection( &invalidated_devices_cs ); + + while (!invalidated_devices_count) + SleepConditionVariableCS( &invalidated_devices_cv, &invalidated_devices_cs, INFINITE ); + + device = invalidated_devices[--invalidated_devices_count]; + + /* Don't hold the CS while enumerating the device. Tests show that + * calling IoInvalidateDeviceRelations() from another thread shouldn't + * block, even if this thread is blocked in an IRP handler. */ + LeaveCriticalSection( &invalidated_devices_cs ); + + handle_bus_relations( device ); + } + + return 0; +} + void pnp_manager_start(void) { static const WCHAR driver_nameW[] = {'\\','D','r','i','v','e','r','\\','P','n','p','M','a','n','a','g','e','r',0}; @@ -1109,6 +1145,8 @@ void pnp_manager_start(void) RpcStringFreeW( &binding_str ); if (err) ERR("RpcBindingFromStringBinding() failed, error %#lx\n", err); + + CreateThread( NULL, 0, device_enum_thread_proc, NULL, 0, NULL ); } void pnp_manager_stop_driver( struct wine_driver *driver ) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 4b269f00bfc..9727292d65d 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -272,11 +272,11 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) device->power_state = PowerDeviceD0; status = ZwWaitForSingleObject(device->plug_event, TRUE, &wait_time); - todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); status = ZwSetEvent(device->plug_event2, NULL); ok(!status, "Failed to set event, status %#lx.\n", status); status = ZwWaitForSingleObject(device->plug_event, TRUE, &wait_time); - todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); ret = STATUS_SUCCESS; break; @@ -692,15 +692,15 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) * for the other. */ status = ZwSetEvent(plug_event, NULL); - todo_wine ok(!status, "Failed to set event, status %#lx.\n", status); + ok(!status, "Failed to set event, status %#lx.\n", status); status = ZwWaitForSingleObject(plug_event2, TRUE, &wait_time); - todo_wine ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); + ok(!status, "Failed to wait for child plug event, status %#lx.\n", status); ok(surprise_removal_count == 1, "Got %u surprise removal events.\n", surprise_removal_count); /* We shouldn't get IRP_MN_REMOVE_DEVICE until all user-space * handles to the device are closed (and the user-space thread is * currently blocked in this ioctl and won't close its handle * yet.) */ - todo_wine ok(!remove_device_count, "Got %u remove events.\n", remove_device_count); + ok(!remove_device_count, "Got %u remove events.\n", remove_device_count); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/413
participants (2)
-
Zebediah Figura -
Zebediah Figura (＠zfigura)