linux-kernel - Re: [PATCH] hwmon: (aquacomputer_d5next) Add support for temperature sensor offsets

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]

Date:   Sun, 23 Oct 2022 15:15:36 +0200
From:   Aleksa Savic <savicaleksa83@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-hwmon@...r.kernel.org, leonard.anderweit@...il.com,
        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] hwmon: (aquacomputer_d5next) Add support for temperature
 sensor offsets

On 2022-10-22 15:57:20 GMT+02:00, Guenter Roeck wrote:
> Please go up to 100 columns to avoid excessive line splits.

Will fix this and other comments in v2.

> Is it really necessary to re-read the control buffer repeatedly
> to report this value ? I don't know how costly that is, but unlike
> the pwm value I would not expect the number to change.

Yes, aside from the driver userspace can also change settings on the
device using hidraw and we'd end up with stale data. Reading it is
very fast, it takes about 4ms in my testing.

> Also, is this number indeed not included in the regular reports
> sent from the controller ?

Unfortunately, it's not. The sensor report only includes final (calculated)
sensor readings.

> The driver doesn't distinguish between offsets in the control buffer
> (pwm, and now temperature sensor offset) and offsets in the report buffer,
> making it a bit difficult to determine if those are the same or not.
> Some explanation in the driver would be nice if someone finds the time
> to provide one. If the control buffer offsets are in a different number
> space, they should really be marked accordingly (for example with a
> _CTRL in the define).

I can see how it can be confusing. After this, I can send a patch to
reorder the macros & initializations and add more comments regarding
what is what.

Thanks,
Aleksa