[PATCH 1/5] ntoskrnl.exe/tests: Retry timer wait to address spurious failures.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index f1c72d8fdaf..918b4632c40 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -756,6 +756,8 @@ static void test_sync(void) ok(ret == 0, "got %#x\n", ret); ret = wait_single(&timer, 0); + /* aliasing makes it sometimes succeeds, try again in that case */ + if (ret == 0) ret = wait_single(&timer, 0); ok(ret == WAIT_TIMEOUT, "got %#x\n", ret); ret = wait_single(&timer, -40 * 10000); -- 2.32.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.exe.spec | 6 +- dlls/ntoskrnl.exe/sync.c | 42 +++++++++++ dlls/ntoskrnl.exe/tests/driver.c | 113 ++++++++++++++++++++++++++++ include/ddk/wdm.h | 4 + 4 files changed, 162 insertions(+), 3 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec index 5fdaa922f45..c1244a42146 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec @@ -567,7 +567,7 @@ @ stub KeI386SetGdtSelector @ stub KeIcacheFlushCount @ stdcall KeInitializeApc(ptr ptr long ptr ptr ptr long ptr) -@ stub KeInitializeDeviceQueue +@ stdcall KeInitializeDeviceQueue(ptr) @ stdcall KeInitializeDpc(ptr ptr ptr) @ stdcall KeInitializeEvent(ptr long long) @ stub KeInitializeInterrupt @@ -579,7 +579,7 @@ @ stdcall KeInitializeTimer(ptr) @ stdcall KeInitializeTimerEx(ptr long) @ stub KeInsertByKeyDeviceQueue -@ stub KeInsertDeviceQueue +@ stdcall KeInsertDeviceQueue(ptr ptr) @ stub KeInsertHeadQueue @ stdcall KeInsertQueue(ptr ptr) @ stub KeInsertQueueApc @@ -617,7 +617,7 @@ @ stdcall KeReleaseSpinLockFromDpcLevel(ptr) @ stub KeRemoveByKeyDeviceQueue @ stub KeRemoveByKeyDeviceQueueIfBusy -@ stub KeRemoveDeviceQueue +@ stdcall KeRemoveDeviceQueue(ptr) @ stub KeRemoveEntryDeviceQueue @ stub KeRemoveQueue @ stub KeRemoveQueueDpc diff --git a/dlls/ntoskrnl.exe/sync.c b/dlls/ntoskrnl.exe/sync.c index 445c8d890ab..5d028792e5f 100644 --- a/dlls/ntoskrnl.exe/sync.c +++ b/dlls/ntoskrnl.exe/sync.c @@ -1362,3 +1362,45 @@ BOOLEAN WINAPI KeSetTimer(KTIMER *timer, LARGE_INTEGER duetime, KDPC *dpc) return KeSetTimerEx(timer, duetime, 0, dpc); } + +void WINAPI KeInitializeDeviceQueue( KDEVICE_QUEUE *queue ) +{ + TRACE( "queue %p.\n", queue ); + + KeInitializeSpinLock( &queue->Lock ); + InitializeListHead( &queue->DeviceListHead ); + queue->Busy = FALSE; + queue->Type = IO_TYPE_DEVICE_QUEUE; + queue->Size = sizeof(*queue); +} + +BOOLEAN WINAPI KeInsertDeviceQueue( KDEVICE_QUEUE *queue, KDEVICE_QUEUE_ENTRY *entry ) +{ + KIRQL irql; + + TRACE( "queue %p, entry %p.\n", queue, entry ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if ((entry->Inserted = queue->Busy)) + InsertTailList( &queue->DeviceListHead, &entry->DeviceListEntry ); + queue->Busy = TRUE; + KeReleaseSpinLock( &queue->Lock, irql ); + + return entry->Inserted; +} + +KDEVICE_QUEUE_ENTRY *WINAPI KeRemoveDeviceQueue( KDEVICE_QUEUE *queue ) +{ + LIST_ENTRY *entry = NULL; + KIRQL irql; + + TRACE( "queue %p.\n", queue ); + + KeAcquireSpinLock( &queue->Lock, &irql ); + if (IsListEmpty( &queue->DeviceListHead )) queue->Busy = FALSE; + else entry = RemoveHeadList( &queue->DeviceListHead ); + KeReleaseSpinLock( &queue->Lock, irql ); + + if (!entry) return NULL; + return CONTAINING_RECORD( entry, KDEVICE_QUEUE_ENTRY, DeviceListEntry ); +} diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index 918b4632c40..d5f8b07bd28 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -141,6 +141,118 @@ static void test_irp_struct(IRP *irp, DEVICE_OBJECT *device) IoFreeIrp(irp); } +static int cancel_queue_cnt; + +static void WINAPI cancel_queued_irp(DEVICE_OBJECT *device, IRP *irp) +{ + IoReleaseCancelSpinLock(irp->CancelIrql); + ok(irp->Cancel == TRUE, "Cancel = %x\n", irp->Cancel); + ok(!irp->CancelRoutine, "CancelRoutine = %p\n", irp->CancelRoutine); + irp->IoStatus.Status = STATUS_CANCELLED; + irp->IoStatus.Information = 0; + cancel_queue_cnt++; +} + +static void test_queue(void) +{ + KDEVICE_QUEUE_ENTRY *entry; + KDEVICE_QUEUE queue; + BOOLEAN ret; + KIRQL irql; + IRP *irp; + + irp = IoAllocateIrp(1, FALSE); + + memset(&queue, 0xcd, sizeof(queue)); + KeInitializeDeviceQueue(&queue); + ok(!queue.Busy, "unexpected Busy state\n"); + ok(queue.Size == sizeof(queue), "unexpected Size %x\n", queue.Size); + ok(queue.Type == IO_TYPE_DEVICE_QUEUE, "unexpected Type %x\n", queue.Type); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(!entry, "expected KeRemoveDeviceQueue to return NULL\n"); + ok(!queue.Busy, "unexpected Busy state\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(ret, "expected KeInsertDeviceQueue to insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(!IsListEmpty(&queue.DeviceListHead), "unexpected empty queue list\n"); + ok(queue.DeviceListHead.Flink == &irp->Tail.Overlay.DeviceQueueEntry.DeviceListEntry, + "unexpected queue list head\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n"); + ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, + "unexpected IRP returned\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry == NULL, "expected KeRemoveDeviceQueue to return NULL\n"); + ok(!queue.Busy, "unexpected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + IoCancelIrp(irp); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(!ret, "expected KeInsertDeviceQueue to not insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(ret, "expected KeInsertDeviceQueue to insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(queue.DeviceListHead.Flink == &irp->Tail.Overlay.DeviceQueueEntry.DeviceListEntry, + "unexpected queue list head\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n"); + ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, + "unexpected IRP returned\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + IoFreeIrp(irp); + + irp = IoAllocateIrp(1, FALSE); + + IoAcquireCancelSpinLock(&irql); + IoSetCancelRoutine(irp, cancel_queued_irp); + IoReleaseCancelSpinLock(irql); + + ret = KeInsertDeviceQueue(&queue, &irp->Tail.Overlay.DeviceQueueEntry); + ok(ret, "expected KeInsertDeviceQueue to insert IRP\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(queue.DeviceListHead.Flink == &irp->Tail.Overlay.DeviceQueueEntry.DeviceListEntry, + "unexpected queue list head\n"); + + IoCancelIrp(irp); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + ok(cancel_queue_cnt, "expected cancel routine to be called\n"); + + entry = KeRemoveDeviceQueue(&queue); + ok(entry != NULL, "expected KeRemoveDeviceQueue to return non-NULL\n"); + ok(CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry) == irp, + "unexpected IRP returned\n"); + ok(irp->Cancel, "unexpected non-cancelled state\n"); + ok(cancel_queue_cnt, "expected cancel routine to be called\n"); + ok(queue.Busy, "expected Busy state\n"); + ok(IsListEmpty(&queue.DeviceListHead), "expected empty queue list\n"); + + IoFreeIrp(irp); +} + static void test_mdl_map(void) { char buffer[20] = "test buffer"; @@ -2128,6 +2240,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st test_init_funcs(); test_load_driver(); test_sync(); + test_queue(); test_version(); test_stack_callout(); test_lookaside_list(); diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h index 447833c4bc9..9bc3305471c 100644 --- a/include/ddk/wdm.h +++ b/include/ddk/wdm.h @@ -397,6 +397,7 @@ typedef struct _WAIT_CONTEXT_BLOCK { #define IO_TYPE_ERROR_LOG 0x0b #define IO_TYPE_ERROR_MESSAGE 0x0c #define IO_TYPE_DEVICE_OBJECT_EXTENSION 0x0d +#define IO_TYPE_DEVICE_QUEUE 0x14 typedef struct _DEVICE_OBJECT { CSHORT Type; @@ -1750,6 +1751,7 @@ void WINAPI KeEnterCriticalRegion(void); void WINAPI KeGenericCallDpc(PKDEFERRED_ROUTINE,PVOID); ULONG WINAPI KeGetCurrentProcessorNumber(void); PKTHREAD WINAPI KeGetCurrentThread(void); +void WINAPI KeInitializeDeviceQueue(KDEVICE_QUEUE*); void WINAPI KeInitializeDpc(KDPC*,PKDEFERRED_ROUTINE,void*); void WINAPI KeInitializeEvent(PRKEVENT,EVENT_TYPE,BOOLEAN); void WINAPI KeInitializeMutex(PRKMUTEX,ULONG); @@ -1757,6 +1759,7 @@ void WINAPI KeInitializeSemaphore(PRKSEMAPHORE,LONG,LONG); void WINAPI KeInitializeSpinLock(KSPIN_LOCK*); void WINAPI KeInitializeTimerEx(PKTIMER,TIMER_TYPE); void WINAPI KeInitializeTimer(KTIMER*); +BOOLEAN WINAPI KeInsertDeviceQueue(KDEVICE_QUEUE*,KDEVICE_QUEUE_ENTRY*); void WINAPI KeLeaveCriticalRegion(void); ULONG WINAPI KeQueryActiveProcessorCountEx(USHORT); KAFFINITY WINAPI KeQueryActiveProcessors(void); @@ -1769,6 +1772,7 @@ LONG WINAPI KeReleaseMutex(PRKMUTEX,BOOLEAN); LONG WINAPI KeReleaseSemaphore(PRKSEMAPHORE,KPRIORITY,LONG,BOOLEAN); void WINAPI KeReleaseSpinLock(KSPIN_LOCK*,KIRQL); void WINAPI KeReleaseSpinLockFromDpcLevel(KSPIN_LOCK*); +KDEVICE_QUEUE_ENTRY * WINAPI KeRemoveDeviceQueue(KDEVICE_QUEUE*); LONG WINAPI KeResetEvent(PRKEVENT); void WINAPI KeRevertToUserAffinityThread(void); void WINAPI KeRevertToUserAffinityThreadEx(KAFFINITY affinity); -- 2.32.0
This shows that removing a device should send IRP_MN_SURPRISE_REMOVAL only, and that it's still possible to use DeviceIoControl on already opened handles. IRP_MN_REMOVE_DEVICE should only be sent when all then opened handles are closed. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.h | 2 + dlls/ntoskrnl.exe/tests/driver_pnp.c | 81 ++++++++++++++++++++++++++-- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 30 ++++++++++- 3 files changed, 107 insertions(+), 6 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 2c62baa0a61..455695ad36b 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -35,6 +35,8 @@ #define IOCTL_WINETEST_RETURN_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_MISMATCHED_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80b, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_COMPLETION CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80c, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_MARK_PENDING CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_CHECK_REMOVED CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80e, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_MAIN CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x800, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_REGISTER_IFACE CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x801, METHOD_BUFFERED, FILE_ANY_ACCESS) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 774c98bae89..58dd32039cf 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -41,6 +41,11 @@ 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; + struct device { struct list entry; @@ -51,6 +56,36 @@ struct device DEVICE_POWER_STATE power_state; }; +static IRP *pop_pending_irp(DEVICE_OBJECT *device) +{ + KDEVICE_QUEUE_ENTRY *entry; + IRP *irp; + + if (!(entry = KeRemoveDeviceQueue(&device->DeviceQueue))) irp = NULL; + else irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry); + + return irp; +} + +static NTSTATUS push_pending_irp(DEVICE_OBJECT *device, IRP *irp) +{ + do { IoMarkIrpPending(irp); } + while (!KeInsertDeviceQueue(&device->DeviceQueue, &irp->Tail.Overlay.DeviceQueueEntry)); + + return STATUS_PENDING; +} + +static void remove_pending_irps(DEVICE_OBJECT *device) +{ + IRP *irp; + + while ((irp = pop_pending_irp(device))) + { + irp->IoStatus.Status = STATUS_DEVICE_REMOVED; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } +} + static struct list device_list = LIST_INIT(device_list); static FAST_MUTEX driver_lock; @@ -207,6 +242,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) POWER_STATE state = {.DeviceState = PowerDeviceD0}; NTSTATUS status; + KeInitializeDeviceQueue(&device_obj->DeviceQueue); + ok(!stack->Parameters.StartDevice.AllocatedResources, "expected no resources\n"); ok(!stack->Parameters.StartDevice.AllocatedResourcesTranslated, "expected no translated resources\n"); @@ -223,6 +260,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) } case IRP_MN_REMOVE_DEVICE: + /* should've been checked and reset by IOCTL_WINETEST_CHECK_REMOVED */ + ok(remove_device_count == 0, "expected no IRP_MN_REMOVE_DEVICE\n"); + todo_wine ok(surprise_removal_count == 0, "expected no IRP_MN_SURPRISE_REMOVAL\n"); + ok(query_remove_device_count == 0, "expected no IRP_MN_QUERY_REMOVE_DEVICE\n"); + ok(cancel_remove_device_count == 0, "expected no IRP_MN_CANCEL_REMOVE_DEVICE\n"); + + remove_device_count++; + remove_pending_irps(device_obj); if (device->removed) { IoSetDeviceInterfaceState(&device->child_symlink, FALSE); @@ -289,8 +334,20 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) break; } - case IRP_MN_QUERY_REMOVE_DEVICE: case IRP_MN_SURPRISE_REMOVAL: + surprise_removal_count++; + remove_pending_irps(device_obj); + ret = STATUS_SUCCESS; + break; + + case IRP_MN_QUERY_REMOVE_DEVICE: + query_remove_device_count++; + remove_pending_irps(device_obj); + ret = STATUS_SUCCESS; + break; + + case IRP_MN_CANCEL_REMOVE_DEVICE: + cancel_remove_device_count++; ret = STATUS_SUCCESS; break; } @@ -579,8 +636,10 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) } } -static NTSTATUS pdo_ioctl(struct device *device, IRP *irp, IO_STACK_LOCATION *stack, ULONG code) +static NTSTATUS pdo_ioctl(DEVICE_OBJECT *device_obj, IRP *irp, IO_STACK_LOCATION *stack, ULONG code) { + struct device *device = device_obj->DeviceExtension; + switch (code) { case IOCTL_WINETEST_CHILD_GET_ID: @@ -590,6 +649,20 @@ static NTSTATUS pdo_ioctl(struct device *device, IRP *irp, IO_STACK_LOCATION *st irp->IoStatus.Information = sizeof(device->id); return STATUS_SUCCESS; + case IOCTL_WINETEST_MARK_PENDING: + return push_pending_irp(device_obj, irp); + + case IOCTL_WINETEST_CHECK_REMOVED: + ok(remove_device_count == 0, "expected IRP_MN_REMOVE_DEVICE\n"); + ok(surprise_removal_count == 1, "expected IRP_MN_SURPRISE_REMOVAL\n"); + ok(query_remove_device_count == 0, "expected no IRP_MN_QUERY_REMOVE_DEVICE\n"); + ok(cancel_remove_device_count == 0, "expected no IRP_MN_CANCEL_REMOVE_DEVICE\n"); + remove_device_count = 0; + surprise_removal_count = 0; + query_remove_device_count = 0; + cancel_remove_device_count = 0; + return STATUS_SUCCESS; + default: ok(0, "Unexpected ioctl %#x.\n", code); return STATUS_NOT_IMPLEMENTED; @@ -605,10 +678,10 @@ static NTSTATUS WINAPI driver_ioctl(DEVICE_OBJECT *device, IRP *irp) if (device == bus_fdo) status = fdo_ioctl(irp, stack, code); else - status = pdo_ioctl(device->DeviceExtension, irp, stack, code); + status = pdo_ioctl(device, irp, stack, code); irp->IoStatus.Status = status; - IoCompleteRequest(irp, IO_NO_INCREMENT); + if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; } diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 432bc168259..8fdb6c75f99 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1150,10 +1150,11 @@ static void test_pnp_devices(void) }; HDEVNOTIFY notify_handle; DWORD size, type, dword; + HANDLE bus, child, tmp; OBJECT_ATTRIBUTES attr; UNICODE_STRING string; + OVERLAPPED ovl = {0}; IO_STATUS_BLOCK io; - HANDLE bus, child; HDEVINFO set; HWND window; BOOL ret; @@ -1349,6 +1350,14 @@ static void test_pnp_devices(void) CloseHandle(child); + ret = NtOpenFile(&child, SYNCHRONIZE, &attr, &io, 0, 0); + ok(!ret, "failed to open child: %#x\n", ret); + + ret = DeviceIoControl(child, IOCTL_WINETEST_MARK_PENDING, NULL, 0, NULL, 0, &size, &ovl); + ok(!ret, "DeviceIoControl succeded\n"); + ok(GetLastError() == ERROR_IO_PENDING, "got error %u\n", GetLastError()); + ok(size == 0, "got size %u\n", size); + id = 1; ret = DeviceIoControl(bus, IOCTL_WINETEST_BUS_REMOVE_CHILD, &id, sizeof(id), NULL, 0, &size, NULL); ok(ret, "got error %u\n", GetLastError()); @@ -1357,7 +1366,24 @@ static void test_pnp_devices(void) ok(got_child_arrival == 1, "got %u child arrival messages\n", got_child_arrival); ok(got_child_removal == 1, "got %u child removal messages\n", got_child_removal); - ret = NtOpenFile(&child, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); + ret = DeviceIoControl(child, IOCTL_WINETEST_CHECK_REMOVED, NULL, 0, NULL, 0, &size, NULL); + todo_wine ok(ret, "got error %u\n", GetLastError()); + + ret = NtOpenFile(&tmp, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); + todo_wine ok(ret == STATUS_NO_SUCH_DEVICE, "got %#x\n", ret); + + ret = GetOverlappedResult(child, &ovl, &size, TRUE); + ok(!ret, "unexpected success.\n"); + ok(GetLastError() == ERROR_DEVICE_REMOVED, "got error %u\n", GetLastError()); + ok(size == 0, "got size %u\n", size); + + CloseHandle(child); + + pump_messages(); + ok(got_child_arrival == 1, "got %u child arrival messages\n", got_child_arrival); + ok(got_child_removal == 1, "got %u child removal messages\n", got_child_removal); + + ret = NtOpenFile(&tmp, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); ok(ret == STATUS_OBJECT_NAME_NOT_FOUND, "got %#x\n", ret); CloseHandle(bus); -- 2.32.0
On 6/28/21 5:22 AM, Rémi Bernon wrote:
This shows that removing a device should send IRP_MN_SURPRISE_REMOVAL only, and that it's still possible to use DeviceIoControl on already opened handles.
IRP_MN_REMOVE_DEVICE should only be sent when all then opened handles are closed.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.h | 2 + dlls/ntoskrnl.exe/tests/driver_pnp.c | 81 ++++++++++++++++++++++++++-- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 30 ++++++++++- 3 files changed, 107 insertions(+), 6 deletions(-)
diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 2c62baa0a61..455695ad36b 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -35,6 +35,8 @@ #define IOCTL_WINETEST_RETURN_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_MISMATCHED_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80b, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_COMPLETION CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80c, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_MARK_PENDING CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_CHECK_REMOVED CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80e, METHOD_NEITHER, FILE_ANY_ACCESS)
#define IOCTL_WINETEST_BUS_MAIN CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x800, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_REGISTER_IFACE CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x801, METHOD_BUFFERED, FILE_ANY_ACCESS) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 774c98bae89..58dd32039cf 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -41,6 +41,11 @@ 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; + struct device { struct list entry; @@ -51,6 +56,36 @@ struct device DEVICE_POWER_STATE power_state; };
+static IRP *pop_pending_irp(DEVICE_OBJECT *device) +{ + KDEVICE_QUEUE_ENTRY *entry; + IRP *irp; + + if (!(entry = KeRemoveDeviceQueue(&device->DeviceQueue))) irp = NULL; + else irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry); + + return irp; +} + +static NTSTATUS push_pending_irp(DEVICE_OBJECT *device, IRP *irp) +{ + do { IoMarkIrpPending(irp); } + while (!KeInsertDeviceQueue(&device->DeviceQueue, &irp->Tail.Overlay.DeviceQueueEntry)); + + return STATUS_PENDING; +} +
See, the problem is that as far as I can tell (though it's tricky, because Microsoft doesn't actually provide any code samples) this doesn't match the intended usage pattern of KeInsertDeviceQueue(), which makes it look like a mistake. And it still doesn't look better than this: static IRP *pop_pending_irp(DEVICE_OBJECT *device) { LIST_ENTRY *entry; IRQL irql; IRP *irp; KeAcquireSpinLock(&device->lock, &irql); entry = RemoveHeadList(&device->pending_irps); KeReleaseSpinLock(&device->lock, irql); if (entry == &device->pending_irps) return NULL; return CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); } static void push_pending_irp(DEVICE_OBJECT *device, IRP *irp) { IRQL irql; IoMarkIrpPending(irp); KeAcquireSpinLock(&device->lock, &irql); InsertTailList(&device->pending_irps, &irp->Tail.Overlay.ListEntry); KeReleaseSpinLock(&device->lock, irql); } A couple more lines of code, and it uses a couple extra fields, but it's that much clearer what it's supposed to be doing. On a sort of related note, though, I have considered having some sort of framework for PnP drivers. Device removal is one reason why. In this case WDF might actually not be a terrible fit; I've been sort of working on reimplementing that locally, and while its API isn't good, it also doesn't seem very bad. But I also haven't gotten very far into it, so it's worth examining how it handles certain things before proceeding.
+static void remove_pending_irps(DEVICE_OBJECT *device) +{ + IRP *irp; + + while ((irp = pop_pending_irp(device))) + { + irp->IoStatus.Status = STATUS_DEVICE_REMOVED; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } +} + static struct list device_list = LIST_INIT(device_list);
static FAST_MUTEX driver_lock; @@ -207,6 +242,8 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) POWER_STATE state = {.DeviceState = PowerDeviceD0}; NTSTATUS status;
+ KeInitializeDeviceQueue(&device_obj->DeviceQueue); + ok(!stack->Parameters.StartDevice.AllocatedResources, "expected no resources\n"); ok(!stack->Parameters.StartDevice.AllocatedResourcesTranslated, "expected no translated resources\n");
@@ -223,6 +260,14 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) }
case IRP_MN_REMOVE_DEVICE: + /* should've been checked and reset by IOCTL_WINETEST_CHECK_REMOVED */ + ok(remove_device_count == 0, "expected no IRP_MN_REMOVE_DEVICE\n"); + todo_wine ok(surprise_removal_count == 0, "expected no IRP_MN_SURPRISE_REMOVAL\n"); + ok(query_remove_device_count == 0, "expected no IRP_MN_QUERY_REMOVE_DEVICE\n"); + ok(cancel_remove_device_count == 0, "expected no IRP_MN_CANCEL_REMOVE_DEVICE\n"); + + remove_device_count++; + remove_pending_irps(device_obj); if (device->removed) { IoSetDeviceInterfaceState(&device->child_symlink, FALSE); @@ -289,8 +334,20 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device_obj, IRP *irp) break; }
- case IRP_MN_QUERY_REMOVE_DEVICE: case IRP_MN_SURPRISE_REMOVAL: + surprise_removal_count++; + remove_pending_irps(device_obj); + ret = STATUS_SUCCESS; + break; + + case IRP_MN_QUERY_REMOVE_DEVICE: + query_remove_device_count++; + remove_pending_irps(device_obj); + ret = STATUS_SUCCESS; + break; + + case IRP_MN_CANCEL_REMOVE_DEVICE: + cancel_remove_device_count++; ret = STATUS_SUCCESS; break; } @@ -579,8 +636,10 @@ static NTSTATUS fdo_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG code) } }
-static NTSTATUS pdo_ioctl(struct device *device, IRP *irp, IO_STACK_LOCATION *stack, ULONG code) +static NTSTATUS pdo_ioctl(DEVICE_OBJECT *device_obj, IRP *irp, IO_STACK_LOCATION *stack, ULONG code) { + struct device *device = device_obj->DeviceExtension; + switch (code) { case IOCTL_WINETEST_CHILD_GET_ID: @@ -590,6 +649,20 @@ static NTSTATUS pdo_ioctl(struct device *device, IRP *irp, IO_STACK_LOCATION *st irp->IoStatus.Information = sizeof(device->id); return STATUS_SUCCESS;
+ case IOCTL_WINETEST_MARK_PENDING: + return push_pending_irp(device_obj, irp); + + case IOCTL_WINETEST_CHECK_REMOVED: + ok(remove_device_count == 0, "expected IRP_MN_REMOVE_DEVICE\n"); + ok(surprise_removal_count == 1, "expected IRP_MN_SURPRISE_REMOVAL\n"); + ok(query_remove_device_count == 0, "expected no IRP_MN_QUERY_REMOVE_DEVICE\n"); + ok(cancel_remove_device_count == 0, "expected no IRP_MN_CANCEL_REMOVE_DEVICE\n"); + remove_device_count = 0; + surprise_removal_count = 0; + query_remove_device_count = 0; + cancel_remove_device_count = 0; + return STATUS_SUCCESS; + default: ok(0, "Unexpected ioctl %#x.\n", code); return STATUS_NOT_IMPLEMENTED; @@ -605,10 +678,10 @@ static NTSTATUS WINAPI driver_ioctl(DEVICE_OBJECT *device, IRP *irp) if (device == bus_fdo) status = fdo_ioctl(irp, stack, code); else - status = pdo_ioctl(device->DeviceExtension, irp, stack, code); + status = pdo_ioctl(device, irp, stack, code);
irp->IoStatus.Status = status; - IoCompleteRequest(irp, IO_NO_INCREMENT); + if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT); return status; }
diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 432bc168259..8fdb6c75f99 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1150,10 +1150,11 @@ static void test_pnp_devices(void) }; HDEVNOTIFY notify_handle; DWORD size, type, dword; + HANDLE bus, child, tmp; OBJECT_ATTRIBUTES attr; UNICODE_STRING string; + OVERLAPPED ovl = {0}; IO_STATUS_BLOCK io; - HANDLE bus, child; HDEVINFO set; HWND window; BOOL ret; @@ -1349,6 +1350,14 @@ static void test_pnp_devices(void)
CloseHandle(child);
+ ret = NtOpenFile(&child, SYNCHRONIZE, &attr, &io, 0, 0); + ok(!ret, "failed to open child: %#x\n", ret); + + ret = DeviceIoControl(child, IOCTL_WINETEST_MARK_PENDING, NULL, 0, NULL, 0, &size, &ovl); + ok(!ret, "DeviceIoControl succeded\n"); + ok(GetLastError() == ERROR_IO_PENDING, "got error %u\n", GetLastError()); + ok(size == 0, "got size %u\n", size); + id = 1; ret = DeviceIoControl(bus, IOCTL_WINETEST_BUS_REMOVE_CHILD, &id, sizeof(id), NULL, 0, &size, NULL); ok(ret, "got error %u\n", GetLastError()); @@ -1357,7 +1366,24 @@ static void test_pnp_devices(void) ok(got_child_arrival == 1, "got %u child arrival messages\n", got_child_arrival); ok(got_child_removal == 1, "got %u child removal messages\n", got_child_removal);
- ret = NtOpenFile(&child, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); + ret = DeviceIoControl(child, IOCTL_WINETEST_CHECK_REMOVED, NULL, 0, NULL, 0, &size, NULL); + todo_wine ok(ret, "got error %u\n", GetLastError()); + + ret = NtOpenFile(&tmp, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); + todo_wine ok(ret == STATUS_NO_SUCH_DEVICE, "got %#x\n", ret); + + ret = GetOverlappedResult(child, &ovl, &size, TRUE); + ok(!ret, "unexpected success.\n"); + ok(GetLastError() == ERROR_DEVICE_REMOVED, "got error %u\n", GetLastError()); + ok(size == 0, "got size %u\n", size); + + CloseHandle(child); + + pump_messages(); + ok(got_child_arrival == 1, "got %u child arrival messages\n", got_child_arrival); + ok(got_child_removal == 1, "got %u child removal messages\n", got_child_removal); + + ret = NtOpenFile(&tmp, SYNCHRONIZE, &attr, &io, 0, FILE_SYNCHRONOUS_IO_NONALERT); ok(ret == STATUS_OBJECT_NAME_NOT_FOUND, "got %#x\n", ret);
CloseHandle(bus);
On 6/28/21 6:15 PM, Zebediah Figura (she/her) wrote:
On 6/28/21 5:22 AM, Rémi Bernon wrote:
This shows that removing a device should send IRP_MN_SURPRISE_REMOVAL only, and that it's still possible to use DeviceIoControl on already opened handles.
IRP_MN_REMOVE_DEVICE should only be sent when all then opened handles are closed.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.h | 2 + dlls/ntoskrnl.exe/tests/driver_pnp.c | 81 ++++++++++++++++++++++++++-- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 30 ++++++++++- 3 files changed, 107 insertions(+), 6 deletions(-)
diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 2c62baa0a61..455695ad36b 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -35,6 +35,8 @@ #define IOCTL_WINETEST_RETURN_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_MISMATCHED_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80b, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_COMPLETION CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80c, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_MARK_PENDING CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_CHECK_REMOVED CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80e, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_MAIN CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x800, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_REGISTER_IFACE CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x801, METHOD_BUFFERED, FILE_ANY_ACCESS) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 774c98bae89..58dd32039cf 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -41,6 +41,11 @@ 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; + struct device { struct list entry; @@ -51,6 +56,36 @@ struct device DEVICE_POWER_STATE power_state; }; +static IRP *pop_pending_irp(DEVICE_OBJECT *device) +{ + KDEVICE_QUEUE_ENTRY *entry; + IRP *irp; + + if (!(entry = KeRemoveDeviceQueue(&device->DeviceQueue))) irp = NULL; + else irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry); + + return irp; +} + +static NTSTATUS push_pending_irp(DEVICE_OBJECT *device, IRP *irp) +{ + do { IoMarkIrpPending(irp); } + while (!KeInsertDeviceQueue(&device->DeviceQueue, &irp->Tail.Overlay.DeviceQueueEntry)); + + return STATUS_PENDING; +} +
See, the problem is that as far as I can tell (though it's tricky, because Microsoft doesn't actually provide any code samples) this doesn't match the intended usage pattern of KeInsertDeviceQueue(), which makes it look like a mistake. And it still doesn't look better than this:
static IRP *pop_pending_irp(DEVICE_OBJECT *device) { LIST_ENTRY *entry; IRQL irql; IRP *irp;
KeAcquireSpinLock(&device->lock, &irql); entry = RemoveHeadList(&device->pending_irps); KeReleaseSpinLock(&device->lock, irql);
if (entry == &device->pending_irps) return NULL; return CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); }
static void push_pending_irp(DEVICE_OBJECT *device, IRP *irp) { IRQL irql;
IoMarkIrpPending(irp); KeAcquireSpinLock(&device->lock, &irql); InsertTailList(&device->pending_irps, &irp->Tail.Overlay.ListEntry); KeReleaseSpinLock(&device->lock, irql); }
A couple more lines of code, and it uses a couple extra fields, but it's that much clearer what it's supposed to be doing.
I don't know, it just feels a waste to have this code duplicated every where, so the smaller it is the better. Not having to add these members in every driver (and in particular the spinlock) is also better IMHO, especially if we consider the functions could then be shared in a common private header. I think it's also easier to make mistakes with the acquire/release, and the temptation to reuse the lock for something else than this KeInsertDeviceQueue little specificity.
On a sort of related note, though, I have considered having some sort of framework for PnP drivers. Device removal is one reason why. In this case WDF might actually not be a terrible fit; I've been sort of working on reimplementing that locally, and while its API isn't good, it also doesn't seem very bad. But I also haven't gotten very far into it, so it's worth examining how it handles certain things before proceeding.
Yes, that would be nice. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 6/28/21 12:35 PM, Rémi Bernon wrote:
On 6/28/21 6:15 PM, Zebediah Figura (she/her) wrote:
On 6/28/21 5:22 AM, Rémi Bernon wrote:
This shows that removing a device should send IRP_MN_SURPRISE_REMOVAL only, and that it's still possible to use DeviceIoControl on already opened handles.
IRP_MN_REMOVE_DEVICE should only be sent when all then opened handles are closed.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.h | 2 + dlls/ntoskrnl.exe/tests/driver_pnp.c | 81 ++++++++++++++++++++++++++-- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 30 ++++++++++- 3 files changed, 107 insertions(+), 6 deletions(-)
diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 2c62baa0a61..455695ad36b 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -35,6 +35,8 @@ #define IOCTL_WINETEST_RETURN_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_MISMATCHED_STATUS CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80b, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_COMPLETION CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80c, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_MARK_PENDING CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_CHECK_REMOVED CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80e, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_MAIN CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x800, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_REGISTER_IFACE CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x801, METHOD_BUFFERED, FILE_ANY_ACCESS) diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 774c98bae89..58dd32039cf 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -41,6 +41,11 @@ 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; + struct device { struct list entry; @@ -51,6 +56,36 @@ struct device DEVICE_POWER_STATE power_state; }; +static IRP *pop_pending_irp(DEVICE_OBJECT *device) +{ + KDEVICE_QUEUE_ENTRY *entry; + IRP *irp; + + if (!(entry = KeRemoveDeviceQueue(&device->DeviceQueue))) irp = NULL; + else irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.DeviceQueueEntry); + + return irp; +} + +static NTSTATUS push_pending_irp(DEVICE_OBJECT *device, IRP *irp) +{ + do { IoMarkIrpPending(irp); } + while (!KeInsertDeviceQueue(&device->DeviceQueue, &irp->Tail.Overlay.DeviceQueueEntry)); + + return STATUS_PENDING; +} +
See, the problem is that as far as I can tell (though it's tricky, because Microsoft doesn't actually provide any code samples) this doesn't match the intended usage pattern of KeInsertDeviceQueue(), which makes it look like a mistake. And it still doesn't look better than this:
static IRP *pop_pending_irp(DEVICE_OBJECT *device) { LIST_ENTRY *entry; IRQL irql; IRP *irp;
KeAcquireSpinLock(&device->lock, &irql); entry = RemoveHeadList(&device->pending_irps); KeReleaseSpinLock(&device->lock, irql);
if (entry == &device->pending_irps) return NULL; return CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); }
static void push_pending_irp(DEVICE_OBJECT *device, IRP *irp) { IRQL irql;
IoMarkIrpPending(irp); KeAcquireSpinLock(&device->lock, &irql); InsertTailList(&device->pending_irps, &irp->Tail.Overlay.ListEntry); KeReleaseSpinLock(&device->lock, irql); }
A couple more lines of code, and it uses a couple extra fields, but it's that much clearer what it's supposed to be doing.
I don't know, it just feels a waste to have this code duplicated every where, so the smaller it is the better. Not having to add these members in every driver (and in particular the spinlock) is also better IMHO, especially if we consider the functions could then be shared in a common private header.
Don't get me wrong, I'm in favor of sharing this code. But I'd also like the shared code to be clear and readable, and I just don't think that KDEVICE_QUEUE APIs allow for that. Plus, if we really want to, we could always alias the KDEVICE_QUEUE with our own to save memory. After all, we're not using it...
I think it's also easier to make mistakes with the acquire/release, and the temptation to reuse the lock for something else than this KeInsertDeviceQueue little specificity.
Sure, it should probably be renamed "queue_lock" or something.
On a sort of related note, though, I have considered having some sort of framework for PnP drivers. Device removal is one reason why. In this case WDF might actually not be a terrible fit; I've been sort of working on reimplementing that locally, and while its API isn't good, it also doesn't seem very bad. But I also haven't gotten very far into it, so it's worth examining how it handles certain things before proceeding.
Yes, that would be nice.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index a512015d478..897be25ed51 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -235,6 +235,22 @@ static WCHAR *get_compatible_ids(DEVICE_OBJECT *device) return dst; } +static void remove_pending_irps(DEVICE_OBJECT *device) +{ + struct device_extension *ext = device->DeviceExtension; + LIST_ENTRY *entry; + + EnterCriticalSection(&ext->cs); + while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) + { + IRP *queued_irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.s.ListEntry); + queued_irp->IoStatus.u.Status = STATUS_DELETE_PENDING; + queued_irp->IoStatus.Information = 0; + IoCompleteRequest(queued_irp, IO_NO_INCREMENT); + } + LeaveCriticalSection(&ext->cs); +} + DEVICE_OBJECT *bus_create_hid_device(const WCHAR *busidW, WORD vid, WORD pid, WORD input, DWORD version, DWORD uid, const WCHAR *serialW, BOOL is_gamepad, const platform_vtbl *vtbl, DWORD platform_data_size) @@ -687,17 +703,8 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; - LIST_ENTRY *entry; - EnterCriticalSection(&ext->cs); - while ((entry = RemoveHeadList(&ext->irp_queue)) != &ext->irp_queue) - { - IRP *queued_irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.s.ListEntry); - queued_irp->IoStatus.u.Status = STATUS_DELETE_PENDING; - queued_irp->IoStatus.Information = 0; - IoCompleteRequest(queued_irp, IO_NO_INCREMENT); - } - LeaveCriticalSection(&ext->cs); + remove_pending_irps(device); ext->vtbl->free_device(device); -- 2.32.0
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winebus.sys/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c index 897be25ed51..f3e63e87aa2 100644 --- a/dlls/winebus.sys/main.c +++ b/dlls/winebus.sys/main.c @@ -374,10 +374,6 @@ void bus_unlink_hid_device(DEVICE_OBJECT *device) EnterCriticalSection(&device_list_cs); list_remove(&pnp_device->entry); LeaveCriticalSection(&device_list_cs); - - EnterCriticalSection(&ext->cs); - ext->removed = TRUE; - LeaveCriticalSection(&ext->cs); } void bus_remove_hid_device(DEVICE_OBJECT *device) @@ -700,11 +696,21 @@ static NTSTATUS pdo_pnp_dispatch(DEVICE_OBJECT *device, IRP *irp) status = STATUS_SUCCESS; break; + case IRP_MN_SURPRISE_REMOVAL: + EnterCriticalSection(&ext->cs); + remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); + break; + case IRP_MN_REMOVE_DEVICE: { struct pnp_device *pnp_device = ext->pnp_device; + EnterCriticalSection(&ext->cs); remove_pending_irps(device); + ext->removed = TRUE; + LeaveCriticalSection(&ext->cs); ext->vtbl->free_device(device); -- 2.32.0
participants (2)
-
Rémi Bernon -
Zebediah Figura (she/her)