[PATCH v3 0/8] 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.
-- v3: ntoskrnl: Set Device ContainerID from driver when registering an interface ntoskrnl: Set device ContainerID from driver ntoskrnl: Improve error handling in get_device_id hidclass: Expose ContainerID hidclass: Copy device ContainerID to child devices hidclass: Copy ContainerID from underlying driver in driver_add_device
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..1282285da4c 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 BaseContainerId[] = {'B','a','s','e','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 }, + [0x24] = { REG_SZ, "BaseContainerId", BaseContainerId }, };
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
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 | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/dlls/hidclass.sys/pnp.c b/dlls/hidclass.sys/pnp.c index 873c55c4a67..afc7b6e933d 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -71,7 +71,7 @@ static minidriver *find_minidriver(DRIVER_OBJECT *driver) static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR *id) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Status = STATUS_NOT_IMPLEMENTED, .Information = 0 }; KEVENT event; IRP *irp;
@@ -87,8 +87,14 @@ static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCH 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.Information && !irp_status.Status) + wcscpy(id, (WCHAR *)irp_status.Information); + else + id[0] = 0; + + if (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 | 1 + 2 files changed, 2 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 afc7b6e933d..a187058a2d4 100644 --- a/dlls/hidclass.sys/pnp.c +++ b/dlls/hidclass.sys/pnp.c @@ -173,6 +173,7 @@ static NTSTATUS WINAPI driver_add_device(DRIVER_OBJECT *driver, DEVICE_OBJECT *b ext->u.fdo.hid_ext.NextDeviceObject = bus_pdo; swprintf(ext->device_id, ARRAY_SIZE(ext->device_id), L"HID\%s", wcsrchr(device_id, '\') + 1); wcscpy(ext->instance_id, instance_id); + get_device_id(bus_pdo, BusQueryContainerID, ext->container_id);
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;
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 a187058a2d4..1c0ba839b9e 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 1c0ba839b9e..903d5e9d3dd 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
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/ntoskrnl.exe/pnp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 71c03586897..69f15d82875 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -75,7 +75,7 @@ static struct wine_rb_tree device_interfaces = { interface_rb_compare }; static NTSTATUS get_device_id( DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id ) { IO_STACK_LOCATION *irpsp; - IO_STATUS_BLOCK irp_status; + IO_STATUS_BLOCK irp_status = { .Information = 0 }; KEVENT event; IRP *irp;
From: Claire Girka claire@sitedethib.com
--- dlls/ntoskrnl.exe/pnp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index 69f15d82875..e12e46f94f1 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -281,6 +281,13 @@ static BOOL install_device_driver( DEVICE_OBJECT *device, HDEVINFO set, SP_DEVIN sizeof_multiszW( ids ) * sizeof(WCHAR) ); ExFreePool( ids );
+ if (!get_device_id(device, BusQueryContainerID, &ids) && ids) + { + SetupDiSetDeviceRegistryPropertyW( set, sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)ids, + (lstrlenW( ids ) + 1) * sizeof(WCHAR) ); + ExFreePool( ids ); + } + if (!SetupDiBuildDriverInfoList( set, sp_device, SPDIT_COMPATDRIVER )) { ERR("Failed to build compatible driver list, error %#lx.\n", GetLastError());
From: Claire Girka claire@sitedethib.com
--- dlls/ntoskrnl.exe/pnp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e12e46f94f1..77d879a07a9 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -880,6 +880,7 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla struct wine_rb_entry *entry; DWORD required; HDEVINFO set; + WCHAR *id;
TRACE("device %p, class_guid %s, refstr %s, symbolic_link %p.\n", device, debugstr_guid(class_guid), debugstr_us(refstr), symbolic_link); @@ -913,6 +914,13 @@ NTSTATUS WINAPI IoRegisterDeviceInterface(DEVICE_OBJECT *device, const GUID *cla return STATUS_UNSUCCESSFUL; }
+ if (!get_device_id(device, BusQueryContainerID, &id) && id) + { + SetupDiSetDeviceRegistryPropertyW( set, &sp_device, SPDRP_BASE_CONTAINERID, (BYTE *)id, + (lstrlenW( id ) + 1) * sizeof(WCHAR) ); + ExFreePool( id ); + } + data->DevicePath[1] = '?'; TRACE("Returning path %s.\n", debugstr_w(data->DevicePath)); RtlCreateUnicodeString( &device_path, data->DevicePath);
I resubmitted the MR a bunch of times, tightening the handling of uninitialized memory around the IRP requests in an attempt to fix the test failures (that I cannot reproduce on my computer), but I now believe the test failures to be unrelated to my changes and instead be caused by [recently-introduced](https://gitlab.winehq.org/wine/wine/-/commit/200d7b25eccc96a740b902aa59c86fe...) flaky tests (thanks to fgouget for pointing that out).
Some of those tightening changes, such as 6bb00498b8a2b14e727455dd9732502436fb3134, are most probably unnecessary, but I will leave them in unless requested otherwise.
Jinoh Kang (@iamahuman) commented about dlls/ntoskrnl.exe/pnp.c:
static NTSTATUS get_device_id( DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id ) { IO_STACK_LOCATION *irpsp;
- IO_STATUS_BLOCK irp_status;
- IO_STATUS_BLOCK irp_status = { .Information = 0 };
This is missing the corresponding NULL check.
On Tue Jul 12 16:28:17 2022 +0000, Jinoh Kang wrote:
This is missing the corresponding NULL check.
Eh, nevermind, perhaps `*id` is permitted to be NULL.
On Tue Jul 12 16:29:10 2022 +0000, Jinoh Kang wrote:
Eh, nevermind, perhaps `*id` is permitted to be NULL.
The existing call sites do check whether it is NULL.
-
Claire (＠ClearlyClaire) -
Claire Girka
-
Jinoh Kang (＠iamahuman)