2009-01-18 23:35:57

by Roel Kluin

Subject: [PATCH] DVB: negative internal->sub_range won't get noticed

internal->sub_range is unsigned, a negative won't get noticed.

Signed-off-by: Roel Kluin <[email protected]>
---
diff --git a/drivers/media/dvb/frontends/stb0899_algo.c b/drivers/media/dvb/frontends/stb0899_algo.c
index 83dc7e1..2ea32da 100644
--- a/drivers/media/dvb/frontends/stb0899_algo.c
+++ b/drivers/media/dvb/frontends/stb0899_algo.c
@@ -464,13 +464,14 @@ static void next_sub_range(struct stb0899_state *state)

if (internal->sub_dir > 0) {
old_sub_range = internal->sub_range;
- internal->sub_range = MIN((internal->srch_range / 2) -
+ if (internal->tuner_offst + internal->sub_range / 2 >=
+ internal->srch_range / 2)
+ internal->sub_range = 0;
+ else
+ internal->sub_range = MIN((internal->srch_range / 2) -
(internal->tuner_offst + internal->sub_range / 2),
internal->sub_range);

- if (internal->sub_range < 0)
- internal->sub_range = 0;
-
internal->tuner_offst += (old_sub_range + internal->sub_range) / 2;
}

2009-01-27 06:41:20

by Andrew Morton

Subject: Re: [PATCH] DVB: negative internal->sub_range won't get noticed

On Mon, 19 Jan 2009 00:35:47 +0100 Roel Kluin <[email protected]> wrote:

> internal->sub_range is unsigned, a negative won't get noticed.
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> diff --git a/drivers/media/dvb/frontends/stb0899_algo.c b/drivers/media/dvb/frontends/stb0899_algo.c
> index 83dc7e1..2ea32da 100644
> --- a/drivers/media/dvb/frontends/stb0899_algo.c
> +++ b/drivers/media/dvb/frontends/stb0899_algo.c
> @@ -464,13 +464,14 @@ static void next_sub_range(struct stb0899_state *state)
>
> if (internal->sub_dir > 0) {
> old_sub_range = internal->sub_range;
> - internal->sub_range = MIN((internal->srch_range / 2) -
> + if (internal->tuner_offst + internal->sub_range / 2 >=
> + internal->srch_range / 2)
> + internal->sub_range = 0;
> + else
> + internal->sub_range = MIN((internal->srch_range / 2) -
> (internal->tuner_offst + internal->sub_range / 2),
> internal->sub_range);
>
> - if (internal->sub_range < 0)
> - internal->sub_range = 0;
> -
> internal->tuner_offst += (old_sub_range + internal->sub_range) / 2;
> }

I hope someone understands that function :(

Do we actually need that test at all? Perhaps it has never triggered?
Perhaps values in the 0x80000000 - 0xffffffff are actually OK?

This driver has managed to get itself a secret private version of the
min(), max() and abs() macros. They're buggy - they reference their
argument multiple times. The driver should be converted to use the
kernel.h versions.

2009-01-27 18:52:28

by Manu Abraham

Subject: Re: [PATCH] DVB: negative internal->sub_range won't get noticed

On Mon, 19 Jan 2009 00:35:47 +0100 Roel Kluin <[email protected]>
wrote:

> internal->sub_range is unsigned, a negative won't get noticed.
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> diff --git a/drivers/media/dvb/frontends/stb0899_algo.c
b/drivers/media/dvb/frontends/stb0899_algo.c
> index 83dc7e1..2ea32da 100644
> --- a/drivers/media/dvb/frontends/stb0899_algo.c
> +++ b/drivers/media/dvb/frontends/stb0899_algo.c
> @@ -464,13 +464,14 @@ static void next_sub_range(struct
stb0899_state *state)
>
> if (internal->sub_dir > 0) {
> old_sub_range = internal->sub_range;
> - internal->sub_range = MIN((internal->srch_range / 2) -
> + if (internal->tuner_offst + internal->sub_range / 2 >=
> + internal->srch_range / 2)
> + internal->sub_range = 0;
> + else
> + internal->sub_range = MIN((internal->srch_range / 2) -
> (internal->tuner_offst + internal->sub_range / 2),
> internal->sub_range);
>
> - if (internal->sub_range < 0)
> - internal->sub_range = 0;
> -
> internal->tuner_offst += (old_sub_range + internal->sub_range) / 2;
> }

> I hope someone understands that function :(


I guess the relevant people might. Sending it to wrong people and to
a wrong mailing list, doesn't help much i guess.


That function does a step through a "next slice" of the RF carrier
based on a RF center frequency, which is used for carrier acquisition.


> Do we actually need that test at all? Perhaps it has never
triggered?


Ranges can go negative, from the current offset to the left anything
is negative, to the right it is positive. There of course is a bug
that the variable is not signed.

I have pushed out a fix out here at
http://jusst.de/hg/v4l-dvb/rev/368dc6078295

including your comment on the macros. Thanks for pointing the bug.

Regards,
Manu