defects for uses of abs(u64) (was: Re: Regression: can't apply frequency offsets above 1000ppm)

[Joe Perches <joe@perches.com>]

On Fri, 2015-09-04 at 18:00 -0700, John Stultz wrote:
> On Fri, Sep 4, 2015 at 5:57 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Thu, Sep 3, 2015 at 4:26 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote:
> >> On Wed, Sep 02, 2015 at 04:16:00PM -0700, John Stultz wrote:
> >>> On Tue, Sep 1, 2015 at 6:14 PM, Nuno Gonçalves <nunojpg@gmail.com> wrote:
> >>> > And just installing chrony from the feeds. With any kernel from 3.17
> >>> > you'll have wrong estimates at chronyc sourcestats.
> >>>
> >>> Wrong estimates? Could you be more specific about what the failure
> >>> you're seeing is here? The
> >>>
> >>> I installed the image above, which comes with a 4.1.6 kernel, and
> >>> chrony seems to have gotten my BBB into ~1ms sync w/ servers over the
> >>> internet fairly quickly (at least according to chronyc tracking).
> >>
> >> To see the bug with chronyd the initial offset shouldn't be very close
> >> to zero, so it's forced to correct the offset by adjusting the
> >> frequency in a larger step.
> >>
> >> I'm attaching a simple C program that prints the frequency offset
> >> as measured between the REALTIME and MONOTONIC_RAW clocks when the
> >> adjtimex tick is set to 9000. It should show values close to -100000
> >> ppm and I suspect on the BBB it will be much smaller.
> >
> > So I spent some time on this late last night and this afternoon.
> >
> > It was a little odd because things don't seem totally broken, but
> > something isn't quite right.
> >
> > Digging around it seems the iterative logrithmic approximation done in
> > timekeeping_freqadjust() wasn't working right. Instead of making
> > smaller order alternating positive and negative adjustments, it was
> > doing strange growing adjustments for the same value that wern't large
> > enough to actually correct things very quickly. This made it much
> > slower to adapt to specified frequency values.
> >
> > The odd bit, is it seems to come down to:
> >     tick_error = abs(tick_error);
> >
> > Haven't chased down why yet, but apparently abs() isn't doing what one
> > would think when passed a s64 value.
> 
> Well.. chasing it down wasn't hard.. from include/linux/kernel.h:
> /*
>  * abs() handles unsigned and signed longs, ints, shorts and chars.  For all
>  * input types abs() returns a signed long.
>  * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
>  * for those.
>  */
> 
> Ouch.

Here's a little cocci script that finds more of these in:

lib/percpu_counter.c
drivers/input/joystick/walkera0701.c
drivers/md/raid5.c
drivers/spi/spi-pxa2xx.c
fs/f2fs/debug.c

$ cat abs.cocci
@@
u64 t;
@@

*	abs(t)

@@
s64 t;
@@

*	abs(t)

@@
long long t;
@@

*	abs(t)

@@
unsigned long long t;
@@

*	abs(t)

@@
uint64_t t;
@@

*	abs(t)

@@
int64_t t;
@@

*	abs(t)

$

diff -u -p ./lib/percpu_counter.c /tmp/nothing/lib/percpu_counter.c
--- ./lib/percpu_counter.c
+++ /tmp/nothing/lib/percpu_counter.c
@@ -203,7 +203,6 @@ int __percpu_counter_compare(struct perc
 
 	count = percpu_counter_read(fbc);
 	/* Check to see if rough count will be sufficient for comparison */
-	if (abs(count - rhs) > (batch * num_online_cpus())) {
 		if (count > rhs)
 			return 1;
 		else

diff -u -p ./drivers/input/joystick/walkera0701.c /tmp/nothing/drivers/input/joystick/walkera0701.c
--- ./drivers/input/joystick/walkera0701.c
+++ /tmp/nothing/drivers/input/joystick/walkera0701.c
@@ -150,7 +150,6 @@ static void walkera0701_irq_handler(void
 		if (w->counter == 24) {	/* full frame */
 			walkera0701_parse_frame(w);
 			w->counter = NO_SYNC;
-			if (abs(pulse_time - SYNC_PULSE) < RESERVE)	/* new frame sync */
 				w->counter = 0;
 		} else {
 			if ((pulse_time > (ANALOG_MIN_PULSE - RESERVE)
@@ -161,7 +160,6 @@ static void walkera0701_irq_handler(void
 			} else
 				w->counter = NO_SYNC;
 		}
-	} else if (abs(pulse_time - SYNC_PULSE - BIN0_PULSE) <
 				RESERVE + BIN1_PULSE - BIN0_PULSE)	/* frame sync .. */
 		w->counter = 0;

diff -u -p ./drivers/md/raid5.c /tmp/nothing/drivers/md/raid5.c
--- ./drivers/md/raid5.c
+++ /tmp/nothing/drivers/md/raid5.c
@@ -6701,8 +6701,6 @@ static int run(struct mddev *mddev)
 			 * readonly mode so it can take control before
 			 * allowing any writes.  So just check for that.
 			 */
-			if (abs(min_offset_diff) >= mddev->chunk_sectors &&
-			    abs(min_offset_diff) >= mddev->new_chunk_sectors)
 				/* not really in-place - so OK */;
 			else if (mddev->ro == 0) {
 				printk(KERN_ERR "md/raid:%s: in-place reshape "

diff -u -p ./drivers/spi/spi-pxa2xx.c /tmp/nothing/drivers/spi/spi-pxa2xx.c
--- ./drivers/spi/spi-pxa2xx.c
+++ /tmp/nothing/drivers/spi/spi-pxa2xx.c
@@ -786,7 +786,6 @@ static unsigned int quark_x1000_get_clk_
 		/* Get the remainder */
 		fssp = (u64)fref * m;
 		do_div(fssp, 1 << 24);
-		r1 = abs(fssp - rate);
 
 		/* Choose this one if it suits better */
 		if (r1 < r) {

diff -u -p ./fs/f2fs/debug.c /tmp/nothing/fs/f2fs/debug.c
--- ./fs/f2fs/debug.c
+++ /tmp/nothing/fs/f2fs/debug.c
@@ -109,7 +109,6 @@ static void update_sit_info(struct f2fs_
 	hblks_per_sec = blks_per_sec / 2;
 	for (segno = 0; segno < MAIN_SEGS(sbi); segno += sbi->segs_per_sec) {
 		vblocks = get_valid_blocks(sbi, segno, sbi->segs_per_sec);
-		dist = abs(vblocks - hblks_per_sec);
 		bimodal += dist * dist;
 
 		if (vblocks > 0 && vblocks < blks_per_sec) {