Re: [7/11]hidclass.sys: Implement HID devices

[Aric Stewart]

On 6/22/15 5:33 AM, Thomas Faber wrote:
> 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).

Lots of great points here! Thanks a lot!
> 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.

I did some digging and learned that IoSetCompletionRoutine is just a macro anyway. So I took this route.
> > -
> >        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)

You are correct that I am doing this very wrong in a number of places. Thanks for pointing it out. I will make sure that I fix this issue. I think the issue is also in my minidriver where I am putting STATUS_PENDING into the IoStatus.Status and returning ERROR_SUCCESS from my ioctl handler.
> > -
> >        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.

Yeah, I was going for simpler code. We are not constrained in resources like I believe you are in the actual windows kernel. In wine this is still still user space. Which I admit makes it all sort of strange. But for now I though simplicity was best for this first pass.
> > -
> >    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.

Thanks for this point! I totally missed that!
> > -
> >    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.

yeah, as above I am totally mishandling the pending status.

thanks!
-aric