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. 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) I think the correct solution would be to lock hidraw while the drivers are doing requests. After a (short) look: Introducing a mutex in the hidraw struct which would be locked in hidraw_ioctl and could also be locked in the corsair-psu and corsair-cpro drivers could be a solution. If there are no objections or better suggestions, I will try this over the weekend. Greetings Marius Added Jiri Kosina for hidraw to Cc: > > > Added corsair-psu maintainer to Cc: > > > > On Thu, Jun 17, 2021 at 7:14 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > > > > > On Thu, Jun 17, 2021 at 01:11:38PM +0600, Aleksandr Mezin wrote: > > > > On Thu, Jun 17, 2021 at 12:27 PM Marius Zachmann <mail@xxxxxxxxxxxxxxxxx> wrote: > > > > ... > > > > > This device uses an echo of the command > > > > > in the answer and if they don't match it returns an error. This could > > > > > maybe lead to a false error when the replies are switched, but is > > > > > probably preferable. > > > > > > > > Hm... If the response includes the id of the request, it should be > > > > possible to filter reports in raw_event, i. e. don't signal completion > > > > if the report doesn't match, and wait more. Yes, there is a corner > > > > case, "if a command is not supported, the length value in the reply is > > > > okay, but the command value is set to 0". But timing out (250 ms) in > > > > this case should probably be fine... Actually I have a compatible > > > > Corsair PSU so maybe I'll send a patch. > > > > > > Patches to improve the situation are welcome. My understanding is > > > that with the current driver users should disable the kernel driver > > > if they plan to use userspace tools to access the device. > > > > > > Guenter > >
