On 6/22/21 6:14 PM, Zebediah Figura (she/her) wrote:
On 6/22/21 10:29 AM, Rémi Bernon wrote:
On 6/22/21 5:20 PM, Zebediah Figura (she/her) wrote:
On 6/22/21 7:02 AM, Rémi Bernon wrote:
Signed-off-by: Rémi Bernon rbernon@codeweavers.com
MSDN says that the cancel spinlock has to be held whenever these device queues are used, when setting cancel routines, but it's not clear to me why, as IRP cancellation has apparently no effect on these functions behavior (I initially thought they should check and not queue cancelled IRPs, but it looks like they don't).
Where do you see this? I couldn't find any mention of it from a brief search.
For instance [1]:
"A driver must hold the system cancel spin lock when calling this routine if the driver uses the I/O manager-supplied device queue in the device object. The driver runs at IRQL = DISPATCH_LEVEL after calling IoAcquireCancelSpinLock until it releases the cancel spin lock with IoReleaseCancelSpinLock."
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-ios...
I understand the "I/O manager-supplied device queue in the device object" is the "KDEVICE_QUEUE DeviceQueue" member that I wanted to use with these functions instead of re-implementing a queue and a lock again.
Based on [2] and [3] I'm guessing the device queue functions never acquire the lock, but rather the point is to synchronize with dequeuing and cancelling. I can't figure out why it makes a difference, though...
It doesn't seem obvious to me that using KDEVICE_QUEUEs in Wine makes our job any easier, fwiw. Especially the part where you can't actually queue the first pending IRP. I'm not sure it's meant to handle those kinds of pending IRPs, but then it's not really clear how these things are supposed to be used; there are no code samples anywhere.
It probably also has something to do with IoStart(Next)Packet which need to handle cancellation.
Even when not using these, and even if queue-ing an IRP needs a loop I think using the device queue directly makes the code slightly simpler as it doesn't need locking.
It also saves a dedicated irp_queue and irp_queue_lock members which are already in the device object, and currently unused.
+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);
Could we test the actual contents of the queue list after all these calls?
I guess. It's hopefully going to be empty?
Empty after this call, but not after an eventual KeInsertDeviceQueue call; I think it's worth testing the state after all of the Ke* calls below.
Yeah I figured, I'll add more tests.