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

[Rémi Bernon]

On 8/23/21 7:23 PM, Zebediah Figura (she/her) wrote:
> 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.
> 

Well, each report contains the full state of the device, hidclass.sys 
just pass them to the clients and doesn't care about missing reports.

The way it's done here it is possible that the "internal" PDO will get 
more reports than the "xinput" PDO, and applications reading the 
"xinput" devices may then miss some input events as a consequence.

In general I think we should avoid as much as possible cases where 
reports could be missed or dropped, to avoid missing input events. Some 
people already reported that sometimes Wine misses a button press for 
instance.

(FWIW I suspect it may come from winebus.sys current design, which 
already makes it possible to miss reports, and which I'm planning to fix 
at some point.)

There's a ring-buffer for every reader with a user-space configurable 
size in hidclass.sys and I believe that's the only place were we're 
allowed to drop a report.

Then, in this case I considered that the bogus "xinput" devices were 
already bogus so didn't deserve much effort, but as some code seems to 
rely on them maybe we should avoid losing reports in all cases. And so 
we'll have to make both run in lock-step.
-- 
Rémi Bernon <rbernon at codeweavers.com>