Re: corsair-cpro and hidraw

[Wilken Gottwalt <wilken.gottwalt@posteo.net>]

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