linux-kernel - Re: [PATCH 1/4] power: supply: max17042

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: <1c4a7088-bcef-ca5c-ff3e-efd1049dc402@redhat.com>
Date:   Fri, 18 Mar 2022 10:51:26 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>,
        Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org
Cc:     Purism Kernel Team <kernel@...i.sm>, Rob Herring <robh@...nel.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion
 macros

Hi,

On 3/18/22 10:06, Krzysztof Kozlowski wrote:
> On 18/03/2022 10:00, Hans de Goede wrote:
>> Hi,
>>
>> On 3/18/22 09:16, Krzysztof Kozlowski wrote:
>>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>>> Instead of sprinkling the code with magic numbers, put the unit
>>>> definitions used by the gauge into a set of macros. Macros are
>>>> used instead of simple defines in order to not require floating
>>>> point operations for divisions.
>>>>
>>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>
>>>> ---
>>>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>>>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>>>> index ab031bbfbe78..c019d6c52363 100644
>>>> --- a/drivers/power/supply/max17042_battery.c
>>>> +++ b/drivers/power/supply/max17042_battery.c
>>>> @@ -51,6 +51,15 @@
>>>>  
>>>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>>>  
>>>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
>>>
>>> Is this really long long? The usage in max17042_get_status() is with int
>>> operand and result.
>>
>> The "ll" is part of the original code which these macros replace,
>> dropping the "ll" is IMHO out of scope for this patch, it would
>> clearly break the only change 1 thing per patch/commit rule.
> 
> Not in max17042_get_status(). The usage there is without ll. Three other
> places use it in 64-bit context (result is 64-bit), so there indeed. But
> in max17042_get_status() this is now different.

Ah, good catch and there is a reason why it is not a ll there, a divide
is done on it, which is now a 64 bit divide which will break on 32 bit
builds...

Note that e.g. this existing block:

        case POWER_SUPPLY_PROP_CURRENT_NOW:
                if (chip->pdata->enable_current_sense) {
                        ret = regmap_read(map, MAX17042_Current, &data);
                        if (ret < 0)
                                return ret;

                        data64 = sign_extend64(data, 15) * 1562500ll;
                        val->intval = div_s64(data64, chip->pdata->r_sns);
                } else {
                        return -EINVAL;
                }
                break;

Solves this by using the div_s64 helper. So the code in max17042_get_status()
needs to be fixed to do the same.

The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
fit in a 32 bit integer.

So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
a potential bug there and as such that really should be done in
a separate preparation patch with a Cc stable.

Regards,

Hans