linux-kernel - Re: [PATCH] staging: s5k3e2fx.c: reduce complexity by factoring

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4B27F849.8090406@gawab.com>
Date:	Tue, 15 Dec 2009 12:57:45 -0800
From:	Justin Madru <jdm64@...ab.com>
To:	Ray Lee <ray-lk@...rabbit.org>
CC:	gregkh@...e.de, linux-kernel@...r.kernel.org,
	Dan Carpenter <error27@...il.com>
Subject: Re: [PATCH] staging: s5k3e2fx.c: reduce complexity by factoring

On 12/15/2009 11:10 AM, Ray Lee wrote:
> On Tue, Dec 15, 2009 at 10:37 AM, Justin Madru<jdm64@...ab.com>  wrote:
>    
>> But, wouldn't you agree that if the code was suppose to deal with "rounding
>> issues" that there's a
>> simpler expression?
>>      
> No, I don't agree. Five minutes of effort below shows your patch will
> generate different numbers than the original. If this is controlling a
> stepper motor trying to hit a home position, it's off now. Or, the
> errors in the expressions for moving near and far may have balanced
> each other out before, and now there may be a systematic error causing
> a skew over time toward one end rather than the other.
>
> My point is that you need to run this past the guy with the actual
> hardware who wrote it in the first place such that it can be tested,
> and make sure the slapped-together expression isn't just working by
> accident, as ugly as it might be.
>
> #include<stdio.h>
>
> typedef int int32_t;
> typedef short int16_t;
> typedef unsigned int uint32_t;
>
> enum {MOVE_NEAR, MOVE_FAR} move_direction;
>
> int32_t s5k3e2fx_move_focus(int direction, int32_t num_steps)
> {
>          int32_t i;
>          int16_t step_direction;
>          int16_t actual_step;
>          int16_t s_move[5], s_move_2[5];
>          uint32_t gain, gain_2;
>
>          if (direction == MOVE_NEAR)
>                  step_direction = 20;
>          else
>                  step_direction = -20;
>
>          actual_step = step_direction * (int16_t)num_steps;
>
>          gain = actual_step * 0x400 / 5;
> 	gain_2 = actual_step / 5;
> 	
>          for (i = 0; i<= 4; i++) {
>                  if (actual_step>= 0)
>                          s_move[i] = ((((i+1)*gain+0x200) -
> (i*gain+0x200))/0x400);
>                  else
>                          s_move[i] = ((((i+1)*gain-0x200) -
> (i*gain-0x200))/0x400);
>          }
>
>          for (i = 0; i<= 4; i++)
>                  s_move_2[i] = gain_2;
>
> 	if (memcmp(s_move, s_move_2, sizeof(s_move))) {
> 		printf("s1, s2 differ for direction %d, num_steps %d\n", direction,
> num_steps);
> 		for (i=0; i<5; i++)
> 			printf(" [%d] %d %d", i, s_move[i], s_move_2[i]);
> 		printf("\n");
> 	}
>
> }
>
> int main(void) {
> 	int steps;
> 	for (steps = -65535; steps<  65536; steps++) {
> 		s5k3e2fx_move_focus(MOVE_NEAR, steps);
> 		s5k3e2fx_move_focus(MOVE_FAR, steps);
> 	}
> }
>
>    

Ok, I tested the example code and it does lead to different values!
But, I did some testing and came up with a new patch that has been tested
this time to come up with the same values as the old code but uses less 
calculations.

gain = ((actual_step << 10) / 5) >> 10;
         for (i = 0; i <= 4; i++)
                 s_move[i] = gain;

Greg, disregard my last patch. Instead, please accept my new patch -- 
pending review.
         http://lkml.org/lkml/2009/12/15/453

Justin Madru
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/