[PATCH 4/5] xinput.sys: Create an internal PDO, on the XINPUT bus.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Mon Aug 23 11:58:01 CDT 2021
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?
> +{
> + 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.
> + }
> + 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?
> + 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.
More information about the wine-devel
mailing list