* corsair-cpro and hidraw @ 2021-06-17 3:11 Aleksandr Mezin 2021-06-17 4:33 ` Aleksandr Mezin 2021-06-17 6:27 ` Marius Zachmann 0 siblings, 2 replies; 14+ messages in thread From: Aleksandr Mezin @ 2021-06-17 3:11 UTC (permalink / raw) To: Marius Zachmann, linux-hwmon Hello. I've noticed that corsair-cpro communicates with the device in request-reply pattern, and also exposes hidraw interface, so userspace can also send requests. How does the driver figure out that the current input report is a response to the request sent by the driver, and not for some other request from userspace? Also if userspace reads from hidraw, how can it be sure that the report it read is the response to its last request, and not to some driver's request? I do not have the hardware, I was just looking at corsair-cpro as a reference - I'm working on a driver for a similar device (nzxt rgb & fan controller) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-17 3:11 corsair-cpro and hidraw Aleksandr Mezin @ 2021-06-17 4:33 ` Aleksandr Mezin 2021-06-17 6:27 ` Marius Zachmann 1 sibling, 0 replies; 14+ messages in thread From: Aleksandr Mezin @ 2021-06-17 4:33 UTC (permalink / raw) To: Marius Zachmann, linux-hwmon On Thu, Jun 17, 2021 at 9:11 AM Aleksandr Mezin <mezin.alexander@gmail.com> wrote: > > Hello. > > I've noticed that corsair-cpro communicates with the device in > request-reply pattern, and also exposes hidraw interface, so userspace > can also send requests. How does the driver figure out that the > current input report is a response to the request sent by the driver, > and not for some other request from userspace? Also if userspace reads > from hidraw, how can it be sure that the report it read is the > response to its last request, and not to some driver's request? > > I do not have the hardware, I was just looking at corsair-cpro as a > reference - I'm working on a driver for a similar device (nzxt rgb & > fan controller) Oops, there's a comment about this exact issue at the beginning of the file, and I missed it somehow. Sorry for the noise. I wonder if the driver can synchronize the access through hidraw somehow, i. e. make hidraw read/write block while mutex is locked. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-17 3:11 corsair-cpro and hidraw Aleksandr Mezin 2021-06-17 4:33 ` Aleksandr Mezin @ 2021-06-17 6:27 ` Marius Zachmann 2021-06-17 7:11 ` Aleksandr Mezin 1 sibling, 1 reply; 14+ messages in thread From: Marius Zachmann @ 2021-06-17 6:27 UTC (permalink / raw) To: Aleksandr Mezin; +Cc: linux-hwmon Am Donnerstag, 17. Juni 2021, 05:11:10 CEST schrieben Sie: > Hello. > > I've noticed that corsair-cpro communicates with the device in > request-reply pattern, and also exposes hidraw interface, so userspace > can also send requests. How does the driver figure out that the > current input report is a response to the request sent by the driver, > and not for some other request from userspace? Also if userspace reads > from hidraw, how can it be sure that the report it read is the > response to its last request, and not to some driver's request? > > I do not have the hardware, I was just looking at corsair-cpro as a > reference - I'm working on a driver for a similar device (nzxt rgb & > fan controller) > Hello, Sadly, it doesn't. There is no way to distinguish where the reply is coming from. The hardware should probably use report ids in the first byte of the reply for something like this, but it does not. I thought about possibilities to stop hidraw before a request and restart it after the reply, but there is (as far as I know) no way to do this without changing the hid-driver. Maybe I should have a look at this idea again. The choice was either to allow it or block most userspace tools. I also tried to get false readings and it can be done, albeit highly unlikely. As a "solution" I slapped a comment at the beginning of the driver to point out the problem. I do not know, what your device is doing, but the (similar) corsair-psu driver has the same problem. 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. Greetings Marius ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-17 6:27 ` Marius Zachmann @ 2021-06-17 7:11 ` Aleksandr Mezin 2021-06-17 13:14 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Aleksandr Mezin @ 2021-06-17 7:11 UTC (permalink / raw) To: Marius Zachmann; +Cc: linux-hwmon On Thu, Jun 17, 2021 at 12:27 PM Marius Zachmann <mail@mariuszachmann.de> wrote: ... > I do not know, what your device is doing Actually, NZXT devices (at least grid/"smart device" and "smart device v2"/rgb&fan controller) don't have such issues - they use report ids, and don't even expect request-reply communication pattern. I've just noticed that something seems to be wrong with corsair-cpro (but somehow didn't notice the comment) and decided to ask. > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-17 7:11 ` Aleksandr Mezin @ 2021-06-17 13:14 ` Guenter Roeck 2021-06-17 23:56 ` Aleksandr Mezin 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2021-06-17 13:14 UTC (permalink / raw) To: Aleksandr Mezin; +Cc: Marius Zachmann, linux-hwmon 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@mariuszachmann.de> wrote: > ... > > I do not know, what your device is doing > > Actually, NZXT devices (at least grid/"smart device" and "smart device > v2"/rgb&fan controller) don't have such issues - they use report ids, > and don't even expect request-reply communication pattern. I've just > noticed that something seems to be wrong with corsair-cpro (but > somehow didn't notice the comment) and decided to ask. > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-17 13:14 ` Guenter Roeck @ 2021-06-17 23:56 ` Aleksandr Mezin 2021-06-18 5:45 ` Wilken Gottwalt 0 siblings, 1 reply; 14+ messages in thread From: Aleksandr Mezin @ 2021-06-17 23:56 UTC (permalink / raw) To: Guenter Roeck; +Cc: Marius Zachmann, linux-hwmon, Wilken Gottwalt 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. Added corsair-psu maintainer to Cc: On Thu, Jun 17, 2021 at 7:14 PM Guenter Roeck <linux@roeck-us.net> 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@mariuszachmann.de> wrote: > > ... > > > I do not know, what your device is doing > > > > Actually, NZXT devices (at least grid/"smart device" and "smart device > > v2"/rgb&fan controller) don't have such issues - they use report ids, > > and don't even expect request-reply communication pattern. I've just > > noticed that something seems to be wrong with corsair-cpro (but > > somehow didn't notice the comment) and decided to ask. > > > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-17 23:56 ` Aleksandr Mezin @ 2021-06-18 5:45 ` Wilken Gottwalt 2021-06-18 6:18 ` Marius Zachmann 0 siblings, 1 reply; 14+ messages in thread From: Wilken Gottwalt @ 2021-06-18 5:45 UTC (permalink / raw) To: Aleksandr Mezin; +Cc: Guenter Roeck, Marius Zachmann, linux-hwmon On Fri, 18 Jun 2021 05:56:29 +0600 Aleksandr Mezin <mezin.alexander@gmail.com> 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 > Added corsair-psu maintainer to Cc: > > On Thu, Jun 17, 2021 at 7:14 PM Guenter Roeck <linux@roeck-us.net> 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@mariuszachmann.de> wrote: > > > ... > > > > I do not know, what your device is doing > > > > > > Actually, NZXT devices (at least grid/"smart device" and "smart device > > > v2"/rgb&fan controller) don't have such issues - they use report ids, > > > and don't even expect request-reply communication pattern. I've just > > > noticed that something seems to be wrong with corsair-cpro (but > > > somehow didn't notice the comment) and decided to ask. > > > > > > > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 5:45 ` Wilken Gottwalt @ 2021-06-18 6:18 ` Marius Zachmann 2021-06-18 6:47 ` Wilken Gottwalt 0 siblings, 1 reply; 14+ messages in thread From: Marius Zachmann @ 2021-06-18 6:18 UTC (permalink / raw) To: Aleksandr Mezin, Wilken Gottwalt; +Cc: Guenter Roeck, linux-hwmon, Jiri Kosina 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@gmail.com> 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@roeck-us.net> 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@mariuszachmann.de> 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 6:18 ` Marius Zachmann @ 2021-06-18 6:47 ` Wilken Gottwalt 2021-06-18 7:06 ` Marius Zachmann 2021-06-18 12:10 ` Guenter Roeck 0 siblings, 2 replies; 14+ messages in thread From: Wilken Gottwalt @ 2021-06-18 6:47 UTC (permalink / raw) To: Marius Zachmann; +Cc: Aleksandr Mezin, Guenter Roeck, linux-hwmon, Jiri Kosina On Fri, 18 Jun 2021 08:18:23 +0200 Marius Zachmann <mail@mariuszachmann.de> 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@gmail.com> 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? greetings, Will > 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@roeck-us.net> 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@mariuszachmann.de> 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 > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 6:47 ` Wilken Gottwalt @ 2021-06-18 7:06 ` Marius Zachmann 2021-06-18 12:13 ` Guenter Roeck 2021-06-18 12:10 ` Guenter Roeck 1 sibling, 1 reply; 14+ messages in thread From: Marius Zachmann @ 2021-06-18 7:06 UTC (permalink / raw) To: Wilken Gottwalt; +Cc: Aleksandr Mezin, Guenter Roeck, linux-hwmon, Jiri Kosina On 18.06.21 at 08:47:37 CEST, Wilken Gottwalt wrote > On Fri, 18 Jun 2021 08:18:23 +0200 > Marius Zachmann <mail@mariuszachmann.de> 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@gmail.com> 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. > > > 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? > > greetings, > Will > > > 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@roeck-us.net> 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@mariuszachmann.de> 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 > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 7:06 ` Marius Zachmann @ 2021-06-18 12:13 ` Guenter Roeck 2021-06-18 12:22 ` Marius Zachmann 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2021-06-18 12:13 UTC (permalink / raw) To: Marius Zachmann Cc: Wilken Gottwalt, Aleksandr Mezin, linux-hwmon, Jiri Kosina 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@mariuszachmann.de> 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@gmail.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 12:13 ` Guenter Roeck @ 2021-06-18 12:22 ` Marius Zachmann 2021-06-18 19:41 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Marius Zachmann @ 2021-06-18 12:22 UTC (permalink / raw) To: Guenter Roeck; +Cc: Wilken Gottwalt, Aleksandr Mezin, linux-hwmon, Jiri Kosina On 18.06.21 at 14:13:00 CEST, Guenter Roeck wrote > 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@mariuszachmann.de> 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@gmail.com> 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 > For now I did not get a bug report nor did anyone seem to have a real problem. It is mostly a theoretical issue. I am not unhappy about how it is. Maybe until a real problem occurs, we should just do nothing? Greetings Marius ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 12:22 ` Marius Zachmann @ 2021-06-18 19:41 ` Guenter Roeck 0 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2021-06-18 19:41 UTC (permalink / raw) To: Marius Zachmann Cc: Wilken Gottwalt, Aleksandr Mezin, linux-hwmon, Jiri Kosina On Fri, Jun 18, 2021 at 02:22:57PM +0200, Marius Zachmann wrote: > > > > 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 > > > > For now I did not get a bug report nor did anyone seem to have a real problem. > It is mostly a theoretical issue. > I am not unhappy about how it is. > Maybe until a real problem occurs, we should just do nothing? > That would be my suggestion. Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: corsair-cpro and hidraw 2021-06-18 6:47 ` Wilken Gottwalt 2021-06-18 7:06 ` Marius Zachmann @ 2021-06-18 12:10 ` Guenter Roeck 1 sibling, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2021-06-18 12:10 UTC (permalink / raw) To: Wilken Gottwalt Cc: Marius Zachmann, Aleksandr Mezin, linux-hwmon, Jiri Kosina 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@mariuszachmann.de> 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@gmail.com> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-18 19:41 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-17 3:11 corsair-cpro and hidraw Aleksandr Mezin 2021-06-17 4:33 ` Aleksandr Mezin 2021-06-17 6:27 ` Marius Zachmann 2021-06-17 7:11 ` Aleksandr Mezin 2021-06-17 13:14 ` Guenter Roeck 2021-06-17 23:56 ` Aleksandr Mezin 2021-06-18 5:45 ` Wilken Gottwalt 2021-06-18 6:18 ` Marius Zachmann 2021-06-18 6:47 ` Wilken Gottwalt 2021-06-18 7:06 ` Marius Zachmann 2021-06-18 12:13 ` Guenter Roeck 2021-06-18 12:22 ` Marius Zachmann 2021-06-18 19:41 ` Guenter Roeck 2021-06-18 12:10 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.