From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 5/6] xinput.sys: Create an internal PDO, on the XINPUT bus. Message-Id: <9261d117-2adb-37e2-6740-9c297b468c55@codeweavers.com> Date: Mon, 30 Aug 2021 11:22:02 -0500 In-Reply-To: <7808547a-0589-aa59-c17a-f4044fe4381e@codeweavers.com> References: <20210826055904.828578-1-rbernon@codeweavers.com> <20210826055904.828578-5-rbernon@codeweavers.com> <09351bcc-3338-2d03-3256-55e6885031fe@codeweavers.com> <7808547a-0589-aa59-c17a-f4044fe4381e@codeweavers.com> On 8/27/21 3:55 PM, Rémi Bernon wrote: > 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. I'm fine with xinput.sys, but I'd rather see the device ID named WINEXINPUT. > >>> -            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. Yeah, I meant putting them first, as you seem to have realized. > >>> @@ -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. Well, right, I'm talking about the eventual situation. And, I hate to say it, but "to me it's actually intuitive" isn't great :-( >> 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? I think a comment near the top is definitely going to be necessary regardless of what terminology we eventually use. My logic with "real"/"fake" is that the "real" device reports the "real" data that we get from winebus; the "fake" device fixes it up and hides some of it. But yeah, it's clearly not unambiguous either. The best thing it has going for it is that they're clearly attributes of the child devices rather than namespace prefixes or referring to internal IOCTLs or anything else. >>> + >>> +    /* 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. It doesn't sound too bad from my armchair ;-) >>> @@ -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. > I think we usually don't worry about it. The only instance I know offhand is strmbase, which is used to build Windows tests. See strmbase_filter_init() and strmbase_filter_cleanup() for what to do in such a case.