[PATCH 1/2] setupapi: Demote ERR on opening driver key to a TRACE.
open_driver_key() is called by create_driver_key() and SetupDiOpenDevRegKey() which depend on the failure path to detect the presence of the key. This causes a lot of unnecessary errors being printed out. Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/setupapi/devinst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index 0758fef5603..dd1ddaa9d33 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -560,7 +560,7 @@ static LONG open_driver_key(struct device *device, REGSAM access, HKEY *key) RegCloseKey(class_key); return l; } - ERR("Failed to open driver key, error %u.\n", l); + TRACE("Failed to open driver key, error %u.\n", l); } RegCloseKey(class_key); -- 2.31.1
SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER registry property set. It uses another input method as a fallback instead. Since SDL_Joystick GUIDs are dependant on the input method this can mess with mappings and controller type detection. The change fixes Hades wrongly displaying Xbox prompts with Sony controllers over winebus.sys/bus_udev.c. Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/hidclass.sys/Makefile.in | 2 +- dlls/hidclass.sys/device.c | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/hidclass.sys/Makefile.in b/dlls/hidclass.sys/Makefile.in index be4af747853..ffc49e65257 100644 --- a/dlls/hidclass.sys/Makefile.in +++ b/dlls/hidclass.sys/Makefile.in @@ -1,7 +1,7 @@ MODULE = hidclass.sys IMPORTLIB = hidclass IMPORTS = hal ntoskrnl -DELAYIMPORTS = setupapi hid +DELAYIMPORTS = setupapi hid kernelbase EXTRADLLFLAGS = -mno-cygwin diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index fc1dfd07db1..4554181e8bd 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -80,6 +80,7 @@ NTSTATUS HID_LinkDevice(DEVICE_OBJECT *device) HDEVINFO devinfo; GUID hidGuid; BASE_DEVICE_EXTENSION *ext; + HKEY hkey; HidD_GetHidGuid(&hidGuid); ext = device->DeviceExtension; @@ -104,6 +105,14 @@ NTSTATUS HID_LinkDevice(DEVICE_OBJECT *device) FIXME( "failed to register device info %x\n", GetLastError()); goto error; } + + hkey = SetupDiCreateDevRegKeyW(devinfo, &Data, DICS_FLAG_GLOBAL, 0, DIREG_DRV, NULL, NULL); + if (hkey == INVALID_HANDLE_VALUE) + { + FIXME( "failed to create driver reg key %x\n", GetLastError() ); + goto error; + } + RegCloseKey(hkey); } else if (GetLastError() != ERROR_DEVINST_ALREADY_EXISTS) { -- 2.31.1
On 4/13/21 10:31 AM, Arkadiusz Hiler wrote:
SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER registry property set. It uses another input method as a fallback instead.
Since SDL_Joystick GUIDs are dependant on the input method this can mess with mappings and controller type detection.
The change fixes Hades wrongly displaying Xbox prompts with Sony controllers over winebus.sys/bus_udev.c.
It seems that Windows actually installs a null function driver (i.e. one with the SPSVCINST_ASSOCSERVICE flag set) for HID devices, so setupapi will create the key automatically in this case. I'm not sure how that interacts with drivers that also want to layer onto the HID device; probably it means our SetupDiSelectBestCompatDrv() implementation needs to be less stubby.
Signed-off-by: Arkadiusz Hiler <ahiler(a)codeweavers.com> --- dlls/hidclass.sys/Makefile.in | 2 +- dlls/hidclass.sys/device.c | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/Makefile.in b/dlls/hidclass.sys/Makefile.in index be4af747853..ffc49e65257 100644 --- a/dlls/hidclass.sys/Makefile.in +++ b/dlls/hidclass.sys/Makefile.in @@ -1,7 +1,7 @@ MODULE = hidclass.sys IMPORTLIB = hidclass IMPORTS = hal ntoskrnl -DELAYIMPORTS = setupapi hid +DELAYIMPORTS = setupapi hid kernelbase
EXTRADLLFLAGS = -mno-cygwin
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index fc1dfd07db1..4554181e8bd 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -80,6 +80,7 @@ NTSTATUS HID_LinkDevice(DEVICE_OBJECT *device) HDEVINFO devinfo; GUID hidGuid; BASE_DEVICE_EXTENSION *ext; + HKEY hkey;
HidD_GetHidGuid(&hidGuid); ext = device->DeviceExtension; @@ -104,6 +105,14 @@ NTSTATUS HID_LinkDevice(DEVICE_OBJECT *device) FIXME( "failed to register device info %x\n", GetLastError()); goto error; } + + hkey = SetupDiCreateDevRegKeyW(devinfo, &Data, DICS_FLAG_GLOBAL, 0, DIREG_DRV, NULL, NULL); + if (hkey == INVALID_HANDLE_VALUE) + { + FIXME( "failed to create driver reg key %x\n", GetLastError() ); + goto error; + } + RegCloseKey(hkey); } else if (GetLastError() != ERROR_DEVINST_ALREADY_EXISTS) {
On Tue, Apr 13, 2021 at 10:54:12AM -0500, Zebediah Figura (she/her) wrote:
On 4/13/21 10:31 AM, Arkadiusz Hiler wrote:
SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER registry property set. It uses another input method as a fallback instead.
Since SDL_Joystick GUIDs are dependant on the input method this can mess with mappings and controller type detection.
The change fixes Hades wrongly displaying Xbox prompts with Sony controllers over winebus.sys/bus_udev.c.
It seems that Windows actually installs a null function driver (i.e. one with the SPSVCINST_ASSOCSERVICE flag set) for HID devices, so setupapi will create the key automatically in this case.
I was just following what we already do for the display drivers (winex11.drv, winemac.drv). So we would need to add an .inf with an appropriate AddService directive and install it instead of just adding the driver property pointing to an empty driver key?
I'm not sure how that interacts with drivers that also want to layer onto the HID device; probably it means our SetupDiSelectBestCompatDrv() implementation needs to be less stubby.
I don't understand this comment. I tried to follow the function's usage + see how we end up creating a HID device out of the underlaying one but the call chains are long and it gets confusing quickly. Looks like SetupDiSelectBestCompatDrv() is used by SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV) and that in turn is used by ntoskrnl.exe with each function when adding a new PNP device. It seems that whenever a specific driver (one of the implemenations in winebus.sys) detects a new hardware it creates a device on it's own bus (e.g. HIDRAW for udev) and then calls IoInvalidateDeviceRelations() IoInvalidateDeviceRelations() -> handle_bus_relations() -> enumerate_new_device() -> install_device_driver() -> SetupDiCallClassInstaller(DIF_REGISTER_COINSTLLERS) -> SetupDiRegisterCoDeviceInstllers() -> create_driver_key(). Which installs the winehid.ini for the device on the underlaying HIDRAW bus. Then enumerate_new_device() ends up calling start_device() which creates HID device by invoking hidclass.sys/pnp.c:PNP_AddDevice(). Right now hidclass.sys (just like the display drivers) seems to be in charge of its own SetupAPI entries. How exactly does SetupDiSelectBestCompatDrv() tie into that? -- Cheers, Arek
On 4/13/21 2:03 PM, Arkadiusz Hiler wrote:
On Tue, Apr 13, 2021 at 10:54:12AM -0500, Zebediah Figura (she/her) wrote:
On 4/13/21 10:31 AM, Arkadiusz Hiler wrote:
SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER registry property set. It uses another input method as a fallback instead.
Since SDL_Joystick GUIDs are dependant on the input method this can mess with mappings and controller type detection.
The change fixes Hades wrongly displaying Xbox prompts with Sony controllers over winebus.sys/bus_udev.c.
It seems that Windows actually installs a null function driver (i.e. one with the SPSVCINST_ASSOCSERVICE flag set) for HID devices, so setupapi will create the key automatically in this case.
I was just following what we already do for the display drivers (winex11.drv, winemac.drv).
So we would need to add an .inf with an appropriate AddService directive and install it instead of just adding the driver property pointing to an empty driver key?
Right. On Windows this is c:\windows\input.inf.
I'm not sure how that interacts with drivers that also want to layer onto the HID device; probably it means our SetupDiSelectBestCompatDrv() implementation needs to be less stubby.
I don't understand this comment. I tried to follow the function's usage + see how we end up creating a HID device out of the underlaying one but the call chains are long and it gets confusing quickly.
Looks like SetupDiSelectBestCompatDrv() is used by SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV) and that in turn is used by ntoskrnl.exe with each function when adding a new PNP device.
It seems that whenever a specific driver (one of the implemenations in winebus.sys) detects a new hardware it creates a device on it's own bus (e.g. HIDRAW for udev) and then calls IoInvalidateDeviceRelations()
IoInvalidateDeviceRelations() -> handle_bus_relations() -> enumerate_new_device() -> install_device_driver() -> SetupDiCallClassInstaller(DIF_REGISTER_COINSTLLERS) -> SetupDiRegisterCoDeviceInstllers() -> create_driver_key().
Which installs the winehid.ini for the device on the underlaying HIDRAW bus.
Then enumerate_new_device() ends up calling start_device() which creates HID device by invoking hidclass.sys/pnp.c:PNP_AddDevice().
Right now hidclass.sys (just like the display drivers) seems to be in charge of its own SetupAPI entries.
How exactly does SetupDiSelectBestCompatDrv() tie into that?
It is long and complicated ;-) The basic point of .inf files is to detect a layered driver for a device. input.inf contains an section that says "the layered driver for this device is no driver at all", which isn't the same as a driver not being present. But it's possible for a third-party driver to layer on top of HID (or even a first-party driver, such as mouhid.sys), in which case that driver *should* take precedence. SetupDiSelectBestCompatDrv() is supposed to pick the "best" driver. According to [1] it should try to match hardware IDs in order, so that a driver targeting a specific hardware ID "HID\VID_1234&PID_5678&MI_00" is selected instead of a driver targeting a less specific "HID_DEVICE" ID. DIF_SELECTBESTCOMPATDRV is done by UpdateDriverForPlugAndPlayDevices(), or the ntoskrnl equivalent install_device_driver(). Basically the order goes: 1. Call SetupDiBuildDriverInfoList(), which scans all installed .inf files for matching drivers and stores them in a list. 2. Call SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV), which picks the best driver. In Wine this currently picks the first driver, since we don't really expect there to be more than one. We should probably print an explicit FIXME instead of a WARN if there are. 3. Call SetupDiCallClassInstaller with other DIF_* values, these populate various registry keys and copy files, etc. Honestly, I think with the aforementioned FIXME, it's probably not necessary to improve SetupDiSelectBestCompatDrv() yet. On the other hand, it probably wouldn't be that hard. [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifier...
On Tue, Apr 13, 2021 at 02:29:48PM -0500, Zebediah Figura (she/her) wrote:
On 4/13/21 2:03 PM, Arkadiusz Hiler wrote:
On Tue, Apr 13, 2021 at 10:54:12AM -0500, Zebediah Figura (she/her) wrote:
On 4/13/21 10:31 AM, Arkadiusz Hiler wrote:
SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER registry property set. It uses another input method as a fallback instead.
Since SDL_Joystick GUIDs are dependant on the input method this can mess with mappings and controller type detection.
The change fixes Hades wrongly displaying Xbox prompts with Sony controllers over winebus.sys/bus_udev.c.
It seems that Windows actually installs a null function driver (i.e. one with the SPSVCINST_ASSOCSERVICE flag set) for HID devices, so setupapi will create the key automatically in this case.
I was just following what we already do for the display drivers (winex11.drv, winemac.drv).
So we would need to add an .inf with an appropriate AddService directive and install it instead of just adding the driver property pointing to an empty driver key?
Right. On Windows this is c:\windows\input.inf.
I'm not sure how that interacts with drivers that also want to layer onto the HID device; probably it means our SetupDiSelectBestCompatDrv() implementation needs to be less stubby.
I don't understand this comment. I tried to follow the function's usage + see how we end up creating a HID device out of the underlaying one but the call chains are long and it gets confusing quickly.
Looks like SetupDiSelectBestCompatDrv() is used by SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV) and that in turn is used by ntoskrnl.exe with each function when adding a new PNP device.
It seems that whenever a specific driver (one of the implemenations in winebus.sys) detects a new hardware it creates a device on it's own bus (e.g. HIDRAW for udev) and then calls IoInvalidateDeviceRelations()
IoInvalidateDeviceRelations() -> handle_bus_relations() -> enumerate_new_device() -> install_device_driver() -> SetupDiCallClassInstaller(DIF_REGISTER_COINSTLLERS) -> SetupDiRegisterCoDeviceInstllers() -> create_driver_key().
Which installs the winehid.ini for the device on the underlaying HIDRAW bus.
Then enumerate_new_device() ends up calling start_device() which creates HID device by invoking hidclass.sys/pnp.c:PNP_AddDevice().
Right now hidclass.sys (just like the display drivers) seems to be in charge of its own SetupAPI entries.
How exactly does SetupDiSelectBestCompatDrv() tie into that?
It is long and complicated ;-)
The basic point of .inf files is to detect a layered driver for a device. input.inf contains an section that says "the layered driver for this device is no driver at all", which isn't the same as a driver not being present. But it's possible for a third-party driver to layer on top of HID (or even a first-party driver, such as mouhid.sys), in which case that driver *should* take precedence.
SetupDiSelectBestCompatDrv() is supposed to pick the "best" driver. According to [1] it should try to match hardware IDs in order, so that a driver targeting a specific hardware ID "HID\VID_1234&PID_5678&MI_00" is selected instead of a driver targeting a less specific "HID_DEVICE" ID.
DIF_SELECTBESTCOMPATDRV is done by UpdateDriverForPlugAndPlayDevices(), or the ntoskrnl equivalent install_device_driver(). Basically the order goes:
1. Call SetupDiBuildDriverInfoList(), which scans all installed .inf files for matching drivers and stores them in a list. 2. Call SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV), which picks the best driver. In Wine this currently picks the first driver, since we don't really expect there to be more than one. We should probably print an explicit FIXME instead of a WARN if there are. 3. Call SetupDiCallClassInstaller with other DIF_* values, these populate various registry keys and copy files, etc.
Honestly, I think with the aforementioned FIXME, it's probably not necessary to improve SetupDiSelectBestCompatDrv() yet. On the other hand, it probably wouldn't be that hard.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifier...
Windows driver world is completely new to me but I thing I start to understand the control flow thanks to your explanations and reading through MSDN. AFAIU: 1. winebus.sys does: bus_create_hid_device("HIDRAW") which adds it to the pnp devset followed by IoInvalidateDeviceRelations(bus_pdo); 2. IoInvalidateDeviceRelations(bus_pdo) does IRP_MN_QUERY_DEVICE_RELATIONS/BusRelations on the winebus.sys through the bus_pdo which gets all the devices from the pnp devset. Then it checks what's a new child and does enumerate_new_device(child). Then it checks for what was removed and calls remove_device(child). 3. enumerate_new_device(child) does install_device_driver() that you have explained above (thanks again!), which picks up winehid.inf, as it has the entry for HIDRAW. 4. it then calls start_device() which does AddDevice() on the function driver selected using SPDRP_SERVICE, which is winehid as set by winehid.inf. 5. winehid is registered as HID Minidriver with hidclass.sys, which overrides AddDevice with PNP_AddDevice, so we end up calling it. Now, I think we want to introduce hidclass.inf that would be installed for HID devices. The questions is what is the correct way to do this. I see two options: 1. Turn hidclass.sys into something similar to winebus.sys - it would then call IoInvalidateDeviceRelations() and install the NULL driver. 2. Keep it as is but call SetupDiCreateDevRegKeyW() with InfHandle and InfSectionName (I am not 100% sure it does what we want it to do). I assume it's #1, but there may be a third, correct option :-P -- Cheers, Arek
On 4/14/21 8:59 AM, Arkadiusz Hiler wrote:
On Tue, Apr 13, 2021 at 02:29:48PM -0500, Zebediah Figura (she/her) wrote:
On 4/13/21 2:03 PM, Arkadiusz Hiler wrote:
On Tue, Apr 13, 2021 at 10:54:12AM -0500, Zebediah Figura (she/her) wrote:
On 4/13/21 10:31 AM, Arkadiusz Hiler wrote:
SDL doesn't use HID devices directly if they don't have SPDRP_DRIVER registry property set. It uses another input method as a fallback instead.
Since SDL_Joystick GUIDs are dependant on the input method this can mess with mappings and controller type detection.
The change fixes Hades wrongly displaying Xbox prompts with Sony controllers over winebus.sys/bus_udev.c.
It seems that Windows actually installs a null function driver (i.e. one with the SPSVCINST_ASSOCSERVICE flag set) for HID devices, so setupapi will create the key automatically in this case.
I was just following what we already do for the display drivers (winex11.drv, winemac.drv).
So we would need to add an .inf with an appropriate AddService directive and install it instead of just adding the driver property pointing to an empty driver key?
Right. On Windows this is c:\windows\input.inf.
I'm not sure how that interacts with drivers that also want to layer onto the HID device; probably it means our SetupDiSelectBestCompatDrv() implementation needs to be less stubby.
I don't understand this comment. I tried to follow the function's usage + see how we end up creating a HID device out of the underlaying one but the call chains are long and it gets confusing quickly.
Looks like SetupDiSelectBestCompatDrv() is used by SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV) and that in turn is used by ntoskrnl.exe with each function when adding a new PNP device.
It seems that whenever a specific driver (one of the implemenations in winebus.sys) detects a new hardware it creates a device on it's own bus (e.g. HIDRAW for udev) and then calls IoInvalidateDeviceRelations()
IoInvalidateDeviceRelations() -> handle_bus_relations() -> enumerate_new_device() -> install_device_driver() -> SetupDiCallClassInstaller(DIF_REGISTER_COINSTLLERS) -> SetupDiRegisterCoDeviceInstllers() -> create_driver_key().
Which installs the winehid.ini for the device on the underlaying HIDRAW bus.
Then enumerate_new_device() ends up calling start_device() which creates HID device by invoking hidclass.sys/pnp.c:PNP_AddDevice().
Right now hidclass.sys (just like the display drivers) seems to be in charge of its own SetupAPI entries.
How exactly does SetupDiSelectBestCompatDrv() tie into that?
It is long and complicated ;-)
The basic point of .inf files is to detect a layered driver for a device. input.inf contains an section that says "the layered driver for this device is no driver at all", which isn't the same as a driver not being present. But it's possible for a third-party driver to layer on top of HID (or even a first-party driver, such as mouhid.sys), in which case that driver *should* take precedence.
SetupDiSelectBestCompatDrv() is supposed to pick the "best" driver. According to [1] it should try to match hardware IDs in order, so that a driver targeting a specific hardware ID "HID\VID_1234&PID_5678&MI_00" is selected instead of a driver targeting a less specific "HID_DEVICE" ID.
DIF_SELECTBESTCOMPATDRV is done by UpdateDriverForPlugAndPlayDevices(), or the ntoskrnl equivalent install_device_driver(). Basically the order goes:
1. Call SetupDiBuildDriverInfoList(), which scans all installed .inf files for matching drivers and stores them in a list. 2. Call SetupDiCallClassInstaller(DIF_SELECTBESTCOMPATDRV), which picks the best driver. In Wine this currently picks the first driver, since we don't really expect there to be more than one. We should probably print an explicit FIXME instead of a WARN if there are. 3. Call SetupDiCallClassInstaller with other DIF_* values, these populate various registry keys and copy files, etc.
Honestly, I think with the aforementioned FIXME, it's probably not necessary to improve SetupDiSelectBestCompatDrv() yet. On the other hand, it probably wouldn't be that hard.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifier...
Windows driver world is completely new to me but I thing I start to understand the control flow thanks to your explanations and reading through MSDN.
AFAIU:
1. winebus.sys does: bus_create_hid_device("HIDRAW") which adds it to the pnp devset followed by IoInvalidateDeviceRelations(bus_pdo);
2. IoInvalidateDeviceRelations(bus_pdo) does IRP_MN_QUERY_DEVICE_RELATIONS/BusRelations on the winebus.sys through the bus_pdo which gets all the devices from the pnp devset. Then it checks what's a new child and does enumerate_new_device(child). Then it checks for what was removed and calls remove_device(child).
3. enumerate_new_device(child) does install_device_driver() that you have explained above (thanks again!), which picks up winehid.inf, as it has the entry for HIDRAW.
4. it then calls start_device() which does AddDevice() on the function driver selected using SPDRP_SERVICE, which is winehid as set by winehid.inf.
5. winehid is registered as HID Minidriver with hidclass.sys, which overrides AddDevice with PNP_AddDevice, so we end up calling it.
Now, I think we want to introduce hidclass.inf that would be installed for HID devices. The questions is what is the correct way to do this. I see two options:
1. Turn hidclass.sys into something similar to winebus.sys - it would then call IoInvalidateDeviceRelations() and install the NULL driver.
2. Keep it as is but call SetupDiCreateDevRegKeyW() with InfHandle and InfSectionName (I am not 100% sure it does what we want it to do).
I assume it's #1, but there may be a third, correct option :-P
The answer is broadly #1, assuming I understand you correctly, and I have patches to do this queued. We should be creating devices the normal PnP way in hidclass, not calling into setupapi directly.
On Wed, Apr 14, 2021 at 09:33:02AM -0500, Zebediah Figura (she/her) wrote:
1. Turn hidclass.sys into something similar to winebus.sys - it would then call IoInvalidateDeviceRelations() and install the NULL driver.
The answer is broadly #1, assuming I understand you correctly, and I have patches to do this queued. We should be creating devices the normal PnP way in hidclass, not calling into setupapi directly.
Yes, I've phrased it poorly. Doing it the normal PnP is what I have meant. I'll wait for those queued patches then :-) -- Cheers, Arek
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=88648 Your paranoid android. === debiant2 (build log) === Task: Could not create the win32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the win32 report === debiant2 (build log) === Task: Could not create the wow32 wineprefix: Failed to disable the crash dialogs: Task: WineTest did not produce the wow32 report
participants (3)
-
Arkadiusz Hiler -
Marvin -
Zebediah Figura (she/her)