On 8/23/21 6:58 PM, Zebediah Figura (she/her) wrote:
On 8/18/21 2:31 AM, Rémi Bernon wrote:
+static NTSTATUS WINAPI xinput_ioctl(DEVICE_OBJECT *device, IRP *irp)
Can we please keep "internal" in the name?
Sure, I may have to find a better name for the other "internal" PDO then, I named it xinput_ioctl as opposed to the other internal_ioctl.
+{ + 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?
+ } + 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.
+ else IoSkipCurrentIrpStackLocation(irp); return IoCallDriver(state->bus_pdo, irp); }
...
@@ -152,21 +338,33 @@ static NTSTATUS WINAPI pdo_pnp(DEVICE_OBJECT *device, IRP *irp) struct device_state *state = ext->state; ULONG code = stack->MinorFunction; NTSTATUS status; + IRP *pending; TRACE("device %p, irp %p, code %#x, bus_pdo %p.\n", device, irp, code, state->bus_pdo); switch (code) { case IRP_MN_START_DEVICE: - status = STATUS_SUCCESS; + if (ext->is_xinput) status = xinput_pdo_start_device(device); + else status = STATUS_SUCCESS; break; case IRP_MN_SURPRISE_REMOVAL: status = STATUS_SUCCESS; if (InterlockedExchange(&ext->removed, TRUE)) break;
+ if (!ext->is_xinput) break;
+ if ((pending = pop_pending_read(state))) + { + pending->IoStatus.Status = STATUS_DELETE_PENDING; + pending->IoStatus.Information = 0; + IoCompleteRequest(pending, IO_NO_INCREMENT); + }
Now that you're doing something nontrivial in your remove, using atomic functions to check "removed" is no longer thread-safe. In this case you can get a pending read queued after this IRP_MN_SURPRISE_REMOVAL returns.
Yeah of course...