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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux