corsair-cpro and hidraw

corsair-cpro and hidraw

[Aleksandr Mezin]

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)

Re: corsair-cpro and hidraw

[Aleksandr Mezin]

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.

Re: corsair-cpro and hidraw

[Marius Zachmann]

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

Re: corsair-cpro and hidraw

[Aleksandr Mezin]

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.

Re: corsair-cpro and hidraw

[Guenter Roeck]

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

Re: corsair-cpro and hidraw

[Aleksandr Mezin]

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

Re: corsair-cpro and hidraw

[Wilken Gottwalt]

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

Re: corsair-cpro and hidraw

[Marius Zachmann]

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
> 
> 

Re: corsair-cpro and hidraw

[Wilken Gottwalt]

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
> > 
> > 
> 
> 
> 
> 

Re: corsair-cpro and hidraw

[Marius Zachmann]

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
> > > 
> > > 
> > 
> > 
> > 
> > 
> 
> 

Re: corsair-cpro and hidraw

[Guenter Roeck]

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

Re: corsair-cpro and hidraw

[Marius Zachmann]

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

Re: corsair-cpro and hidraw

[Guenter Roeck]

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

Re: corsair-cpro and hidraw

[Guenter Roeck]

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