[PATCH 1/3] RawInput: Uppercase the device names up to the GUID.

[Arkadiusz Hiler]

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 <ahiler at codeweavers.com>
> > > > > > ---
> > > > > >     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