[PATCH 4/5] xinput.sys: Create an internal PDO, on the XINPUT bus.
Rémi Bernon
rbernon at codeweavers.com
Mon Aug 23 12:16:20 CDT 2021
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...
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list