From: Arkadiusz Hiler Subject: Re: [PATCH 1/3] RawInput: Uppercase the device names up to the GUID. Message-Id: <20210225094640.qc5dqrob6idialzw@failcraft.local> Date: Thu, 25 Feb 2021 11:46:40 +0200 In-Reply-To: References: <20210222115038.359258-1-ahiler@codeweavers.com> <20210224085006.pvv7s5nuuhwpt42l@failcraft.local> <3f31b6ae-277a-d808-b029-8cb40bfd62de@codeweavers.com> <20210224131300.mg4a2ono2stngtt6@failcraft.local> On Wed, Feb 24, 2021 at 02:09:41PM -0600, Zebediah Figura (she/her) wrote: > 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()? SetupAPI's InstanceId is strupr() and DevicePath is 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. I was thinking about doing something along the lines of: SetupDiGetDeviceInstanceId(instance_id); instance_key = RegOpenKey(enum_key, instance_id); NtQueryKey(instance_key, KeyNameInformation, cased_instance_id); /* \Registry\Machine\System\CurrentControlSet\Enum\HID\VID_0000&PID_0000\0&WINEMOUSE&0&0 */ Then we can take everything starting with HID and overwrite a part of the device path, replacing '\' with '#'. If you think that uppercasing (either part of or the whole) device path in user32/rawinput is good enough for now I am more than fine with that. -- Cheers, Arek