From: "Rémi Bernon" Subject: Re: [PATCH 5/6] xinput.sys: Create an internal PDO, on the XINPUT bus. Message-Id: <7808547a-0589-aa59-c17a-f4044fe4381e@codeweavers.com> Date: Fri, 27 Aug 2021 22:55:05 +0200 In-Reply-To: <09351bcc-3338-2d03-3256-55e6885031fe@codeweavers.com> References: <20210826055904.828578-1-rbernon@codeweavers.com> <20210826055904.828578-5-rbernon@codeweavers.com> <09351bcc-3338-2d03-3256-55e6885031fe@codeweavers.com> On 8/27/21 10:31 PM, Zebediah Figura (she/her) wrote: >> wcsrchr(device_id, '\\') + 1); >>       wcscpy(ext->instance_id, instance_id); >> +    ext->interface_guid = !wcsncmp(device_id, L"XINPUT\\", 7) ? >> &GUID_DEVINTERFACE_XINPUT >> +                                                              : >> &GUID_DEVINTERFACE_HID; > > It's a matter of taste, I guess, but personally if I have to wrap a > ternary operator, I take it as a sign I should be using if/else instead ;-) > Yeah, I spent a lot of time hesitating on the best looking way to write that. > It also occurs to me that since this is very internal it might be a good > idea to use WINEXINPUT or something rather than just XINPUT. Unlikely to > make a difference, of course, but it also sort of communicates that this > is internal, and more clearly tells anyone looking for the code where to > look for the driver that reports WINEXINPUT devices (viz. elsewhere in > the code tree.) > Works for me, I'm still not convinced that winexinput.sys is much clearer though, but I can probably live with it. >> -            if ((status = IoRegisterDeviceInterface(device, >> &GUID_DEVINTERFACE_HID, NULL, &ext->u.pdo.link_name))) >> +            if ((status = IoRegisterDeviceInterface(device, >> ext->interface_guid, NULL, &ext->u.pdo.link_name))) >>               { >>                   ERR("Failed to register interface, status %#x.\n", >> status); >>                   break; > > Any chance we could put the hidclass hunks in a separate patch? > > ... > That would cause the "internal" "xinput" PDO be momentarily listed in the HID device interface class, as a duplicate device with the gamepad, and although applications aren't supposed to see or use it. Not a big deal, bug probably not very nice. >> @@ -52,14 +54,24 @@ struct device_state >>   { >>       DEVICE_OBJECT *bus_pdo; >>       DEVICE_OBJECT *gamepad_pdo; >> +    DEVICE_OBJECT *xinput_pdo; >>       WCHAR bus_id[MAX_DEVICE_ID_LEN]; >>       WCHAR instance_id[MAX_DEVICE_ID_LEN]; >> + >> +    /* everything below requires holding the cs */ >> +    CRITICAL_SECTION cs; >> +    IRP *pending_read; >> +    BOOL pending_is_gamepad; > > Honestly after thinking about it I don't dislike "internal" as much I > dislike "xinput" now. Because you'd think that "xinput" refers to the > device we access from xinput, but actually it's the other way around. It's still the case there, but eventually (ie: in an eventual PATCH 7/6 where xinput is switched to the internal device interface class), this is how it will indeed be. So to me it's actually intuitive. > Not to mention that calling either device "xinput" sucks when the whole > driver is named "xinput" and it just looks like an namespace prefix. > > Of course "gamepad" isn't great either, because they're both gamepads :-( > > Naming things is hard, of course, and I don't have any great > suggestions. The best I can come up with after thinking about it are > "real" and "fake". Or maybe "private" and "fake", which is a little less > ambiguous as to which one is which. > Well, to me fake or real are really vague concepts, especially when speaking about device. The gamepad is as real as the other, it just adds a translation layer. I don't have any better idea than gamepad/xinput right now tbh. If the driver were to be renamed to winexinput.sys, maybe xinput would be less like a namespace prefix... In any case, I don't think the driver is supposed to become more complicated than these patches (it just misses the report translation and that's it), it should be quite straightforward to get the whole picture. With a few comments maybe? >> + >> +    /* only one read at a time per device from hidclass.sys design */ >> +    assert(pending_is_gamepad != ext->is_gamepad); >> +    gamepad_irp = ext->is_gamepad ? irp : pending; >> +    xinput_irp = ext->is_gamepad ? pending : irp; >> + >> +    offset = state->report_buf[0] ? 0 : 1; >> +    if (!(status = sync_ioctl(state->bus_pdo, IOCTL_HID_READ_REPORT, >> NULL, 0, >> +                              state->report_buf + offset, >> state->report_len - offset))) > > This blocks in the IRP handler. I'm pretty sure this *works* in Wine, > but you aren't really supposed to do this, and I could easily see future > Wine changes causing problems here. (One of the many reasons that Linux > really is the superior operating system...) > I wondered about it for a bit, and it was simpler like this. I guess it could be done with completions without making it it much more complicated... I'll have a try. >> @@ -374,6 +593,7 @@ static NTSTATUS WINAPI add_device(DRIVER_OBJECT >> *driver, DEVICE_OBJECT *bus_pdo) >>       wcscpy(state->bus_id, bus_id); >>       swprintf(ext->device_id, MAX_DEVICE_ID_LEN, L"%s\\%s", bus_id, >> device_id); >>       wcscpy(state->instance_id, instance_id); >> +    RtlInitializeCriticalSection(&state->cs); >>       TRACE("fdo %p, bus %s, device %s, instance %s.\n", fdo, >> debugstr_w(state->bus_id), debugstr_w(ext->device_id), >> debugstr_w(state->instance_id)); >> > > No matching call to RtlDeleteCriticalSection; I guess that should go in > FDO remove. > > Also, missing CS debug info. > Sure. FWIW I don't know if we ever care (certainly not here), but setting the CS debug info causes crashes on Windows. I figured that when I was trying my HID dinput.dll there. -- Rémi Bernon