[PATCH v10 0/7] MR432: Add support for SPDRP_BASE_CONTAINERID by querying drivers with BusQueryContainerID
This is the hidclass/ntoskrnl/setupapi counterpart to !359. As with that merge request, implementing `BusQueryContainerID` in the bus driver is left out and will be addressed in a different MR.
I believe this is useful to expose the Container ID no matter how those end up being assigned by the bus driver.
-- v10: ntoskrnl: Set device ContainerID from driver hidclass: Expose ContainerID hidclass: Copy device ContainerID to child devices hidclass: Copy ContainerID from underlying driver in driver_add_device hidclass: Improve error handling in get_device_id ntoskrnl/tests: Add test for getting SPDRP_BASE_CONTAINERID from PnP driver setupapi: Add support for SPDRP_BASE_CONTAINERID
From: Claire Girka claire@sitedethib.com
--- dlls/setupapi/devinst.c | 2 ++ include/setupapi.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 73e721753aa..901af4650c8 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -96,6 +96,7 @@ static const WCHAR Capabilities[] = {'C','a','p','a','b','i','l','i','t','i','e' static const WCHAR UINumber[] = {'U','I','N','u','m','b','e','r',0}; static const WCHAR UpperFilters[] = {'U','p','p','e','r','F','i','l','t','e','r','s',0}; static const WCHAR LowerFilters[] = {'L','o','w','e','r','F','i','l','t','e','r','s',0}; +static const WCHAR ContainerId[] = {'C','o','n','t','a','i','n','e','r','I','d',0}; static const WCHAR Phantom[] = {'P','h','a','n','t','o','m',0}; static const WCHAR SymbolicLink[] = {'S','y','m','b','o','l','i','c','L','i','n','k',0}; static const WCHAR Control[] = {'C','o','n','t','r','o','l',0}; @@ -654,6 +655,7 @@ static const struct PropertyMapEntry PropertyMap[] = { { REG_DWORD, "UINumber", UINumber }, { REG_MULTI_SZ, "UpperFilters", UpperFilters }, { REG_MULTI_SZ, "LowerFilters", LowerFilters }, + [SPDRP_BASE_CONTAINERID] = { REG_SZ, "ContainerId", ContainerId }, };
static BOOL SETUPDI_SetDeviceRegistryPropertyW(struct device *device, diff --git a/include/setupapi.h b/include/setupapi.h index 14e3774fc42..806df7baaf5 100644 --- a/include/setupapi.h +++ b/include/setupapi.h @@ -1315,7 +1315,8 @@ DECL_WINELIB_SETUPAPI_TYPE_AW(PSP_INF_SIGNER_INFO) #define SPDRP_REMOVAL_POLICY_HW_DEFAULT 0x00000020 #define SPDRP_REMOVAL_POLICY_OVERRIDE 0x00000021 #define SPDRP_INSTALL_STATE 0x00000022 -#define SPDRP_MAXIMUM_PROPERTY 0x00000023 +#define SPDRP_BASE_CONTAINERID 0x00000024 +#define SPDRP_MAXIMUM_PROPERTY 0x00000025
#define DPROMPT_SUCCESS 0 #define DPROMPT_CANCEL 1
From: Claire Girka claire@sitedethib.com
--- dlls/ntoskrnl.exe/tests/driver_pnp.c | 5 ++++- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/dlls/ntoskrnl.exe/tests/driver_pnp.c b/dlls/ntoskrnl.exe/tests/driver_pnp.c index 1f22c4b9997..9a965f584cd 100644 --- a/dlls/ntoskrnl.exe/tests/driver_pnp.c +++ b/dlls/ntoskrnl.exe/tests/driver_pnp.c @@ -228,7 +228,10 @@ static NTSTATUS query_id(struct device *device, IRP *irp, BUS_QUERY_ID_TYPE type }
case BusQueryContainerID: - return STATUS_NOT_SUPPORTED; + if (!(id = ExAllocatePool(PagedPool, 39 * sizeof(WCHAR)))) + return STATUS_NO_MEMORY; + wcscpy(id, L"{12345678-1234-1234-1234-123456789123}"); + break;
default: ok(0, "Unexpected ID query type %#x.\n", type); diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 64debcbebbf..6d1185301eb 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1462,6 +1462,7 @@ static void test_pnp_devices(void) static const char expect_compat_id[] = "winetest_compat\0winetest_compat_1\0";
char buffer[200]; + WCHAR buffer_w[200]; SP_DEVICE_INTERFACE_DETAIL_DATA_A *iface_detail = (void *)buffer; SP_DEVICE_INTERFACE_DATA iface = {sizeof(iface)}; SP_DEVINFO_DATA device = {sizeof(device)}; @@ -1646,6 +1647,11 @@ static void test_pnp_devices(void) ok(size == sizeof(expect_hardware_id), "got size %lu\n", size); ok(!memcmp(buffer, expect_hardware_id, size), "got hardware IDs %s\n", debugstr_an(buffer, size));
+ /* Using the WCHAR variant because Windows returns a WCHAR for this property even when using SetupDiGetDeviceRegistryPropertyA */ + ret = SetupDiGetDeviceRegistryPropertyW(set, &device, SPDRP_BASE_CONTAINERID, + &type, (BYTE *)buffer_w, sizeof(buffer_w), &size); + todo_wine ok(ret, "got error %#lx\n", GetLastError()); + ret = SetupDiGetDeviceRegistryPropertyA(set, &device, SPDRP_COMPATIBLEIDS, &type, (BYTE *)buffer, sizeof(buffer), &size); ok(ret, "got error %#lx\n", GetLastError());
On Sat Jul 16 08:41:48 2022 +0000, Claire wrote:
I am not sure what the test shows, running the test under Wine returns similar results (although the queries appear to be performed twice), with the same IDs being queried, none involving `BusQueryContainerId` (0x5):
driver_pnp.c:136: Test failed: Unexpected ID query of type 0. driver_pnp.c:136: Test failed: Unexpected ID query of type 0x3. driver_pnp.c:136: Test failed: Unexpected ID query of type 0. driver_pnp.c:136: Test failed: Unexpected ID query of type 0x3.To my understanding, the documentation suggests that Windows queries all properties *before* saving those to the registry and installing the device driver, while Wine interleaves querying and storing in `install_device_driver`, and in the case of my patches, queries and stores the container ID after running `install_device_driver`. So if I understand correctly, you're suggesting to move the ContainerID querying back to `install_device_driver`. However, as I explained earlier, I think we want the ContainerID to be re-queried in `enumerate_new_device` even if the driver is already installed (which seems to occur in the case of winebus HID devices across reboots and port changes when a Serial number is exposed), and my current implementation of ContainerID (not submitted so far) relies on this. Maybe the driver already being installed across reboots and ports is a bug, though.
I have decided to move the querying of `BusQueryContainerID` before the (conditional) call to `install_device_driver`. I doubt this changes anything in practice, especially in the case where the driver is already installed, but this is closer to what Windows does and this does not break my use case.
From: Claire Girka claire@sitedethib.com
Do not assume the underlying driver will return meaningful data, as it may not support BusQueryContainerID which will be queried in a next commit. --- dlls/hidclass.sys/pnp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 873c55c4a67..91d80f74403 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -84,11 +84,15 @@ static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCH irpsp->MinorFunction = IRP_MN_QUERY_ID; irpsp->Parameters.QueryId.IdType = type;
+ irp->IoStatus.Status = STATUS_NOT_SUPPORTED; if (IoCallDriver(device, irp) == STATUS_PENDING) KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL);
- wcscpy(id, (WCHAR *)irp_status.Information); - ExFreePool((WCHAR *)irp_status.Information); + if (!irp_status.Status) { + wcscpy(id, (WCHAR *)irp_status.Information); + ExFreePool((WCHAR *)irp_status.Information); + } + return irp_status.Status; }
From: Claire Girka claire@sitedethib.com
--- dlls/hidclass.sys/hid.h | 1 + dlls/hidclass.sys/pnp.c | 3 +++ 2 files changed, 4 insertions(+)
diff --git a/dlls/hidclass.sys/hid.h b/dlls/hidclass.sys/hid.h index e20f12809eb..b8cec55fb7c 100644 --- a/dlls/hidclass.sys/hid.h +++ b/dlls/hidclass.sys/hid.h @@ -81,6 +81,7 @@ typedef struct _BASE_DEVICE_EXTENSION * for convenience. */ WCHAR device_id[MAX_DEVICE_ID_LEN]; WCHAR instance_id[MAX_DEVICE_ID_LEN]; + WCHAR container_id[MAX_GUID_STRING_LEN]; const GUID *class_guid;
BOOL is_fdo; diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 91d80f74403..a2315b686ca 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -172,6 +172,9 @@ static NTSTATUS WINAPI driver_add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *b swprintf(ext->device_id, ARRAY_SIZE(ext->device_id), L"HID\%s", wcsrchr(device_id, '\') + 1); wcscpy(ext->instance_id, instance_id);
+ if (get_device_id(bus_pdo, BusQueryContainerID, ext->container_id)) + ext->container_id[0] = 0; + is_xinput_class = !wcsncmp(device_id, L"WINEXINPUT\", 7) && wcsstr(device_id, L"&XI_") != NULL; if (is_xinput_class) ext->class_guid = &GUID_DEVINTERFACE_WINEXINPUT; else ext->class_guid = &GUID_DEVINTERFACE_HID;
From: Claire Girka claire@sitedethib.com
--- dlls/hidclass.sys/pnp.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index a2315b686ca..271c777129d 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -231,6 +231,7 @@ static void create_child(minidriver *minidriver, DEVICE_OBJECT *fdo) KeInitializeSpinLock( &pdo_ext->u.pdo.queues_lock ); wcscpy(pdo_ext->device_id, fdo_ext->device_id); wcscpy(pdo_ext->instance_id, fdo_ext->instance_id); + wcscpy(pdo_ext->container_id, fdo_ext->container_id); pdo_ext->class_guid = fdo_ext->class_guid;
pdo_ext->u.pdo.information.VendorID = attr.VendorID;
From: Claire Girka claire@sitedethib.com
--- dlls/hidclass.sys/pnp.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 271c777129d..023a4d9fa44 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -419,8 +419,16 @@ static NTSTATUS pdo_pnp(DEVICE_OBJECT *device, IRP *irp) irp->IoStatus.Information = (ULONG_PTR)id; status = STATUS_SUCCESS; break; - case BusQueryContainerID: + if (ext->container_id[0]) { + lstrcpyW(id, ext->container_id); + irp->IoStatus.Information = (ULONG_PTR)id; + status = STATUS_SUCCESS; + } else { + ExFreePool(id); + } + break; + case BusQueryDeviceSerialNumber: FIXME("unimplemented id type %#x\n", irpsp->Parameters.QueryId.IdType); ExFreePool(id);
From: Claire Girka claire@sitedethib.com
Using SetupDiGetDeviceRegistryPropertyW rather than SetupDiGetDeviceRegistryPropertyA in the test, because games use the former, and Windows returns the Container ID as a WCHAR string either way. --- dlls/ntoskrnl.exe/pnp.c | 8 ++++++++ dlls/ntoskrnl.exe/tests/ntoskrnl.c | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 71c03586897..a70190db7ce 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -318,6 +318,7 @@ static void enumerate_new_device( DEVICE_OBJECT *device, HDEVINFO set ) BOOL need_driver = TRUE; NTSTATUS status; HKEY key; + WCHAR *id;
if (get_device_instance_id( device, device_instance_id )) return; @@ -347,6 +348,13 @@ static void enumerate_new_device( DEVICE_OBJECT *device, HDEVINFO set ) return; }
+ if (!get_device_id(device, BusQueryContainerID, &id) && id) + { + SetupDiSetDeviceRegistryPropertyW( set, &sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)id, + (lstrlenW( id ) + 1) * sizeof(WCHAR) ); + ExFreePool( id ); + } + if (need_driver && !install_device_driver( device, set, &sp_device ) && !caps.RawDeviceOK) { ERR("Unable to install a function driver for device %s.\n", debugstr_w(device_instance_id)); diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 6d1185301eb..6d8513ffa35 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1460,6 +1460,7 @@ static void test_pnp_devices(void) { static const char expect_hardware_id[] = "winetest_hardware\0winetest_hardware_1\0"; static const char expect_compat_id[] = "winetest_compat\0winetest_compat_1\0"; + static const WCHAR expect_container_id_w[] = L"{12345678-1234-1234-1234-123456789123}";
char buffer[200]; WCHAR buffer_w[200]; @@ -1650,7 +1651,10 @@ static void test_pnp_devices(void) /* Using the WCHAR variant because Windows returns a WCHAR for this property even when using SetupDiGetDeviceRegistryPropertyA */ ret = SetupDiGetDeviceRegistryPropertyW(set, &device, SPDRP_BASE_CONTAINERID, &type, (BYTE *)buffer_w, sizeof(buffer_w), &size); - todo_wine ok(ret, "got error %#lx\n", GetLastError()); + ok(ret, "got error %#lx\n", GetLastError()); + ok(type == REG_SZ, "got type %lu\n", type); + ok(size == sizeof(expect_container_id_w), "got size %lu\n", size); + ok(!memcmp(buffer_w, expect_container_id_w, size), "got container ID %s\n", debugstr_w(buffer_w));
ret = SetupDiGetDeviceRegistryPropertyA(set, &device, SPDRP_COMPATIBLEIDS, &type, (BYTE *)buffer, sizeof(buffer), &size);
This merge request was approved by Zebediah Figura.
R��mi Bernon (@rbernon) commented about dlls/hidclass.sys/pnp.c:
irpsp->MinorFunction = IRP_MN_QUERY_ID; irpsp->Parameters.QueryId.IdType = type;
- irp->IoStatus.Status = STATUS_NOT_SUPPORTED; if (IoCallDriver(device, irp) == STATUS_PENDING) KeWaitForSingleObject(&event, Executive, KernelMode, FALSE, NULL);
- wcscpy(id, (WCHAR *)irp_status.Information);
- ExFreePool((WCHAR *)irp_status.Information);
- if (!irp_status.Status) {
Please keep the brace wrapping consistent.
R��mi Bernon (@rbernon) commented about dlls/hidclass.sys/pnp.c:
irp->IoStatus.Information = (ULONG_PTR)id; status = STATUS_SUCCESS; break;
case BusQueryContainerID:
if (ext->container_id[0]) {lstrcpyW(id, ext->container_id);irp->IoStatus.Information = (ULONG_PTR)id;status = STATUS_SUCCESS;} else {
Same here.
Please mind adding a trailing period to the commit messages for consistency.
Also, IMHO https://gitlab.winehq.org/wine/wine/-/merge_requests/432/diffs?commit_id=aee... could be squashed with the two previous commits in a single change to expose the ContainerID in hidclass.sys.
-
Claire (＠ClearlyClaire) -
Claire Girka
-
Rémi Bernon (＠rbernon)
-
Zebediah Figura (＠zfigura)