Messages in this thread | | | From | Crt Mori <> | Date | Mon, 21 Jan 2019 21:25:00 +0100 | Subject | Re: fix int_sqrt() for very large numbers |
| |
On Mon, 21 Jan 2019 at 01:20, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sun, Jan 20, 2019 at 4:15 AM Florian La Roche > <florian.laroche@googlemail.com> wrote: > > > > @@ -52,7 +52,7 @@ u32 int_sqrt64(u64 x) > > if (x <= ULONG_MAX) > > return int_sqrt((unsigned long) x); > > > > - m = 1ULL << (fls64(x) & ~1ULL); > > + m = 1ULL << ((fls64(x) - 1) & ~1ULL); > > I've applied this part of the patch as commit fbfaf851902c ("fix > int_sqrt64() for very large numbers") with slightly edited commit > log. >
Thanks for the patch - I its indeed my copy-paste error, because __fls64 does not exist on 32bit CPU, but the fls64 is not "equal" replacement. I am very sorry for the bug.
> I still think there are some oddities in here in the types. I > mentioned the caller that unnecessarily does the int_sqrt64() twice, > even though the outer one doesn't actually take a 64-bit value. >
True. This is oddity is originating from time where mlx90632 used its own function for int_sqrt64 on 32bit.
> But in the very line above, there's another type oddity: the "& ~1ULL" > is entirely the wrong type. The shift *count* shouldn't be an unsigned > long long, so that type doesn't make much sense. It should be just a > ~1, or even just "62". >
This was also inline with above copy-paste and variable expansion to force the 64bit everywhere. I will prepare a patch to clean this line to ~1, question is why does the int_sqrt is having UL if this should just be "62". I was thinking because we want to cast to the type before we shift.
> But I didn't actually start micro-editing the patch, and just did that > one-liner off-by-one fix. > > Linus
| |