linux-kernel - Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <904da857-2e27-87a1-4541-6288df564d80@roeck-us.net>
Date:   Sat, 11 Feb 2023 12:48:56 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Leonard Anderweit <leonard.anderweit@...il.com>,
        Aleksa Savic <savicaleksa83@...il.com>,
        linux-hwmon@...r.kernel.org
Cc:     Jack Doan <me@...kdoan.com>, Jean Delvare <jdelvare@...e.com>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset
 control for Aquaero

On 2/11/23 11:48, Leonard Anderweit wrote:
> Am 11.02.23 um 19:54 schrieb Aleksa Savic:
>> On 2023-02-11 19:08:27 GMT+01:00, Guenter Roeck wrote:
>>>
>>> aquaero is already supported, and the checksum is so far generated
>>> and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
>>> don't need it ?
>>
>> Reading its sensors is currently supported, not writing to it (before these
>> patches).
>>
>> The checksum is ignored and not needed for either aquaero 5 (which Leonard has)
>> nor 6 (which I have).
>>
>>>
>>> If it is not needed and ignored, does it really add value to selectively drop it ?
>>
>> I think we can indeed remove that check.
> 
> I don't think that check can be removed as the checksum is not appended to the control report but is in the last two bytes. So using the checksum on Aquaero will overwrite the data at that location. It is currently unknown what these two bytes do so I do not want to overwrite them.
> 

The current code _does_ overwrite those bytes, or am I missing something ?

If so, changing that would be a bug fix which really should not be hidden
in a patch making functional changes.

Thanks,
Guenter

>>
>> Thanks,
>> Aleksa
>>
>>>
>>> Either case, this change is not mentioned in the commit log, and it
>>> violates the "one logical change per patch" rule. Please split it into
>>> a separate patch and explain why the change is needed.
>>>
>>> Another change to separate is the introduction of ctrl_report_id
>>> and the secondary_ctrl_report variables, which is also done silently
>>> and not explained. That should also be a separate patch to simplify
>>> review.
> 
> I will separate the changes into more commits for the next version.
> 
> Regards,
> Leonard
> 
>>>
>>> Thanks,
>>> Guenter
>>