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

[Zebediah Figura (she/her)]

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.