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: <f1e16043-4841-411c-8feb-435b59d7f65a@roeck-us.net>
Date: Wed, 8 Jan 2025 19:04:22 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Leo Yang <leo.yang.sy0@...il.com>
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

On 1/8/25 16:50, Leo Yang wrote:
> 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++
> 
Sorry, I missed that MIN_M_VAL is negative.

Guenter

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