Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

From: Guenter Roeck
Date: Wed Oct 02 2019 - 21:18:22 EST


On 10/2/19 2:43 PM, Nick Desaulniers wrote:
On Mon, Sep 30, 2019 at 5:01 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:

Again, I fail to understand why waiting for a multiple of 20 seconds
under any circumstances would make any sense. Maybe the idea was
to divide us by 1000 before entering the second loop ?

Yes, that's very clearly a mistake of mine.


Looking into the code, there is no need to use udelay() in the first
place. It should be possible to replace the longer waits with
usleep_range(). Something like

if (us < some_low_value) // eg. 0x80
delay(us)

Did you mean udelay here?

Yes

else
usleep_range(us, us * 2);

should do, and at the same time prevent the system from turning
into a space heater.

The issue would persist with the above if udelay remains in a loop
that gets fully unrolled. That's while I "peel" the loop into two
loops over different ranges with different bodies.


Sorry, you lost me. If calls to udelay() with even small delay
parameters for some compiler-related reason no longer work, trying
to fix the problem with some odd driver code is most definitely not
a real solution.

I think I should iterate in the first loop until the number of `us` is
greater than 1000 (us per ms)(which is less of a magical constant and
doesn't expose internal implementation details of udelay), then start
the second loop (dividing us by 1000). What do you think, Guenter?


We should have no second loop, period.

Again, a hot delay loop of 128 ms (actually, more like 245 ms,
adding all delays together) is clearly wrong. Those udelay() calls
in the driver should really be replaced with usleep_range().

Guenter