[PATCH 0/1] MR1494: winebus.sys: Don't use SDL_HAPTIC_INFINITY when calling SDL_HapticRunEffect.
From: Ivo Ivanov logos128@gmail.com
Fixes a kernel warning in hid-pidff, about assigning a 32-bit value to an 8-bit field. Possibly thousands of such warnings can be logged for a longer gaming session.
Signed-off-by: Ivo Ivanov logos128@gmail.com --- dlls/winebus.sys/bus_sdl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index 1af029bed65..fa05312cff2 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -598,7 +598,14 @@ static NTSTATUS sdl_device_physical_effect_control(struct unix_device *iface, BY pSDL_HapticStopAll(impl->sdl_haptic); /* fallthrough */ case PID_USAGE_OP_EFFECT_START: - pSDL_HapticRunEffect(impl->sdl_haptic, id, (iterations == 0xff ? SDL_HAPTIC_INFINITY : iterations)); + /* FIXME: Using SDL_HAPTIC_INFINITY here causes an out of bounds kernel + * warning in the hid-pidff driver (in case of a pid device). + * As SDL_HapticRunEffect sets iterations to INT_MAX, and the driver + * passes the value directly to PID_LOOP_COUNT without rescaling, + * the hid core logs a warning about assigning a 32-bit value to + * an 8-bit field. + * May be revised if/when the hid-pidff driver is fixed. */ + pSDL_HapticRunEffect(impl->sdl_haptic, id, iterations); break; case PID_USAGE_OP_EFFECT_STOP: pSDL_HapticStopEffect(impl->sdl_haptic, id);
I understand it may be annoying to have such warnings logged, but then it's a driver defect, and working around it will just silent the warnings and make it less likely they get fixed.
Unless it's indeed illegal to use that value I don't think we want this in Wine. Not upstream at the very least.
I agree.
It's a warning with a call trace though. Probably tens of thousands for a 30 min game play, etc. It's an issue with the SDL backend, so it can be mitigated with using the hidraw backend (since this is a HID PID device). It is more of a problem on Proton, where the SDL backend cannot be disabled. It's been brought to my attention by another Simucube 2 user, who uses the device in SteamPlay (Proton). Since I suggested the SDL_HAPTIC_INFINITY commit back then, thought to improve it to mitigate the issue, until the hid-pidff driver (and/or SDL) gets fixed.
BTW, few weeks ago I submitted a PR to Proton's Wine repository, which enables the hidraw backend for Simucube steering wheels and Fanatec pedals. It would fix this issue as well. If you have some time, you can look at it.
BTW, few weeks ago I submitted a PR to Proton's Wine repository, which enables the hidraw backend for Simucube steering wheels and Fanatec pedals. It would fix this issue as well. If you have some time, you can look at it.
Yes that's probably something we can have in Proton, it looks good and I'll merge it. It will be in next Experimental release.
Fwiw I'm considering having a similar mechanism as Proton does for upstream, with some default backend depending on the device, as we've got more and more devices working best with `hidraw`. Though I still haven't figured how to do that cleanly yet.
I think anything that is joystick would work just well on hidraw. Even compositing devices with the first interface joystick would be OK. So far I'm using all my devices via the hidraw backend, without any issues. This includes pedals, shifter, FFB (HID-PID) wheel, joystick, rudder pedals, etc. - with all their features properly utilized via the controlling software.
On other side FFB devices not compatible with HID-PID specification (which use separate FFB drivers) need to be assigned to the SDL backend. IMO using the registry for whitelisting (or blacklisting) vid, pid pairs to the respective backends is one possible solution. Better if this can be done visually in the control panel.
Proton's implementation is really good with the hidraw prioritized over SDL. Just the hardcoded vid, pid pairs aren't user friendly IMO.
What is a real problem in the bus_udev implementation is the evdev backend, which enumerates mouse and keyboard devices in addition to the other devices. Especially keyboards are added with empty reports, without any usages as they have only keys, while the evdev backend searches for buttons. Then any keypress crashes the winedevice.exe, etc.
IMO the evdev backend should be disabled through DisableInput by default, so when the device is unavailable via hidraw, the next choice to be SDL.
I think anything that is joystick would work just well on hidraw. Even compositing devices with the first interface joystick would be OK. So far I'm using all my devices via the hidraw backend, without any issues. This includes pedals, shifter, FFB (HID-PID) wheel, joystick, rudder pedals, etc. - with all their features properly utilized via the controlling software.
Good to know!
On other side FFB devices not compatible with HID-PID specification (which use separate FFB drivers) need to be assigned to the SDL backend. IMO using the registry for whitelisting (or blacklisting) vid, pid pairs to the respective backends is one possible solution. Better if this can be done visually in the control panel.
Yes, we had issues with some devices exposed by default through `hidraw`, which is why we decided to not make it the default anymore and only selectively enable it for some devices we know are working well.
Some devices are simply not supported correctly (for instance anything that uses multiple top-level collection). And some other are not working well because they have a different HID descriptor than what they usually look like on Windows (where they may not be HID devices at all), and some games sometimes hardcode their expectations (like I believe it is the case with G920 wheels).
What is a real problem in the bus_udev implementation is the evdev backend, which enumerates mouse and keyboard devices in addition to the other devices. Especially keyboards are added with empty reports, without any usages as they have only keys, while the evdev backend searches for buttons. Then any keypress crashes the winedevice.exe, etc.
I don't think evdev should enumerate mouse and keyboard devices at all, that's probably because you've configured your system with your user in the `input` group. It is not usually the distribution default, as it introduces a potential security problem where any process could then read the mouse and keyboard input without any access control from the X server.
But otherwise yes, we currently don't support such devices on the HID level. I think it is the way forward though, to better fix mouse / keyboard input issues, and ultimately I'd like to investigate a possible X11 backend to winebus.sys and support for mouse and keyboard devices, but that's a long term goal.
I don't think evdev should enumerate mouse and keyboard devices at all, that's probably because you've configured your system with your user in the `input` group. It is not usually the distribution default, as it introduces a potential security problem where any process could then read the mouse and keyboard input without any access control from the X server.
Yes, this happens only with a user, which is a member of the 'input' group. I use a limited account, where the input devices aren't enumerated by bus_udev.
Anyway, would be good if the evdev backend is not the first choice if hidraw is unavailable due to lack of permissions, etc.
I'd like to ask you how to proceed with a MR which includes mostly work on winebus.sys, but there is also some initial work on wineusb.sys. It's an implementation of IOCTL_HID_GET_INDEXED_STRING, and the later work depends (at least logically) on the changes of wineusb.sys. Sorry for asking here!
I'd like to ask you how to proceed with a MR which includes mostly work on winebus.sys, but there is also some initial work on wineusb.sys. It's an implementation of IOCTL_HID_GET_INDEXED_STRING, and the later work depends (at least logically) on the changes of wineusb.sys. Sorry for asking here!
I'm not completely sure to see how wineusb.sys could be involved, and I don't know anything about that driver. I can have a look at the changes beforehand if you point me to a branch. Or feel free to open a Draft MR with conflicting changes too, and rebase it later once the required changes have been merged.
IOCTL_HID_GET_INDEXED_STRING needs to be retrieved from the lower USB level, so this patchset along with other things, implements HID USB (Class 03) function driver functionality for winebus.sys - in addition of it being HID bus driver. This allows wineusb.sys to create FDOs in winebus.sys for all its enumerated children. Those FDOs then are easily accessible and addressable by the PDOs of the HID bus driver. If there is an IOCTL_HID_GET_INDEXED_STRING request to some of the PDOs, it searches for a matching FDO from the HID USB function driver, and if there is one sends an IRP to retrieve the needed indexed string from the lower level driver. There is no strong bond between the device objects - if there is a matching FDO then it'll be used to retrieve an indexed string from its device, if not then a not supported error is returned with the upper IRP.
So, the HID USB function driver acts strictly as such - just creates FDOs for the lower USB bus driver. It doesn't create any children PDOs for the FDOs. The FDOs are solely used to retrieve data from the lower level USB devices.
There are some other small fixes and improvements.
I'm using this for the last 5-6 months, without any issues. The Simucube 2 FFB wheel uses the HID bus driver to access the hidraw interface, while the VKBSim Gunfighter joystick uses both the HID bus driver for hidraw access, and HID USB Function driver for USB raw access to retrieve various indexed strings from the device.
Here is the repository with the branch wip/IOCTL_HID_GET_INDEXED_STRING: https://gitlab.winehq.org/logos128/wine/-/tree/wip/IOCTL_HID_GET_INDEXED_STR...
Only the first three patches are related to wineusb.sys, and basically make the instanceIDs unique and easily recognizable for the upper level.
IOCTL_HID_GET_INDEXED_STRING needs to be retrieved from the lower USB level, so this patchset along with other things, implements HID USB (Class 03) function driver functionality for winebus.sys - in addition of it being HID bus driver.
I'm not sure this makes sense. USB access needs to be explicitly allowed on Linux, probably other platforms too. It certainly can't be the default in Wine.
What needs IOCTL_HID_GET_INDEXED_STRING?
Yes, the USB access needs to be allowed in the same way as hidraw access. To my knowledge there are devices that have USB user access allowed by default via UDEV. Depends on the UDEV rules accepted by upstream systemd and distributed by them. I am using it for VKBSim Gunfighter III joystick and its controlling software through Wine.
This merge request was closed by Ivo Ivanov.
-
Ivo Ivanov -
Ivo Ivanov (＠logos128)
-
Rémi Bernon (＠rbernon)
-
Zebediah Figura (＠zfigura)