[7/11]hidclass.sys: Implement HID devices
Thomas Faber
thomas.faber at reactos.org
Mon Jun 22 05:33:35 CDT 2015
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;
> + }
> + else
> + irp->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
More information about the wine-devel
mailing list