[PATCH 1/5] ntoskrnl: Implement IoSetDevicePropertyData().

[Rémi Bernon]

From: Arkadiusz Hiler

Signed-off-by: Rémi Bernon 
---

In PATCH 1, the return GetLastError() look wrong to me but the same is
done in IoGetDeviceProperty so I kept them. I guess we would need to
convert the setupapi error back to NTSTATUS, or implement the higher
level setupapi on top of these lower level primitives intead.

In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
because I don't know what the call is actually expecting. Maybe it was
unnecessary.

dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  1 +
 dlls/ntoskrnl.exe/pnp.c             | 51 +++++++++++++++++++++++++++++
 include/ddk/wdm.h                   |  4 +++
 3 files changed, 56 insertions(+)

diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
index 2e5e2e6e11b..4eb08faec2e 100644
--- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
+++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
@@ -465,6 +465,7 @@
 @ stdcall IoReuseIrp(ptr long)
 @ stub IoSetCompletionRoutineEx
 @ stdcall IoSetDeviceInterfaceState(ptr long)
+@ stdcall IoSetDevicePropertyData(ptr ptr long long long long ptr)
 @ stub IoSetDeviceToVerify
 @ stub IoSetFileOrigin
 @ stub IoSetHardErrorOrVerifyDevice
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c
index 095344b1073..e425712d738 100644
--- a/dlls/ntoskrnl.exe/pnp.c
+++ b/dlls/ntoskrnl.exe/pnp.c
@@ -38,6 +38,12 @@ DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);

WINE_DEFAULT_DEBUG_CHANNEL(plugplay);

+static inline const char *debugstr_propkey( const DEVPROPKEY *id )
+{
+    if (!id) return "(null)";
+    return wine_dbg_sprintf( "{%s,%04x}", wine_dbgstr_guid( &id->fmtid ), id->pid );
+}
+
 #define MAX_SERVICE_NAME 260

struct device_interface
@@ -783,6 +789,51 @@ NTSTATUS WINAPI IoSetDeviceInterfaceState( UNICODE_STRING *name, BOOLEAN enable
     return ret;
 }

+/***********************************************************************
+ *           IoSetDevicePropertyData (NTOSKRNL.EXE.@)
+ */
+NTSTATUS WINAPI IoSetDevicePropertyData( DEVICE_OBJECT *device, const DEVPROPKEY *property_key, LCID lcid,
+                                         ULONG flags, DEVPROPTYPE type, ULONG size, void *data )
+{
+    SP_DEVINFO_DATA sp_device = {sizeof(sp_device)};
+    WCHAR device_instance_id[MAX_DEVICE_ID_LEN];
+    NTSTATUS status;
+    HDEVINFO set;
+
+    TRACE( "device %p, property_key %s, lcid %#x, flags %#x, type %#x, size %u, data %p.\n",
+           device, debugstr_propkey(property_key), lcid, flags, type, size, data );
+
+    /* flags is always treated as PLUGPLAY_PROPERTY_PERSISTENT starting with Win 8 / 2012 */
+
+    if (lcid != LOCALE_NEUTRAL) FIXME( "only LOCALE_NEUTRAL is supported\n" );
+
+    if ((status = get_device_instance_id( device, device_instance_id ))) return status;
+
+    if ((set = SetupDiCreateDeviceInfoList( &GUID_NULL, NULL )) == INVALID_HANDLE_VALUE)
+    {
+        ERR( "Failed to create device list, error %#x.\n", GetLastError() );
+        return GetLastError();
+    }
+
+    if (!SetupDiOpenDeviceInfoW( set, device_instance_id, NULL, 0, &sp_device ))
+    {
+        ERR( "Failed to open device, error %#x.\n", GetLastError() );
+        SetupDiDestroyDeviceInfoList( set );
+        return GetLastError();
+    }
+
+    if (!SetupDiSetDevicePropertyW( set, &sp_device, property_key, type, data, size, 0 ))
+    {
+        ERR( "Failed to set property, error %#x.\n", GetLastError() );
+        SetupDiDestroyDeviceInfoList( set );
+        return GetLastError();
+    }
+
+    SetupDiDestroyDeviceInfoList( set );
+
+    return STATUS_SUCCESS;
+}
+
 /***********************************************************************
  *           IoRegisterDeviceInterface (NTOSKRNL.EXE.@)
  */
diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
index 51097bfe3ab..447833c4bc9 100644
--- a/include/ddk/wdm.h
+++ b/include/ddk/wdm.h
@@ -21,6 +21,7 @@
 #define _NTDDK_

#include <ntstatus.h>
+#include <devpropdef.h>

#ifdef _WIN64
 #define POINTER_ALIGNMENT DECLSPEC_ALIGN(8)
@@ -1681,6 +1682,8 @@ void      WINAPI ExReleaseResourceForThreadLite(ERESOURCE*,ERESOURCE_THREAD);
 ULONG     WINAPI ExSetTimerResolution(ULONG,BOOLEAN);
 void      WINAPI ExUnregisterCallback(void*);

+#define PLUGPLAY_PROPERTY_PERSISTENT 0x0001
+
 void      WINAPI IoAcquireCancelSpinLock(KIRQL*);
 NTSTATUS  WINAPI IoAcquireRemoveLockEx(IO_REMOVE_LOCK*,void*,const char*,ULONG, ULONG);
 NTSTATUS  WINAPI IoAllocateDriverObjectExtension(PDRIVER_OBJECT,PVOID,ULONG,PVOID*);
@@ -1728,6 +1731,7 @@ void      WINAPI IoReleaseRemoveLockAndWaitEx(IO_REMOVE_LOCK*,void*,ULONG);
 void      WINAPI IoReleaseRemoveLockEx(IO_REMOVE_LOCK*,void*,ULONG);
 void      WINAPI IoReuseIrp(IRP*,NTSTATUS);
 NTSTATUS  WINAPI IoSetDeviceInterfaceState(UNICODE_STRING*,BOOLEAN);
+NTSTATUS  WINAPI IoSetDevicePropertyData(DEVICE_OBJECT*,const DEVPROPKEY*,LCID,ULONG,DEVPROPTYPE,ULONG,void*);
 NTSTATUS  WINAPI IoWMIRegistrationControl(PDEVICE_OBJECT,ULONG);

void    FASTCALL KeAcquireInStackQueuedSpinLockAtDpcLevel(KSPIN_LOCK*,KLOCK_QUEUE_HANDLE*);

[PATCH 2/5] hidclass.sys: Assign rawinput handles through device properties.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---
 dlls/hidclass.sys/device.c |  4 ----
 dlls/hidclass.sys/pnp.c    | 18 ++++++++++++++++--
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
index a7825080deb..7dc97781b6b 100644
--- a/dlls/hidclass.sys/device.c
+++ b/dlls/hidclass.sys/device.c
@@ -21,7 +21,6 @@
 #include <stdarg.h>
 #define NONAMELESSUNION
 #define NONAMELESSSTRUCT
-#include "initguid.h"
 #include "hid.h"
 #include "winreg.h"
 #include "winuser.h"
@@ -30,9 +29,6 @@
 #include "ddk/hidsdi.h"
 #include "ddk/hidtypes.h"
 #include "ddk/wdm.h"
-#include "devguid.h"
-#include "ntddmou.h"
-#include "ntddkbd.h"

WINE_DEFAULT_DEBUG_CHANNEL(hid);
 WINE_DECLARE_DEBUG_CHANNEL(hid_report);
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c
index 3d81c16356e..2769b78c13b 100644
--- a/dlls/hidclass.sys/pnp.c
+++ b/dlls/hidclass.sys/pnp.c
@@ -21,7 +21,10 @@
 #define NONAMELESSUNION
 #include <unistd.h>
 #include <stdarg.h>
+#include "initguid.h"
 #include "hid.h"
+#include "devguid.h"
+#include "devpkey.h"
 #include "ntddmou.h"
 #include "ntddkbd.h"
 #include "ddk/hidtypes.h"
@@ -33,6 +36,8 @@

WINE_DEFAULT_DEBUG_CHANNEL(hid);

+DEFINE_DEVPROPKEY(DEVPROPKEY_HID_HANDLE, 0xbc62e415, 0xf4fe, 0x405c, 0x8e, 0xda, 0x63, 0x6f, 0xb5, 0x9f, 0x08, 0x98, 2);
+
 #if defined(__i386__) && !defined(_WIN32)

extern void * WINAPI wrap_fastcall_func1( void *func, const void *a );
@@ -231,8 +236,6 @@ static void create_child(minidriver *minidriver, DEVICE_OBJECT *fdo)

pdo_ext->u.pdo.information.DescriptorSize = pdo_ext->u.pdo.preparsed_data->dwSize;

-    IoInvalidateDeviceRelations(fdo_ext->u.fdo.hid_ext.PhysicalDeviceObject, BusRelations);
-
     page = pdo_ext->u.pdo.preparsed_data->caps.UsagePage;
     usage = pdo_ext->u.pdo.preparsed_data->caps.Usage;
     if (page == HID_USAGE_PAGE_GENERIC && usage == HID_USAGE_GENERIC_MOUSE)
@@ -242,6 +245,17 @@ static void create_child(minidriver *minidriver, DEVICE_OBJECT *fdo)
     else
         pdo_ext->u.pdo.rawinput_handle = alloc_rawinput_handle();

+    if ((status = IoSetDevicePropertyData(child_pdo, &DEVPROPKEY_HID_HANDLE, LOCALE_NEUTRAL,
+                                          PLUGPLAY_PROPERTY_PERSISTENT, DEVPROP_TYPE_UINT32,
+                                          sizeof(pdo_ext->u.pdo.rawinput_handle), &pdo_ext->u.pdo.rawinput_handle)))
+    {
+        ERR("Failed to set device handle property, status %x\n", status);
+        IoDeleteDevice(child_pdo);
+        return;
+    }
+
+    IoInvalidateDeviceRelations(fdo_ext->u.fdo.hid_ext.PhysicalDeviceObject, BusRelations);
+
     pdo_ext->u.pdo.poll_interval = DEFAULT_POLL_INTERVAL;

pdo_ext->u.pdo.ring_buffer = RingBuffer_Create(

[PATCH 2/5] hidclass.sys: Assign rawinput handles through device properties.

[Arkadiusz Hiler]

On Mon, Apr 26, 2021 at 12:38:00PM +0200, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon

As I've said on the previous revision - this seems to be in line what
Windows is doing. The rawinput handle device handles are non-pointer
integer values that are constant between processes.

I like how simple this got with the recent changes in the area :-)

Signed-off-by: Arkadiusz Hiler

[PATCH 3/5] user32: Read rawinput device handles assigned by hidclass.sys.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---
 dlls/user32/rawinput.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c
index 129c1933c36..ad93517ae8a 100644
--- a/dlls/user32/rawinput.c
+++ b/dlls/user32/rawinput.c
@@ -38,11 +38,14 @@
 #include "user_private.h"

#include "initguid.h"
+#include "devpkey.h"
 #include "ntddmou.h"
 #include "ntddkbd.h"

WINE_DEFAULT_DEBUG_CHANNEL(rawinput);

+DEFINE_DEVPROPKEY(DEVPROPKEY_HID_HANDLE, 0xbc62e415, 0xf4fe, 0x405c, 0x8e, 0xda, 0x63, 0x6f, 0xb5, 0x9f, 0x08, 0x98, 2);
+
 struct device
 {
     SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail;
@@ -92,17 +95,27 @@ static BOOL array_reserve(void **elements, unsigned int *capacity, unsigned int

static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface)
 {
+    SP_DEVINFO_DATA device_data = {sizeof(device_data)};
     SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail;
     struct device *device;
+    UINT32 handle;
     HANDLE file;
-    DWORD size;
+    DWORD size, type;

-    SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
+    SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, &device_data);
     if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
     {
         ERR("Failed to get device path, error %#x.\n", GetLastError());
         return FALSE;
     }
+
+    if (!SetupDiGetDevicePropertyW(set, &device_data, &DEVPROPKEY_HID_HANDLE, &type, (BYTE *)&handle, sizeof(handle), NULL, 0) ||
+        type != DEVPROP_TYPE_UINT32)
+    {
+        ERR("Failed to get device handle, error %#x.\n", GetLastError());
+        return NULL;
+    }
+
     if (!(detail = malloc(size)))
     {
         ERR("Failed to allocate memory.\n");
@@ -111,7 +124,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface)
     detail->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W);
     SetupDiGetDeviceInterfaceDetailW(set, iface, detail, size, NULL, NULL);

-    TRACE("Found HID device %s.\n", debugstr_w(detail->DevicePath));
+    TRACE("Found device %x / %s.\n", handle, debugstr_w(detail->DevicePath));

file = CreateFileW(detail->DevicePath, GENERIC_READ | GENERIC_WRITE,
             FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0);

[PATCH 3/5] user32: Read rawinput device handles assigned by hidclass.sys.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=89334

Your paranoid android.

=== debiant2 (32 bit Chinese:China report) ===

user32:
win.c:10766: Test failed: 000D0110: expected prev 00430124, got 00000000
win.c:10780: Test failed: hwnd should NOT be topmost
win.c:10782: Test failed: 000D0110: expected NOT topmost
win.c:10726: Test failed: 1: hwnd 000D0110 is still topmost

[PATCH 3/5] user32: Read rawinput device handles assigned by hidclass.sys.

[Arkadiusz Hiler]

On Mon, Apr 26, 2021 at 12:38:01PM +0200, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon

Signed-off-by: Arkadiusz Hiler

[PATCH 4/5] user32: Use rawinput device handles to identify devices.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---
 dlls/user32/rawinput.c    | 32 +++++++++++++++++++++++++++++---
 dlls/user32/tests/input.c |  2 +-
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c
index ad93517ae8a..a2cf5769fcc 100644
--- a/dlls/user32/rawinput.c
+++ b/dlls/user32/rawinput.c
@@ -50,6 +50,7 @@ struct device
 {
     SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail;
     HANDLE file;
+    HANDLE handle;
     RID_DEVICE_INFO info;
     PHIDP_PREPARSED_DATA data;
 };
@@ -100,7 +101,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface)
     struct device *device;
     UINT32 handle;
     HANDLE file;
-    DWORD size, type;
+    DWORD i, size, type;

SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, &device_data);
     if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
@@ -116,6 +117,15 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface)
         return NULL;
     }

+    for (i = 0; i < rawinput_devices_count; ++i)
+    {
+        if (rawinput_devices[i].handle == UlongToHandle(handle))
+        {
+            TRACE("Updating device %x / %s\n", handle, debugstr_w(rawinput_devices[i].detail->DevicePath));
+            return rawinput_devices + i;
+        }
+    }
+
     if (!(detail = malloc(size)))
     {
         ERR("Failed to allocate memory.\n");
@@ -147,6 +157,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface)
     device = &rawinput_devices[rawinput_devices_count++];
     device->detail = detail;
     device->file = file;
+    device->handle = ULongToHandle(handle);
     device->info.cbSize = sizeof(RID_DEVICE_INFO);
     device->data = NULL;

@@ -244,6 +255,20 @@ static void find_devices(void)
 }

+static struct device *find_device_from_handle(HANDLE handle)
+{
+    UINT i;
+    for (i = 0; i < rawinput_devices_count; ++i)
+        if (rawinput_devices[i].handle == handle)
+            return rawinput_devices + i;
+    find_devices();
+    for (i = 0; i < rawinput_devices_count; ++i)
+        if (rawinput_devices[i].handle == handle)
+            return rawinput_devices + i;
+    return NULL;
+}
+
+
 struct rawinput_thread_data *rawinput_thread_data(void)
 {
     struct user_thread_info *thread_info = get_user_thread_info();
@@ -401,7 +426,7 @@ UINT WINAPI GetRawInputDeviceList(RAWINPUTDEVICELIST *devices, UINT *device_coun

for (i = 0; i < rawinput_devices_count; ++i)
     {
-        devices[2 + i].hDevice = &rawinput_devices[i];
+        devices[2 + i].hDevice = rawinput_devices[i].handle;
         devices[2 + i].dwType = rawinput_devices[i].info.dwType;
     }

@@ -671,7 +696,7 @@ UINT WINAPI GetRawInputDeviceInfoW(HANDLE handle, UINT command, void *data, UINT
     static const RID_DEVICE_INFO_MOUSE mouse_info = {1, 5, 0, FALSE};

RID_DEVICE_INFO info;
-    struct device *device = handle;
+    struct device *device;
     const void *to_copy;
     UINT to_copy_bytes, avail_bytes;

@@ -679,6 +704,7 @@ UINT WINAPI GetRawInputDeviceInfoW(HANDLE handle, UINT command, void *data, UINT
             handle, command, data, data_size);

if (!data_size) return ~0U;
+    if (!(device = find_device_from_handle(handle))) return ~0U;

/* each case below must set:
      *     *data_size: length (meaning defined by command) of data we want to copy
diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c
index 2397b4104e4..b86194e6d0d 100644
--- a/dlls/user32/tests/input.c
+++ b/dlls/user32/tests/input.c
@@ -1885,7 +1885,7 @@ static void test_GetRawInputDeviceList(void)
          * understand that; so use the \?\ prefix instead */
         name[1] = '\';
         file = CreateFileW(name, 0, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL);
-        todo_wine_if(i == 0 || i == 1)
+        todo_wine_if(info.dwType == RIM_TYPEMOUSE || info.dwType == RIM_TYPEKEYBOARD)
             ok(file != INVALID_HANDLE_VALUE, "Failed to open %s, error %u\n", wine_dbgstr_w(name), GetLastError());

sz = 0;

[PATCH 4/5] user32: Use rawinput device handles to identify devices.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=89335

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

user32:
input.c:756: Test failed:  0 (a4/0): 00 from 00 -> 80 unexpected
input.c:756: Test failed:  0 (a4/0): 41 from 01 -> 00 unexpected

=== w7u_el (32 bit report) ===

user32:
input.c:4292: Test failed: SendInput triggered unexpected message 0xc042

=== wvistau64 (64 bit report) ===

user32:
input.c:756: Test failed:  0 (a4/0): 01 from 01 -> 00 unexpected
input.c:756: Test failed:  0 (a4/0): 11 from 01 -> 00 unexpected
input.c:756: Test failed:  0 (a4/0): a2 from 01 -> 00 unexpected

=== w1064 (64 bit report) ===

user32:
input.c:3258: Test failed: expected WM_LBUTTONDOWN message
input.c:3259: Test failed: expected WM_LBUTTONUP message
input.c:3286: Test failed: expected WM_NCHITTEST message
input.c:3287: Test failed: expected WM_RBUTTONDOWN message
input.c:3288: Test failed: expected WM_RBUTTONUP message
input.c:3317: Test failed: expected WM_LBUTTONDOWN message
input.c:3318: Test failed: expected WM_LBUTTONUP message
input.c:3371: Test failed: expected loop with WM_NCHITTEST messages
input.c:3424: Test failed: expected WM_LBUTTONDOWN message
input.c:3425: Test failed: expected WM_LBUTTONUP message
input.c:1322: Test failed: GetCursorPos: (99,100)

[PATCH 4/5] user32: Use rawinput device handles to identify devices.

[Arkadiusz Hiler]

On Mon, Apr 26, 2021 at 12:38:02PM +0200, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon

Signed-off-by: Arkadiusz Hiler

[PATCH 5/5] user32: Remove hardcoded devices from GetRawInputDeviceList.

[Rémi Bernon]

Signed-off-by: Rémi Bernon 
---
 dlls/user32/rawinput.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c
index a2cf5769fcc..5c009624fe6 100644
--- a/dlls/user32/rawinput.c
+++ b/dlls/user32/rawinput.c
@@ -408,29 +408,24 @@ UINT WINAPI GetRawInputDeviceList(RAWINPUTDEVICELIST *devices, UINT *device_coun

if (!devices)
     {
-        *device_count = 2 + rawinput_devices_count;
+        *device_count = rawinput_devices_count;
         return 0;
     }

-    if (*device_count < 2 + rawinput_devices_count)
+    if (*device_count < rawinput_devices_count)
     {
         SetLastError(ERROR_INSUFFICIENT_BUFFER);
-        *device_count = 2 + rawinput_devices_count;
+        *device_count = rawinput_devices_count;
         return ~0U;
     }

-    devices[0].hDevice = WINE_MOUSE_HANDLE;
-    devices[0].dwType = RIM_TYPEMOUSE;
-    devices[1].hDevice = WINE_KEYBOARD_HANDLE;
-    devices[1].dwType = RIM_TYPEKEYBOARD;
-
     for (i = 0; i < rawinput_devices_count; ++i)
     {
-        devices[2 + i].hDevice = rawinput_devices[i].handle;
-        devices[2 + i].dwType = rawinput_devices[i].info.dwType;
+        devices[i].hDevice = rawinput_devices[i].handle;
+        devices[i].dwType = rawinput_devices[i].info.dwType;
     }

-    return 2 + rawinput_devices_count;
+    return rawinput_devices_count;
 }

/***********************************************************************

[PATCH 5/5] user32: Remove hardcoded devices from GetRawInputDeviceList.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=89336

Your paranoid android.

=== debiant2 (32 bit Chinese:China report) ===

user32:
win.c:11487: Test failed: got normal pos (200,88)-(400,288)
win.c:11490: Test failed: got window rect (200,88)-(400,288)
win.c:11503: Test failed: got normal pos (200,88)-(400,288)

=== debiant2 (64 bit WoW report) ===

user32:
win.c:10102: Test failed: Expected foreground window 00000000000E0120, got 0000000000EE0050

[PATCH 5/5] user32: Remove hardcoded devices from GetRawInputDeviceList.

[Arkadiusz Hiler]

On Mon, Apr 26, 2021 at 12:38:03PM +0200, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon

I think it may be better to move this patch one up or squash it into the 4th
one, because of the duplicated IDs.

Anyhow,

Signed-off-by: Arkadiusz Hiler

[Zebediah Figura (she/her)]

On 4/26/21 5:37 AM, Rémi Bernon wrote:
> From: Arkadiusz Hiler
> Signed-off-by: Rémi Bernon
>
> In PATCH 1, the return GetLastError() look wrong to me but the same is
> done in IoGetDeviceProperty so I kept them. I guess we would need to
> convert the setupapi error back to NTSTATUS, or implement the higher
> level setupapi on top of these lower level primitives intead.

It's maybe not exactly ideal, but it's also not clear how to map 
setupapi errors to NTSTATUS, and moreover it probably doesn't matter 
anyway (we don't expect those functions to fail).

I don't think we can implement setupapi functions on top of ntoskrnl 
ones; that crosses the kernel boundary (and there's no clear syscall 
interface for it). Probably on Windows the code is just duplicated.

FWIW, we have a PnP test driver now, so it's actually possible to write 
tests for this function.
> In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
> because I don't know what the call is actually expecting. Maybe it was
> unnecessary.

I think it's correct as-is to move it—as soon as we call 
IoInvalidateDeviceRelations() the device can be exposed.

Actually, to be completely correct (i.e. if we were running this driver 
in Windows), I think we should treat it as possible to receive 
IRP_MN_QUERY_DEVICE_RELATIONS at any time. What we probably should be 
doing is to initialize everything necessary *before* the statement 
"fdo_ext->u.fdo.child_pdo = child_pdo", and then everything else at 
IRP_MN_START_DEVICE (on the child PDO).
> dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  1 +
>   dlls/ntoskrnl.exe/pnp.c             | 51 +++++++++++++++++++++++++++++
>   include/ddk/wdm.h                   |  4 +++
>   3 files changed, 56 insertions(+)
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 2e5e2e6e11b..4eb08faec2e 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -465,6 +465,7 @@
>   @ stdcall IoReuseIrp(ptr long)
>   @ stub IoSetCompletionRoutineEx
>   @ stdcall IoSetDeviceInterfaceState(ptr long)
> +@ stdcall IoSetDevicePropertyData(ptr ptr long long long long ptr)
>   @ stub IoSetDeviceToVerify
>   @ stub IoSetFileOrigin
>   @ stub IoSetHardErrorOrVerifyDevice
> diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c
> index 095344b1073..e425712d738 100644
> --- a/dlls/ntoskrnl.exe/pnp.c
> +++ b/dlls/ntoskrnl.exe/pnp.c
> @@ -38,6 +38,12 @@ DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> +static inline const char *debugstr_propkey( const DEVPROPKEY *id )
> +{
> - if (!id) return "(null)";
> - return wine_dbg_sprintf( "{%s,%04x}", wine_dbgstr_guid( &id->fmtid ), id->pid );
>
> +}
> -
>
> #define MAX_SERVICE_NAME 260
>
> struct device_interface
>
> @@ -783,6 +789,51 @@ NTSTATUS WINAPI IoSetDeviceInterfaceState( UNICODE_STRING *name, BOOLEAN enable
>       return ret;
>   }
>
> +/***********************************************************************
> -
> -
>       IoSetDevicePropertyData (NTOSKRNL.EXE.@)
> - */
>
> +NTSTATUS WINAPI IoSetDevicePropertyData( DEVICE_OBJECT *device, const DEVPROPKEY *property_key, LCID lcid,
> -
>                                     ULONG flags, DEVPROPTYPE type, ULONG size, void *data )
>
> +{
> - SP_DEVINFO_DATA sp_device = {sizeof(sp_device)};
> - WCHAR device_instance_id[MAX_DEVICE_ID_LEN];
> - NTSTATUS status;
> - HDEVINFO set;
> -
> - TRACE( "device %p, property_key %s, lcid %#x, flags %#x, type %#x, size %u, data %p.\n",
> -
>       device, debugstr_propkey(property_key), lcid, flags, type, size, data );
> -
> - /* flags is always treated as PLUGPLAY_PROPERTY_PERSISTENT starting with Win 8 / 2012 */
> -
> - if (lcid != LOCALE_NEUTRAL) FIXME( "only LOCALE_NEUTRAL is supported\n" );
> -
> - if ((status = get_device_instance_id( device, device_instance_id ))) return status;
> -
> - if ((set = SetupDiCreateDeviceInfoList( &GUID_NULL, NULL )) == INVALID_HANDLE_VALUE)
> - {
> -
>    ERR( "Failed to create device list, error %#x.\n", GetLastError() );
> -
>    return GetLastError();
> - }
> -
> - if (!SetupDiOpenDeviceInfoW( set, device_instance_id, NULL, 0, &sp_device ))
> - {
> -
>    ERR( "Failed to open device, error %#x.\n", GetLastError() );
> -
>    SetupDiDestroyDeviceInfoList( set );
> -
>    return GetLastError();
> - }
> -
> - if (!SetupDiSetDevicePropertyW( set, &sp_device, property_key, type, data, size, 0 ))
> - {
> -
>    ERR( "Failed to set property, error %#x.\n", GetLastError() );
> -
>    SetupDiDestroyDeviceInfoList( set );
> -
>    return GetLastError();
> - }
> -
> - SetupDiDestroyDeviceInfoList( set );
> -
> - return STATUS_SUCCESS;
>
> +}
> - /***********************************************************************
> -
>       IoRegisterDeviceInterface (NTOSKRNL.EXE.@)
> */
>
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 51097bfe3ab..447833c4bc9 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -21,6 +21,7 @@
>   #define _NTDDK_
>
> #include <ntstatus.h>
> +#include <devpropdef.h>
>
> #ifdef _WIN64
>   #define POINTER_ALIGNMENT DECLSPEC_ALIGN(8)
> @@ -1681,6 +1682,8 @@ void      WINAPI ExReleaseResourceForThreadLite(ERESOURCE*,ERESOURCE_THREAD);
>   ULONG     WINAPI ExSetTimerResolution(ULONG,BOOLEAN);
>   void      WINAPI ExUnregisterCallback(void*);
>
> +#define PLUGPLAY_PROPERTY_PERSISTENT 0x0001
> - void      WINAPI IoAcquireCancelSpinLock(KIRQL*);
> NTSTATUS  WINAPI IoAcquireRemoveLockEx(IO_REMOVE_LOCK*,void*,const char*,ULONG, ULONG);
> NTSTATUS  WINAPI IoAllocateDriverObjectExtension(PDRIVER_OBJECT,PVOID,ULONG,PVOID*);
>
> @@ -1728,6 +1731,7 @@ void      WINAPI IoReleaseRemoveLockAndWaitEx(IO_REMOVE_LOCK*,void*,ULONG);
>   void      WINAPI IoReleaseRemoveLockEx(IO_REMOVE_LOCK*,void*,ULONG);
>   void      WINAPI IoReuseIrp(IRP*,NTSTATUS);
>   NTSTATUS  WINAPI IoSetDeviceInterfaceState(UNICODE_STRING*,BOOLEAN);
> +NTSTATUS  WINAPI IoSetDevicePropertyData(DEVICE_OBJECT*,const DEVPROPKEY*,LCID,ULONG,DEVPROPTYPE,ULONG,void*);
>   NTSTATUS  WINAPI IoWMIRegistrationControl(PDEVICE_OBJECT,ULONG);
>
> void    FASTCALL KeAcquireInStackQueuedSpinLockAtDpcLevel(KSPIN_LOCK*,KLOCK_QUEUE_HANDLE*);

[Rémi Bernon]

On 4/26/21 6:28 PM, Zebediah Figura (she/her) wrote:
> On 4/26/21 5:37 AM, Rémi Bernon wrote:
> > From: Arkadiusz Hiler
> > Signed-off-by: Rémi Bernon
> >
> > In PATCH 1, the return GetLastError() look wrong to me but the same is
> > done in IoGetDeviceProperty so I kept them. I guess we would need to
> > convert the setupapi error back to NTSTATUS, or implement the higher
> > level setupapi on top of these lower level primitives intead.
>
> It's maybe not exactly ideal, but it's also not clear how to map 
> setupapi errors to NTSTATUS, and moreover it probably doesn't matter 
> anyway (we don't expect those functions to fail).
>
> I don't think we can implement setupapi functions on top of ntoskrnl 
> ones; that crosses the kernel boundary (and there's no clear syscall 
> interface for it). Probably on Windows the code is just duplicated.
>
> FWIW, we have a PnP test driver now, so it's actually possible to write 
> tests for this function.
> > In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
> > because I don't know what the call is actually expecting. Maybe it was
> > unnecessary.
>
> I think it's correct as-is to move it—as soon as we call 
> IoInvalidateDeviceRelations() the device can be exposed.
>
> Actually, to be completely correct (i.e. if we were running this driver 
> in Windows), I think we should treat it as possible to receive 
> IRP_MN_QUERY_DEVICE_RELATIONS at any time. What we probably should be 
> doing is to initialize everything necessary *before* the statement 
> "fdo_ext->u.fdo.child_pdo = child_pdo", and then everything else at 
> IRP_MN_START_DEVICE (on the child PDO).

It's actually causing IoSetDevicePropertyData to fail the first time a 
device is detected (so on prefix creation for both mouse and keyboard 
devices), as setupapi doesn't know about it until we call 
IoInvalidateDeviceRelations.

Note however, although it fails to set the property, as 
SetupDiDestroyDeviceInfoList clears the last error, it still returns 
success, and the device initialization continues.

I think it may be better to move it after, as it's less likely that 
anything happens in between, while missing the property is going to make 
rawinput device enumeration skip the device. But it's just a workaround.

[Zebediah Figura (she/her)]

On 5/4/21 12:37 PM, Rémi Bernon wrote:
> On 4/26/21 6:28 PM, Zebediah Figura (she/her) wrote:
> > On 4/26/21 5:37 AM, Rémi Bernon wrote:
> > > From: Arkadiusz Hiler
> > > Signed-off-by: Rémi Bernon
> > >
> > > In PATCH 1, the return GetLastError() look wrong to me but the same is
> > > done in IoGetDeviceProperty so I kept them. I guess we would need to
> > > convert the setupapi error back to NTSTATUS, or implement the higher
> > > level setupapi on top of these lower level primitives intead.
> >
> > It's maybe not exactly ideal, but it's also not clear how to map
> > setupapi errors to NTSTATUS, and moreover it probably doesn't matter
> > anyway (we don't expect those functions to fail).
> >
> > I don't think we can implement setupapi functions on top of ntoskrnl
> > ones; that crosses the kernel boundary (and there's no clear syscall
> > interface for it). Probably on Windows the code is just duplicated.
> >
> > FWIW, we have a PnP test driver now, so it's actually possible to write
> > tests for this function.
> > > In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
> > > because I don't know what the call is actually expecting. Maybe it was
> > > unnecessary.
> >
> > I think it's correct as-is to move it—as soon as we call
> > IoInvalidateDeviceRelations() the device can be exposed.
> >
> > Actually, to be completely correct (i.e. if we were running this driver
> > in Windows), I think we should treat it as possible to receive
> > IRP_MN_QUERY_DEVICE_RELATIONS at any time. What we probably should be
> > doing is to initialize everything necessary *before* the statement
> > "fdo_ext->u.fdo.child_pdo = child_pdo", and then everything else at
> > IRP_MN_START_DEVICE (on the child PDO).
>
> It's actually causing IoSetDevicePropertyData to fail the first time a
> device is detected (so on prefix creation for both mouse and keyboard
> devices), as setupapi doesn't know about it until we call
> IoInvalidateDeviceRelations.
>
> Note however, although it fails to set the property, as
> SetupDiDestroyDeviceInfoList clears the last error, it still returns
> success, and the device initialization continues.
>
> I think it may be better to move it after, as it's less likely that
> anything happens in between, while missing the property is going to make
> rawinput device enumeration skip the device. But it's just a workaround.

Sorry, can you please clarify—what's causing the initial call to fail?

[Rémi Bernon]

On 5/4/21 9:11 PM, Zebediah Figura (she/her) wrote:
> On 5/4/21 12:37 PM, Rémi Bernon wrote:
> > On 4/26/21 6:28 PM, Zebediah Figura (she/her) wrote:
> > > On 4/26/21 5:37 AM, Rémi Bernon wrote:
> > > > From: Arkadiusz Hiler
> > > > Signed-off-by: Rémi Bernon
> > > >
> > > > In PATCH 1, the return GetLastError() look wrong to me but the same is
> > > > done in IoGetDeviceProperty so I kept them. I guess we would need to
> > > > convert the setupapi error back to NTSTATUS, or implement the higher
> > > > level setupapi on top of these lower level primitives intead.
> > >
> > > It's maybe not exactly ideal, but it's also not clear how to map
> > > setupapi errors to NTSTATUS, and moreover it probably doesn't matter
> > > anyway (we don't expect those functions to fail).
> > >
> > > I don't think we can implement setupapi functions on top of ntoskrnl
> > > ones; that crosses the kernel boundary (and there's no clear syscall
> > > interface for it). Probably on Windows the code is just duplicated.
> > >
> > > FWIW, we have a PnP test driver now, so it's actually possible to write
> > > tests for this function.
> > > > In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
> > > > because I don't know what the call is actually expecting. Maybe it was
> > > > unnecessary.
> > >
> > > I think it's correct as-is to move it—as soon as we call
> > > IoInvalidateDeviceRelations() the device can be exposed.
> > >
> > > Actually, to be completely correct (i.e. if we were running this driver
> > > in Windows), I think we should treat it as possible to receive
> > > IRP_MN_QUERY_DEVICE_RELATIONS at any time. What we probably should be
> > > doing is to initialize everything necessary *before* the statement
> > > "fdo_ext->u.fdo.child_pdo = child_pdo", and then everything else at
> > > IRP_MN_START_DEVICE (on the child PDO).
> >
> > It's actually causing IoSetDevicePropertyData to fail the first time a
> > device is detected (so on prefix creation for both mouse and keyboard
> > devices), as setupapi doesn't know about it until we call
> > IoInvalidateDeviceRelations.
> >
> > Note however, although it fails to set the property, as
> > SetupDiDestroyDeviceInfoList clears the last error, it still returns
> > success, and the device initialization continues.
> >
> > I think it may be better to move it after, as it's less likely that
> > anything happens in between, while missing the property is going to make
> > rawinput device enumeration skip the device. But it's just a workaround.
>
> Sorry, can you please clarify—what's causing the initial call to fail?

IoSetDevicePropertyData fails, because SetupDiOpenDeviceInfoW fails, 
because, I think, the registry keys haven't been created yet (it returns 
ERROR_NO_SUCH_DEVINST).

My understanding (although I didn't investigate much) is that 
IoInvalidateDeviceRelations creates the registry keys if the device has 
not been seen before.

The symptoms are these messages being printed when a device is first 
seen (so on initial prefix creation for mouse and keyboard devices):

err:plugplay:IoSetDevicePropertyData Failed to open device, error 
0xe000020b.

[Arkadiusz Hiler]

On Mon, Apr 26, 2021 at 12:37:59PM +0200, Rémi Bernon wrote:
> From: Arkadiusz Hiler
>
> Signed-off-by: Rémi Bernon

Signed-off-by: Arkadiusz Hiler