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.