On Fri, Jun 18, 2021 at 09:06:02AM +0200, Marius Zachmann wrote: > On 18.06.21 at 08:47:37 CEST, Wilken Gottwalt wrote > > On Fri, 18 Jun 2021 08:18:23 +0200 > > Marius Zachmann <mail@xxxxxxxxxxxxxxxxx> wrote: > > > > > On 18.06.21 at 07:45:00 CEST, Wilken Gottwalt wrote > > > > On Fri, 18 Jun 2021 05:56:29 +0600 > > > > Aleksandr Mezin <mezin.alexander@xxxxxxxxx> wrote: > > > > > > > > > I've looked through corsair-psu sources, and I think filtering in > > > > > raw_event won't be enough. > > > > > > > > > > For example, in corsairpsu_request, there are 2 commands, and they > > > > > should be executed consecutively: > > > > > 1) corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL); > > > > > 2) corsairpsu_usb_cmd(priv, 3, cmd, 0, data); > > > > > > > > > > If the userspace will squeeze another PSU_CMD_SELECT_RAIL between (1) > > > > > and (2), the driver will get data for a wrong rail (and with the > > > > > current code won't even notice it). > > > > > > > > > > So unless there is a way to "lock" hidraw (and it seems that there > > > > > isn't - looking at the code, hidraw calls the low-level hid driver > > > > > directly, as far as I understand), it won't work correctly. > > > > > > > > > > And if a driver can't work correctly with hidraw enabled - maybe it > > > > > shouldn't enable hidraw? So that the user can 1) notice that something > > > > > is wrong 2) blacklist or unbind the driver (or userspace tools that > > > > > use hidraw can unbind automatically). Otherwise everything seems to be > > > > > silently broken. > > > > > > > > > > On the other hand, maybe races between the kernel driver and userspace > > > > > tools are unlikely, because the driver doesn't talk to the device > > > > > continuously - only when sysfs reads happen. > > > > > > > > I never noticed any issues of that kind. I actually did quite a lot of > > > > userspace testing. A result of this a userspace tool you can find here: > > > > https://github.com/wgottwalt/corsair-psu/tree/main/tools/rmi-hxi-query > > > > > > > > Though, if you find a way to trigger such a race condition I have no > > > > problem to remove the hidraw part. > > > > > > > > greetings > > > > Will > > > > > > It is possible. Making a userspace tool with just a loop of read/writes > > > will get you wrong readings in the driver sometimes. > > > > Hmm, did you read the comments in the driver? I warn about writing nonsense > > values to the micro-controller because you can make it stall. If I let you > > access the device by hidraw I assume you know what you are doing. You > > actually can damage your PSU by this, something I also warn about. I even > > mention that I may remove the hidraw feature in future versions. > > Sorry for the confusion. I did not test with corsair-psu. (Do not have > the hardware) I tested with corsair-cpro. Reading temps with userspace and > reading fan speed with the driver simultaneously. > But you are right. This is also not, what a userspace tool should do this > fast and if it doesn't, races are really unlikely. > Same there: Make userspace and kernel mutually exclusive if parallel access is shown to be problematic. "Mutually exclusive" means disable userspace access completely while the driver is loaded, not some cross-subsystem mutex. Guenter
