linux-kernel - Re: [PATCH v4 05/21] hwmon: (mr75203) fix voltage equation for negative source input

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: <YxdSqw6STdkbDESV@smile.fi.intel.com>
Date:   Tue, 6 Sep 2022 17:01:15 +0300
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     "Farber, Eliav" <farbere@...zon.com>
Cc:     jdelvare@...e.com, linux@...ck-us.net, robh+dt@...nel.org,
        p.zabel@...gutronix.de, rtanwar@...linear.com,
        linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, hhhawa@...zon.com, jonnyc@...zon.com
Subject: Re: [PATCH v4 05/21] hwmon: (mr75203) fix voltage equation for
 negative source input

On Tue, Sep 06, 2022 at 04:27:13PM +0300, Farber, Eliav wrote:
> On 9/6/2022 3:03 PM, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 08:33:40AM +0000, Eliav Farber wrote:

...

> > > -             *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
> > > +             *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 <<
> > > PVT_CONV_BITS);
> > 
> > Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to
> > happen,
> > I hope) purposes:
> > 
> > 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31,
> >   which is according to C standard is UB (undefined behaviour).
> > 
> > 2) It makes shorter the line and also drops the pattern where some
> >   dumb robot may propose a patch to basically revert the division
> >   change.
> I originally tried to use BIT(PVT_CONV_BITS) but it gave a different
> result.
> e.g.
> If n = 2720
> *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS) = 0
> *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / BIT(PVT_CONV_BITS) =
> 18014398509481983
> 
> I can try fitting it in one line, either by adding a define for
> (1 << PVT_CONV_BITS) or exceeding 80 characters, but keep in mind that
> in a later patch (#15) it gets even longer (and I must use more than
> one line) since it is multiplied by a pre-scaler factor.

Don't get me wrong, it's not about style, it's about preventing
followup "fixes" of this. All the problems here due to (hidden)
unsigned type(s).

What you can do is to add a good comment on top of that line
explaining why division instead of right shift and why BIT()
may not be used (because it's unsigned).

-- 
With Best Regards,
Andy Shevchenko