[PATCH 4/5] xinput.sys: Create an internal PDO, on the XINPUT bus.

[Zebediah Figura (she/her)]

On 8/23/21 12:16 PM, Rémi Bernon wrote:
>>> +{
>>> +    struct device_extension *ext = ext_from_DEVICE_OBJECT(device);
>>> +    IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp);
>>> +    ULONG code = stack->Parameters.DeviceIoControl.IoControlCode;
>>> +    struct device_state *state = ext->state;
>>> +
>>> +    TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, 
>>> code, state->bus_pdo);
>>> +
>>> +    switch (code)
>>> +    {
>>> +    case IOCTL_HID_READ_REPORT:
>>> +        if (!set_pending_read(state, irp))
>>> +        {
>>> +            ERR("another read IRP was already pending!\n");
>>> +            irp->IoStatus.Status = STATUS_UNSUCCESSFUL;
>>> +            IoCompleteRequest(irp, IO_NO_INCREMENT);
>>> +            return STATUS_UNSUCCESSFUL;
>>
>> Do we really need to handle this case? Can't we depend on hidclass 
>> acting sanely? At most I imagine printing an ERR is reasonable.
>>
> 
> Well, probably not, and now that I know hidclass.sys is supposed to 
> check about everything about the HID report buffers I'm tempted to 
> remove any kind of checks from the lower-level drivers.
> 
> However, this means eventual bugs in hidclass.sys may then go unnoticed 
> or trigger ugly crashes. I'm personally a fan of asserts for such cases, 
> and I would find it better than an error message without error 
> propagation, if that's alright?

An assert sounds reasonable to me too, yeah.

> 
>>> +        }
>>> +        return STATUS_PENDING;
>>> +
>>> +    case IOCTL_HID_GET_INPUT_REPORT:
>>> +    {
>>> +        HID_XFER_PACKET *packet = (HID_XFER_PACKET *)irp->UserBuffer;
>>> +
>>> +        RtlEnterCriticalSection(&state->cs);
>>> +        memcpy(packet->reportBuffer, state->report_buf, 
>>> state->report_len);
>>> +        irp->IoStatus.Information = state->report_len;
>>> +        RtlLeaveCriticalSection(&state->cs);
>>> +
>>> +        irp->IoStatus.Status = STATUS_SUCCESS;
>>> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
>>> +        return STATUS_SUCCESS;
>>> +    }
>>> +
>>> +    default:
>>> +        IoSkipCurrentIrpStackLocation(irp);
>>> +        return IoCallDriver(state->bus_pdo, irp);
>>> +    }
>>> +
>>> +    return STATUS_SUCCESS;
>>> +}
>>> +
>>> +static NTSTATUS WINAPI hid_read_report_completion(DEVICE_OBJECT 
>>> *device, IRP *irp, void *context)
>>> +{
>>> +    struct device_extension *ext = ext_from_DEVICE_OBJECT(device);
>>> +    ULONG read_len = irp->IoStatus.Information;
>>> +    struct device_state *state = ext->state;
>>> +    char *read_buf = irp->UserBuffer;
>>> +
>>> +    TRACE("device %p, irp %p, bus_pdo %p.\n", device, irp, 
>>> state->bus_pdo);
>>> +
>>> +    RtlEnterCriticalSection(&state->cs);
>>> +    if (!state->report_buf) WARN("report buffer not created yet.\n");
>>> +    else if (read_len <= state->report_len) 
>>> memcpy(state->report_buf, read_buf, read_len);
>>> +    else ERR("report length mismatch %u, expected %u\n", read_len, 
>>> state->report_len);
>>> +    RtlLeaveCriticalSection(&state->cs);
>>> +
>>> +    if (irp->PendingReturned) IoMarkIrpPending(irp);
>>> +    return STATUS_SUCCESS;
>>> +}
>>> +
>>>   static NTSTATUS WINAPI internal_ioctl(DEVICE_OBJECT *device, IRP *irp)
>>>   {
>>>       struct device_extension *ext = ext_from_DEVICE_OBJECT(device);
>>>       IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp);
>>>       ULONG code = stack->Parameters.DeviceIoControl.IoControlCode;
>>>       struct device_state *state = ext->state;
>>> +    IRP *pending;
>>>       if (InterlockedOr(&ext->removed, FALSE))
>>>       {
>>> @@ -85,12 +197,86 @@ static NTSTATUS WINAPI 
>>> internal_ioctl(DEVICE_OBJECT *device, IRP *irp)
>>>           IoCompleteRequest(irp, IO_NO_INCREMENT);
>>>       }
>>> +    if (ext->is_xinput) return xinput_ioctl(device, irp);
>>> +
>>>       TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, 
>>> irp, code, state->bus_pdo);
>>> -    IoSkipCurrentIrpStackLocation(irp);
>>> +    if (code == IOCTL_HID_READ_REPORT)
>>> +    {
>>> +        /* we completed the previous internal read, send the fixed 
>>> up report to xinput pdo */
>>> +        if ((pending = pop_pending_read(state)))
>>> +        {
>>> +            RtlEnterCriticalSection(&state->cs);
>>> +            memcpy(pending->UserBuffer, state->report_buf, 
>>> state->report_len);
>>> +            pending->IoStatus.Information = state->report_len;
>>> +            RtlLeaveCriticalSection(&state->cs);
>>> +
>>> +            pending->IoStatus.Status = irp->IoStatus.Status;
>>> +            IoCompleteRequest(pending, IO_NO_INCREMENT);
>>> +        }
>>> +
>>> +        IoCopyCurrentIrpStackLocationToNext(irp);
>>> +        IoSetCompletionRoutine(irp, hid_read_report_completion, 
>>> NULL, TRUE, FALSE, FALSE);
>>> +    }
>>
>> Doesn't this break if you get more than one ioctl on the "internal" 
>> PDO without any on the "xinput" PDO in between?
>>
> 
> I'm not sure to see how? The "internal" read will just complete normally 
> and skip notifying the xinput PDO, which will possibly miss a report but 
> hopefully will catch up on the next read.
> 
> If you mean the two should run in lock-step so that xinput never misses 
> a report, then it will need to be done differently.

I'm not familiar with the way hidclass works, but I'm assuming that it 
needs to see "every" report for both devices? If not, I'm not sure why 
we need to bother waiting for the "xinput" ioctl in order to complete 
the "internal" ioctl.