linux-kernel - Re: [RFC v2] hwmon: add Corsair Commander Pro driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f078c0de-f66a-235a-b575-414a6b756786@roeck-us.net>
Date:   Mon, 15 Jun 2020 11:05:12 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Marius Zachmann <mail@...iuszachmann.de>
Cc:     jdelvare@...e.com, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [RFC v2] hwmon: add Corsair Commander Pro driver

On 6/15/20 7:54 AM, Marius Zachmann wrote:
[ ... ]
>>> +	memset(ccp->buffer, 0x00, OUT_BUFFER_SIZE);
>>> +	ccp->buffer[0] = CTL_SET_FAN_FPWM;
>>> +	ccp->buffer[1] = channel;
>>> +	ccp->buffer[2] = val;
>>
>> As it turns out (unless I am missing something), all commands have
>> one, two, or three parameters. Also, the full buffer is always
>> written. With that, it would be easier to add command, channel,
>> and value as parameters to send_usb_cmd() and handle both
>> memset() and assignments there.
>>
> 
> There is at least one command for setting a fan curve (0x25),
> which I intend to implement at a later point. It needs 27 bytes.
> There seem to be more long commands related to the RGB output.
> Also there is CTL_GET_FAN_CNCT which needs read 6 bytes of the
> response.
> 
In that case it might make sense to add a helper function to handle
_most_ of the commands. CTL_GET_FAN_CNCT can still be handled
with that helper function since the response is 6 bytes, not the
command itself.

>>> +	ret = send_usb_cmd(ccp);
>>> +
>>
>> This makes me wonder if reading a response is always necessary.
>> If not, it might make sense to add a flag to the function indicating
>> that no response is needed, and skip the read part there. If it is
>> needed, it might make sense to add a comment into the send function,
>> explaining that a read command must be executed even if no data is
>> returned.
>>
> 
> Actually yes. After a bit of further testing it seems, there is an
> error code in the first byte of the response, when sending invalid
> commands. I will check for it, return -EIO and log it, since I
> do not know, what all the error codes are.

Makes sense.

Thanks,
Guenter