On Fri, Jun 18, 2021 at 06:47:37AM +0000, 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. > > > Removing hidraw from the drivers is not a solution, because there are > > many userspace tools for these devices and it should be an expected use case > > to have them running at the same time (eg OpenRGB for rgb) > > corsair-psu is a hwmon driver, not a gadget driver. This driver does not > have to provide hidraw, only hwmon api access. The hidraw is just a nice > to have feature. And like my query tool shows, you actually don't need the > driver to query it from userspace. The first prototypes of the driver did > locking in the hidraw part which sometimes resulted in lockups. I will > not increase complexity for a nice-to-have feature. So the only option is, > I remove the feature if it causes trouble. > > Guenter, what do you think? > Don't change something that isn't broken. Again, if a user wants to use userspace tools to access PSU data, that user should only use userspace tools. If there is a real problem with parallel accesses from userspace and the kernel, maybe we can configure the driver such that it blocks userspace access while the driver is loaded. Everything else seems overkill (such the idea of mutexes covering multiple subsystems, if I understand that correctly). Anyone who wants to use userspace tools can then just disable or unload the kernel driver. Guenter
