[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