From: "Zebediah Figura (she/her)" Subject: Re: [PATCH 1/3] RawInput: Uppercase the device names up to the GUID. Message-Id: Date: Wed, 24 Feb 2021 14:09:41 -0600 In-Reply-To: <20210224131300.mg4a2ono2stngtt6@failcraft.local> References: <20210222115038.359258-1-ahiler@codeweavers.com> <20210224085006.pvv7s5nuuhwpt42l@failcraft.local> <3f31b6ae-277a-d808-b029-8cb40bfd62de@codeweavers.com> <20210224131300.mg4a2ono2stngtt6@failcraft.local> On 2/24/21 7:13 AM, Arkadiusz Hiler wrote: > On Wed, Feb 24, 2021 at 12:54:42PM +0100, Rémi Bernon wrote: >> On 2/24/21 9:50 AM, Arkadiusz Hiler wrote: >>> On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote: >>>> On 2/22/21 12:50 PM, Arkadiusz Hiler wrote: >>>>> This is a preparation for a patch that changes setupapi to return lowercased >>>>> device paths, which is the source of RawInput device names. On Windows >>>>> RawInput returns mostly uppercase names (except the GUID part) and we want to >>>>> keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ >>>>> to match xinput devices to their RawInput counterparts. >>>>> >>>>> Signed-off-by: Arkadiusz Hiler >>>>> --- >>>>> dlls/user32/rawinput.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c >>>>> index ba11a121bc5..13aac0278d5 100644 >>>>> --- a/dlls/user32/rawinput.c >>>>> +++ b/dlls/user32/rawinput.c >>>>> @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) >>>>> SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; >>>>> struct device *device; >>>>> HANDLE file; >>>>> - WCHAR *path; >>>>> + WCHAR *path, *pos; >>>>> DWORD size; >>>>> SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL); >>>>> @@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) >>>>> } >>>>> heap_free(detail); >>>>> + /* upper case everything but the GUID */ >>>>> + for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos); >>>>> + >>>>> file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, >>>>> FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); >>>>> if (file == INVALID_HANDLE_VALUE) >>>>> >>>> >>>> Hi Arek! >>>> >>>> I'd think a test to validate this could be nice. I know we don't have any >>>> gamepad device available on the test bot, but maybe it's still possible to >>>> validate at least with with mouse / keyboard devices? It would also let us >>>> check the test manually on a real Windows with a gamepad plugged in. >>> >>> Hey, >>> >>> Thanks for the feedback. >>> >>> Looking at this a bit closer there seems to be more going on with the >>> paths on Windows: >>> >>> rawinput: \\?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>> setupapi: \\?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>> instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000 >>> >>> or >>> >>> rawinput: \\?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>> setupapi: \\?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} >>> instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000 >>> >>> It's not even uppercasing up to second '#'. It looks like there's >>> another source of path/instance_id that is case sensitive (Col01 vs >>> COL01). So having a test that checks if everything is uppercase would >>> fail on Windows. >>> >>> I would treat the patch in current form more as "keeping the old >>> behavior that we know some software depends on[0]" instead of "fixing >>> this once and for all". >>> >>> [0]: https://github.com/libsdl-org/SDL/blob/fadfa5/src/joystick/windows/SDL_rawinputjoystick.c#L696 >>> >> >> Alright, then maybe not checking the whole string but at least a test >> reflecting the SDL assumption, with a comment pointing to it? > > CCed zf as she may have something to say. > > I may take a stab at a proper fix first though. > > 1. Both CM_Get_DeviceID() and SetupDiGetDeviceInstanceId() return strupr() version. > Looks like we handle this correctly. I assume you mean strlwr()? > > 2. RawInput seems to be using the values verbatim from the registry as they are in > HLM/System/CurrentControlSet/Enum and HLM/System/CurrentControlSet/Control/DeviceClasses > > Looks like we need to implement opening relevant keys and using > pathing from there in rawinput.c (NtQueryKey()?) > > We also need to add tests to SetupAPI to make sure that the casing is > preserved in the registy entries. > Yeah, this is what I noticed when I tested. If you manually change the casing with regedit (and log out and back in), user32 will even reflect that. That said, my take is it's probably not worth reimplementing (admittedly not that much of) setupapi just for this, at least, not if we can just uppercase the whole string and have done. > 3. Our casing in the registry is wrong depending on the source of HID > devices, winebus.sys requires some fixes too. >