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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2c0942db0912151110n28f339a7m53d276044623adef@mail.gmail.com>
Date:	Tue, 15 Dec 2009 11:10:47 -0800
From:	Ray Lee <ray-lk@...rabbit.org>
To:	Justin Madru <jdm64@...ab.com>
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 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);
	}
}



>
> Justin
>
--
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/