Re: corsair-cpro and hidraw

On Fri, Jun 18, 2021 at 06:47:37AM +0000, Wilken Gottwalt wrote:

On Fri, 18 Jun 2021 08:18:23 +0200
Marius Zachmann [off-list ref] wrote:

quoted

On 18.06.21 at 07:45:00 CEST, Wilken Gottwalt wrote

quoted

On Fri, 18 Jun 2021 05:56:29 +0600
Aleksandr Mezin [off-list ref] wrote:

quoted

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.

quoted

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