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.