[PATCH] 2.5.59 : sound/oss/vidc.c

[PATCH] 2.5.59 : sound/oss/vidc.c

[Frank Davis]

Hello all,
   The following patch addresses buzilla bug # 318, and removes an
offending semicolon. Please review for inclusion.

Regards,
Frank

--- linux/sound/oss/vidc.c.old	2003-01-16 21:21:38.000000000 -0500
+++ linux/sound/oss/vidc.c	2003-02-07 02:59:44.000000000 -0500
@@ -225,7 +225,7 @@
 			newsize = 208;
 		if (newsize > 4096)
 			newsize = 4096;
-		for (new2size = 128; new2size < newsize; new2size <<= 1);
+		for (new2size = 128; new2size < newsize; new2size <<= 1)
 			if (new2size - newsize > newsize - (new2size >> 1))
 				new2size >>= 1;
 		if (new2size > 4096) {

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Russell King]

On Fri, Feb 07, 2003 at 12:13:04PM -0500, Frank Davis wrote:
> Hello all,
>    The following patch addresses buzilla bug # 318, and removes an
> offending semicolon. Please review for inclusion.
> 
> Regards,
> Frank
> 
> --- linux/sound/oss/vidc.c.old	2003-01-16 21:21:38.000000000 -0500
> +++ linux/sound/oss/vidc.c	2003-02-07 02:59:44.000000000 -0500
> @@ -225,7 +225,7 @@
>  			newsize = 208;
>  		if (newsize > 4096)
>  			newsize = 4096;
> -		for (new2size = 128; new2size < newsize; new2size <<= 1);
> +		for (new2size = 128; new2size < newsize; new2size <<= 1)
>  			if (new2size - newsize > newsize - (new2size >> 1))
>  				new2size >>= 1;
>  		if (new2size > 4096) {

The code is correct as it originally stood.

It looks like indent has a bug and incorrectly formats this to look wrong.
Back in 2.2 times, the code looks  like this:

		for (new2size = 128; new2size < newsize; new2size <<= 1);
		if (new2size - newsize > newsize - (new2size >> 1))
			new2size >>= 1;

and this is the intended functionality.  Please do NOT apply the patch.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[John Bradford]

> > --- linux/sound/oss/vidc.c.old	2003-01-16 21:21:38.000000000 -0500
> > +++ linux/sound/oss/vidc.c	2003-02-07 02:59:44.000000000 -0500
> > @@ -225,7 +225,7 @@
> >  			newsize = 208;
> >  		if (newsize > 4096)
> >  			newsize = 4096;
> > -		for (new2size = 128; new2size < newsize; new2size <<= 1);
> > +		for (new2size = 128; new2size < newsize; new2size <<= 1)
> >  			if (new2size - newsize > newsize - (new2size >> 1))
> >  				new2size >>= 1;
> >  		if (new2size > 4096) {
> 
> The code is correct as it originally stood.
> 
> It looks like indent has a bug and incorrectly formats this to look wrong.
> Back in 2.2 times, the code looks  like this:
> 
> 		for (new2size = 128; new2size < newsize; new2size <<= 1);
> 		if (new2size - newsize > newsize - (new2size >> 1))
> 			new2size >>= 1;
> 
> and this is the intended functionality.  Please do NOT apply the patch.

I thought we were switching to negative tab indentation, anyway:

http://marc.theaimsgroup.com/?l=linux-kernel&m=104125431009832&w=2

:-)

John.

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Richard B. Johnson]

On Fri, 7 Feb 2003, John Bradford wrote:

> > > --- linux/sound/oss/vidc.c.old	2003-01-16 21:21:38.000000000 -0500
> > > +++ linux/sound/oss/vidc.c	2003-02-07 02:59:44.000000000 -0500
> > > @@ -225,7 +225,7 @@
> > >  			newsize = 208;
> > >  		if (newsize > 4096)
> > >  			newsize = 4096;
> > > -		for (new2size = 128; new2size < newsize; new2size <<= 1);
> > > +		for (new2size = 128; new2size < newsize; new2size <<= 1)
> > >  			if (new2size - newsize > newsize - (new2size >> 1))
> > >  				new2size >>= 1;
> > >  		if (new2size > 4096) {
> > 
> > The code is correct as it originally stood.
> > 
> > It looks like indent has a bug and incorrectly formats this to look wrong.
> > Back in 2.2 times, the code looks  like this:
> > 
> > 		for (new2size = 128; new2size < newsize; new2size <<= 1);
> > 		if (new2size - newsize > newsize - (new2size >> 1))
> > 			new2size >>= 1;
> > 
> > and this is the intended functionality.  Please do NOT apply the patch.
> 
> I thought we were switching to negative tab indentation, anyway:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=104125431009832&w=2
> 
> :-)
> 
> John.

If the code is correct, it really should be written as:

   for (new2size = 128; new2size < newsize; new2size <<= 1)
       ;

The code seems to want to make the value of new2size a power of
2 and, greater than 128, but less than newsize. It ignores the
fact that newsize might be less than 128, maybe this is okay.
But, the code goes on, eventually settling on new2size being
less than 4096... hmmm. I'll bet this could be greatly
simplified. I think 'new2size' is really something that will
fit inside 128-byte groups. Maybe an & or a % will greatly
simplify?


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[J.A. Magallon]

On 2003.02.07 Richard B. Johnson wrote:
> On Fri, 7 Feb 2003, John Bradford wrote:
> 
[...]
> 
>    for (new2size = 128; new2size < newsize; new2size <<= 1)
>        ;
> 
> The code seems to want to make the value of new2size a power of
> 2 and, greater than 128, but less than newsize. It ignores the
> fact that newsize might be less than 128, maybe this is okay.
> But, the code goes on, eventually settling on new2size being
> less than 4096... hmmm. I'll bet this could be greatly
> simplified. I think 'new2size' is really something that will
> fit inside 128-byte groups. Maybe an & or a % will greatly
> simplify?
> 

Isn't just a ffs or the like ?

-- 
J.A. Magallon <jamagallon@able.es>      \                 Software is like sex:
werewolf.able.es                         \           It's better when it's free
Mandrake Linux release 9.1 (Cooker) for i586
Linux 2.4.21-pre4-jam1 (gcc 3.2.1 (Mandrake Linux 9.1 3.2.1-5mdk))

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Russell King]

On Fri, Feb 07, 2003 at 10:58:17PM +0100, J.A. Magallon wrote:
> 
> On 2003.02.07 Richard B. Johnson wrote:
> > The code seems to want to make the value of new2size a power of
> > 2 and, greater than 128, but less than newsize. It ignores the
> > fact that newsize might be less than 128, maybe this is okay.

Maybe if you looked at the source, you'd have a better idea:

	if (newsize < 208)
		newsize = 208;

So, newsize will always be greater than 208, and therefore greater
than 128.  Also:

	if (newsize > 4096)
		newsize = 4096;

So it can't be larger than 4096.  Then we do this:

	for (new2size = 128; new2size < newsize; new2size <<= 1)
		;

to find a value of new2size which is a power of two.

	if (new2size - newsize > newsize - (new2size >> 1))
		new2size >>= 1;

ie, find the power of two value of new2size which is numerically
closest to newsize.  There probably is a better algorithm for
finding this.

> > But, the code goes on, eventually settling on new2size being
> > less than 4096... hmmm. I'll bet this could be greatly
> > simplified. I think 'new2size' is really something that will
> > fit inside 128-byte groups. Maybe an & or a % will greatly
> > simplify?

You're welcome to provide a solution.  Take into account that shifts
take either 1 (or 0 cycles when combined with another instruction) on
ARM, and modulus is fairly expensive, and this driver is solely for
use on ARM.

Also note that this is an OSS driver for oldish hardware, and the
above has been working fine since 2.2 days.

To be honest, I don't remember too many of the requirements of this
driver at present, (ie, why 208) so since it's known to be working,
I'd suggest leaving it be (adding a comment / reformatting that bit
of code to look less suspicious of course is acceptable.)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[John Bradford]

> >    for (new2size = 128; new2size < newsize; new2size <<= 1)
> >        ;
> > 
> > The code seems to want to make the value of new2size a power of
> > 2 and, greater than 128, but less than newsize. It ignores the
> > fact that newsize might be less than 128, maybe this is okay.
> > But, the code goes on, eventually settling on new2size being
> > less than 4096... hmmm. I'll bet this could be greatly
> > simplified. I think 'new2size' is really something that will
> > fit inside 128-byte groups. Maybe an & or a % will greatly
> > simplify?
> > 
> 
> Isn't just a ffs or the like ?

I'm just wondering whether

if (newsize > 4096) newsize = 4096;

can be removed, because:

hwrate is verified to be between 3 and 255, so 10000/hwrate will be

39 < hwrate < 3333

and ignoring the lower two bits, (& ~3),

36 < hwrate < 3332

so:

for (new2size = 128; new2size < newsize; new2size <<= 1) ;

will never cause new2size to evaluate to more than 4096 anyway.

(as far as I can see).

John.

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Alan Cox]

On Fri, 2003-02-07 at 17:13, Frank Davis wrote:
> Hello all,
>    The following patch addresses buzilla bug # 318, and removes an
> offending semicolon. Please review for inclusion

This one was ok before. The indentation was wrong thats all

> 

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Rusty Russell]

In message <Pine.LNX.4.44.0302071211390.6917-100000@master> you write:
> Hello all,
>    The following patch addresses buzilla bug # 318, and removes an
> offending semicolon. Please review for inclusion.

NAK.  This one is correct before, AFAICT.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Bill Davidsen]

On Fri, 7 Feb 2003, Richard B. Johnson wrote:

> If the code is correct, it really should be written as:
> 
>    for (new2size = 128; new2size < newsize; new2size <<= 1)
>        ;
> 
> The code seems to want to make the value of new2size a power of
> 2 and, greater than 128, but less than newsize. It ignores the
> fact that newsize might be less than 128, maybe this is okay.

Power of two - yes. Greater than 128 no, if newsize were <=128 new2size
would not increment. Previous code may make newsize larger (and later
limit new2size) but not here. And new2size can be equal to newsize, again
by this code.

A comment like /*empty_loop*/ before the semicolon would help human
readability. Code should be readable by humans, too.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Horst von Brand]

"Richard B. Johnson" <root@chaos.analogic.com> said:

[...]

> A comment like /*empty_loop*/ before the semicolon would help human
> readability. Code should be readable by humans, too.

Wrong. Readability by humans is first, can only be compromised for
extremely good reasons.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] 2.5.59 : sound/oss/vidc.c

[Alexei Podtelezhnikov]

John Bradford (john@grabjohn.com) wrote:

> 36 < hwrate < 3332
       ^^^^^^  should be 'newsize'

Yeap, and the following couple of lines:

                /* 36 < newsize 3332; rounding it off 
                 * to the nearest power of 2, no less than 256 
                 */
                for (new2size = 384; new2size < newsize; new2size <<= 1);
                new2size -= (new2size >> 1);

safely replace the whole following block:

                if (newsize < 208)
                        newsize = 208;
                if (newsize > 4096)
                        newsize = 4096;
                for (new2size = 128; new2size < newsize; new2size <<= 1);
                        if (new2size - newsize > newsize - (new2size >> 1))
                                new2size >>= 1;
                if (new2size > 4096) {
                        printk(KERN_ERR "VIDC: error: dma buffer (%d) %d > 4K\n",
                                newsize, new2size);
                        new2size = 4096;
                }

Would somebody test this?
A.