Sorry if some of my comments only apply to similar code on Windows, I'm not super familiar with Wine's "kernel" code. Feel free to ignore the parts that are not applicable (or enlighten me :p).
On 2015-06-22 03:25, Aric Stewart wrote:
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
+static NTSTATUS WINAPI read_Completion(PDEVICE_OBJECT deviceObject, PIRP irp, PVOID context ) +{
- SetEvent(irp->UserEvent);
- return STATUS_MORE_PROCESSING_REQUIRED;
+}
+static DWORD CALLBACK hid_device_thread(LPVOID args) +{
- DEVICE_OBJECT *device = (DEVICE_OBJECT*)args;
- PIRP irp;
- IO_STATUS_BLOCK irp_status;
- IO_STACK_LOCATION *irpsp;
- DWORD rc;
- HANDLE events[2];
- BASE_DEVICE_EXTENSION *ext = (BASE_DEVICE_EXTENSION*)device->DeviceExtension;
- events[0] = CreateEventA(NULL, FALSE, FALSE, NULL);
- events[1] = ext->halt_event;
- if (ext->information.Polled)
- {
while(1){HID_XFER_PACKET *packet;ResetEvent(events[0]);packet = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*packet) + ext->preparseData->caps.InputReportByteLength);packet->reportBufferLen = ext->preparseData->caps.InputReportByteLength;packet->reportBuffer = ((BYTE*)packet) + sizeof(*packet);packet->reportId = 0;irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,device, NULL, 0, packet, sizeof(packet), TRUE, events[0],&irp_status);irpsp = IoGetNextIrpStackLocation(irp);irpsp->CompletionRoutine = read_Completion;irpsp->Control = SL_INVOKE_ON_SUCCESS | SL_INVOKE_ON_ERROR;
Seems better to implement IoSetCompletionRoutine than to start hand-coding this into drivers. Also I think this event handle would be better suited as the completion routine's context. If/when setting Irp->UserEvent gets implemented in IoCompleteRequest this will break since it's not a KEVENT.
IoCallDriver(device, irp);if (irp->IoStatus.u.Status == STATUS_PENDING)rc = WaitForMultipleObjects(2, events, FALSE, INFINITE);
If a driver returns pending, it owns the IRP and others are forbidden from accessing it (and an IRP's IoStatus.Status should never be STATUS_PENDING anyway). You should check IoCallDriver's return value instead. (There are a couple instances of this pattern across the patch series)
if (irp->IoStatus.u.Status == STATUS_SUCCESS){RingBuffer_Write(ext->ring_buffer, packet);HID_Device_processQueue(device);}IoCompleteRequest(irp, IO_NO_INCREMENT );rc = WaitForSingleObject(ext->halt_event, ext->poll_interval);if (rc == WAIT_OBJECT_0)break;else if (rc != WAIT_TIMEOUT)ERR("Wait returned unexpected value %x\n",rc);}- }
[...] +}
+void handle_minidriver_string(DEVICE_OBJECT *device, IRP *irp, DWORD index) +{
- IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
- WCHAR buffer[127];
- NTSTATUS status;
- status = IoCallMinidriver(IOCTL_HID_GET_STRING, device, (PVOID)index,
sizeof(index), buffer, sizeof(buffer));
Allocating a separate IRP instead of passing down this one would be considered highly wasteful in WDM. I guess this makes for simpler code though.
- if (status == STATUS_SUCCESS)
- {
WCHAR *out_buffer = (WCHAR*)(((BYTE*)irp->MdlAddress->StartVa) + irp->MdlAddress->ByteOffset);int length = irpsp->Parameters.DeviceIoControl.OutputBufferLength/sizeof(WCHAR);TRACE("got string %s from minidriver\n",debugstr_w(buffer));lstrcpynW(out_buffer, buffer, length);irp->IoStatus.Information = (lstrlenW(buffer)+1) * sizeof(WCHAR);- }
- irp->IoStatus.u.Status = status;
+}
+NTSTATUS WINAPI HID_Device_ioctl(DEVICE_OBJECT *device, IRP *irp) +{
- IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
- BASE_DEVICE_EXTENSION *extension = (BASE_DEVICE_EXTENSION*)device->DeviceExtension;
- irp->IoStatus.Information = 0;
- TRACE("device %p ioctl(%x)\n", device, irpsp->Parameters.DeviceIoControl.IoControlCode);
- switch (irpsp->Parameters.DeviceIoControl.IoControlCode)
- {
case IOCTL_HID_GET_POLL_FREQUENCY_MSEC:TRACE("IOCTL_HID_GET_POLL_FREQUENCY_MSEC\n");*((ULONG*)irp->AssociatedIrp.SystemBuffer) = extension->poll_interval;
This should check irpsp->Parameters.DeviceIoControl.OutputBufferLength.
irp->IoStatus.Information = sizeof(ULONG);irp->IoStatus.u.Status = STATUS_SUCCESS;break;case IOCTL_HID_SET_POLL_FREQUENCY_MSEC:{ULONG poll_interval = ((ULONG)irp->AssociatedIrp.SystemBuffer);
Should check for InputBufferLength.
TRACE("IOCTL_HID_SET_POLL_FREQUENCY_MSEC\n");if (poll_interval == 0)FIXME("Handle opportunistic reads\n");else if (poll_interval <= MAX_POLL_INTERVAL_MSEC){extension->poll_interval = poll_interval;irp->IoStatus.u.Status = STATUS_SUCCESS;}elseirp->IoStatus.u.Status = STATUS_INVALID_PARAMETER;break;}[...]
- }
- IoCompleteRequest( irp, IO_NO_INCREMENT );
- return STATUS_SUCCESS;
Returning a failure status (e.g. STATUS_INVALID_PARAMETER) if the IRP failed would be preferable.
+}
+NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp) +{
- HID_XFER_PACKET *packet;
- BASE_DEVICE_EXTENSION *ext = (BASE_DEVICE_EXTENSION*)device->DeviceExtension;
- int buffer_size = RingBuffer_GetBufferSize(ext->ring_buffer);
- /* This is a work around because FsContext is not working in WINE yet */
- static int ptr = -1;
- packet = HeapAlloc(GetProcessHeap(), 0, buffer_size);
+#if 0
- /* When FsContext is working this is how we will track read position
per open file handle */- if (!irp->Tail.Overlay.OriginalFileObject->FsContext)
- {
ptr = RingBuffer_AddPointer(ext->ring_buffer);irp->Tail.Overlay.OriginalFileObject->FsContext = ptr + 1;- }
- else
ptr = irp->Tail.Overlay.OriginalFileObject->FsContext - 1;+#else
- /* Otherwise we use the same pointer for all file handles */
- if (ptr == -1)
ptr = RingBuffer_AddPointer(ext->ring_buffer);- irp->Tail.Overlay.OriginalFileObject->FsContext = (PVOID)ptr;
+#endif
- irp->IoStatus.Information = 0;
- RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size);
- if (buffer_size)
- {
TRACE_(hid_report)("Got Packet %p %i\n", packet->reportBuffer, packet->reportBufferLen);memcpy(irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);
This needs a check against irpsp->Parameters.Read.Length.
irp->IoStatus.Information = packet->reportBufferLen;irp->IoStatus.u.Status = STATUS_SUCCESS;IoCompleteRequest( irp, IO_NO_INCREMENT );- }
- else
- {
irp_entry *entry = HeapAlloc(GetProcessHeap(), 0, sizeof(*entry));
You have both irp->Tail.Overlay.ListEntry and irp->Tail.Overlay.DriverContext at your disposal so this shouldn't be necessary.
TRACE_(hid_report)("Queue irp\n");entry->irp = irp;list_add_tail(&ext->irp_queue, &entry->entry);irp->IoStatus.u.Status = STATUS_PENDING;
This goes with the comment above. The dispatch routine should return STATUS_PENDING (and at least on Windows, call IoMarkIrpPending). irp->IoStatus should then be set before completing the IRP.
- }
- HeapFree(GetProcessHeap(), 0, packet);
- return STATUS_SUCCESS;
+}
+VOID HID_Device_processQueue(DEVICE_OBJECT *device) +{
- irp_entry *entry, *next;
- BASE_DEVICE_EXTENSION *ext = (BASE_DEVICE_EXTENSION*)device->DeviceExtension;
- int buffer_size = RingBuffer_GetBufferSize(ext->ring_buffer);
- HID_XFER_PACKET *packet;
- packet = HeapAlloc(GetProcessHeap(), 0, buffer_size);
- LIST_FOR_EACH_ENTRY_SAFE(entry, next, &ext->irp_queue, irp_entry, entry)
- {
+#if 0
/* See above comment about FsContext */int ptr = (int)entry->irp->Tail.Overlay.OriginalFileObject->FsContext;+#else
int ptr = 0;+#endif
buffer_size = RingBuffer_GetBufferSize(ext->ring_buffer);RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size);if (buffer_size){TRACE_(hid_report)("Processing Request (%i)\n",ptr);list_remove(&entry->entry);memcpy(entry->irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);
Another place where a length check seems appropriate.
entry->irp->IoStatus.Information = packet->reportBufferLen;entry->irp->IoStatus.u.Status = STATUS_SUCCESS;IoCompleteRequest( entry->irp, IO_NO_INCREMENT );HeapFree(GetProcessHeap(), 0, entry);}- }
- HeapFree(GetProcessHeap(), 0, packet);
+}
+NTSTATUS WINAPI HID_Device_write(DEVICE_OBJECT *device, IRP *irp) +{
- IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
- irp->IoStatus.Information = 0;
- TRACE("Buffer length %i\n", irpsp->Parameters.DeviceIoControl.OutputBufferLength);
Should be Parameters.Write.Length.
- FIXME("device %p\n", device);
- irp->IoStatus.u.Status = STATUS_SUCCESS;
- IoCompleteRequest( irp, IO_NO_INCREMENT );
- return STATUS_SUCCESS;
+}
Thanks, Thomas