linux-kernel - Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

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: <CAAfUjZE2x_Fafogna2yhnnohZrGmtW5G3Q64AVhYwVEXuGoaBw@mail.gmail.com>
Date: Thu, 9 Jan 2025 08:50:59 +0800
From: Leo Yang <leo.yang.sy0@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: jdelvare@...e.com, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, Leo-Yang@...ntatw.com, corbet@....net, 
	Delphine_CC_Chiu@...ynn.com, linux-hwmon@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: Add driver for TI INA233 Current and Power Monitor

Hi Guenter,

On Mon, Jan 6, 2025 at 11:31 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> Besides, while I did point out a number of problems, but I did not suggest
> to "rewrite the driver".
>
> Since this is v2 of this driver, the submission should have been versioned,
> and a change log should have been provided.
>

Sorry this is my first v2 patch,
I should have been more aware of this problem, thank you.

>
> Why not just pass the power coefficient directly as parameter ?
>
> > +     if (1000000 % *m) {
>
> I fail to understand the logic here. Why scale if and only if m is a clean
> multiple of 1000000 ? Scale if m == 1000001 but not if m == 1000000 ?
> Please explain.
>
> > +             /* Default value, Scaling to keep integer precision,
> > +              * Change it if you need
>
> This comment does not provide any actual information and thus does not
> add any value. Change to what ? Why ? And, again, why not scale if
> m is a multiple of 1000000, no matter how large or small it is ?
>

When we calculate the Telemetry and Warning Conversion Coefficients,
the m-value of the current needs to be calculated via Equation:
1(A)/Current_LSB(A).

The number 1000000 comes from A->uA to match the unit uA of Current_LSB.
Try to prevent the loss of fractional precision in integer.

But this is not enough,
according to spec 7.5.4 Reading Telemetry Data and Warning Thresholds
If there is decimal information in m, we should try to move the decimal point
so that the value of m is between -32768 and 32767 and maximize it as much
as possible to minimize rounding errors.

Therefore, if m does not have decimal information, even if the value of m is
scaled up, it is not possible to minimize rounding errors.

But my comments are not clear enough, I'll fix it.

> > +
> > +     /* Maximize while keeping it bounded.*/
> > +     while (scaled_m > MAX_M_VAL || scaled_m < MIN_M_VAL) {
> > +             scaled_m /= 10;
>
> This looks wrong. If scaled_m < MIN_M_VAL it doesn't make sense
> to decrease it even more.
>

In this part, I try to move the decimal point so that the value of m is between
-32768 and 32767.
Assuming scaled_m = -40001, I can scale it to m = -4000 and adjust it by R++

> > +             scale_factor++;
> > +     }
> > +     /* Scale up only if fractional part exists. */
> > +     while (scaled_m * 10 < MAX_M_VAL && scaled_m * 10 > MIN_M_VAL && !is_integer) {
>
> This looks just as wrong. If scaled_m > 10 * MIN_M_VAL, why increase it even more ?
>

I think the purpose of spec is to keep as many integers as possible in m, and
then save the information in decimals via R to minimize rounding errors.
So here I keep the positive numbers as close to 32767 as possible, and the
negative numbers as close to -32768 as possible.

And thank you for the suggestions, they are very helpful and I will
try to fix them.

Best Regards,

Leo Yang