* [PATCH] 2.5.59 : sound/oss/vidc.c
@ 2003-02-07 17:13 Frank Davis
2003-02-07 19:43 ` Russell King
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Frank Davis @ 2003-02-07 17:13 UTC (permalink / raw)
To: linux-kernel; +Cc: fdavis, trivial
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) {
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 17:13 [PATCH] 2.5.59 : sound/oss/vidc.c Frank Davis
@ 2003-02-07 19:43 ` Russell King
2003-02-07 20:03 ` John Bradford
2003-02-07 23:54 ` Alan Cox
2003-02-10 0:23 ` Rusty Russell
2 siblings, 1 reply; 12+ messages in thread
From: Russell King @ 2003-02-07 19:43 UTC (permalink / raw)
To: Frank Davis; +Cc: linux-kernel, trivial
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 19:43 ` Russell King
@ 2003-02-07 20:03 ` John Bradford
2003-02-07 20:47 ` Richard B. Johnson
0 siblings, 1 reply; 12+ messages in thread
From: John Bradford @ 2003-02-07 20:03 UTC (permalink / raw)
To: Russell King; +Cc: fdavis, linux-kernel
> > --- 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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 20:03 ` John Bradford
@ 2003-02-07 20:47 ` Richard B. Johnson
2003-02-07 21:58 ` J.A. Magallon
2003-02-10 22:03 ` Bill Davidsen
0 siblings, 2 replies; 12+ messages in thread
From: Richard B. Johnson @ 2003-02-07 20:47 UTC (permalink / raw)
To: John Bradford; +Cc: Russell King, fdavis, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 20:47 ` Richard B. Johnson
@ 2003-02-07 21:58 ` J.A. Magallon
2003-02-07 22:20 ` Russell King
2003-02-07 22:35 ` John Bradford
2003-02-10 22:03 ` Bill Davidsen
1 sibling, 2 replies; 12+ messages in thread
From: J.A. Magallon @ 2003-02-07 21:58 UTC (permalink / raw)
To: root; +Cc: John Bradford, Russell King, fdavis, linux-kernel
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))
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 21:58 ` J.A. Magallon
@ 2003-02-07 22:20 ` Russell King
2003-02-07 22:35 ` John Bradford
1 sibling, 0 replies; 12+ messages in thread
From: Russell King @ 2003-02-07 22:20 UTC (permalink / raw)
To: J.A. Magallon; +Cc: root, John Bradford, fdavis, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 21:58 ` J.A. Magallon
2003-02-07 22:20 ` Russell King
@ 2003-02-07 22:35 ` John Bradford
1 sibling, 0 replies; 12+ messages in thread
From: John Bradford @ 2003-02-07 22:35 UTC (permalink / raw)
To: J.A. Magallon; +Cc: root, rmk, fdavis, linux-kernel
> > 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 20:47 ` Richard B. Johnson
2003-02-07 21:58 ` J.A. Magallon
@ 2003-02-10 22:03 ` Bill Davidsen
2003-02-11 7:49 ` Horst von Brand
1 sibling, 1 reply; 12+ messages in thread
From: Bill Davidsen @ 2003-02-10 22:03 UTC (permalink / raw)
To: Richard B. Johnson; +Cc: Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-10 22:03 ` Bill Davidsen
@ 2003-02-11 7:49 ` Horst von Brand
0 siblings, 0 replies; 12+ messages in thread
From: Horst von Brand @ 2003-02-11 7:49 UTC (permalink / raw)
To: Bill Davidsen; +Cc: Linux Kernel Mailing List
"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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 17:13 [PATCH] 2.5.59 : sound/oss/vidc.c Frank Davis
2003-02-07 19:43 ` Russell King
@ 2003-02-07 23:54 ` Alan Cox
2003-02-10 0:23 ` Rusty Russell
2 siblings, 0 replies; 12+ messages in thread
From: Alan Cox @ 2003-02-07 23:54 UTC (permalink / raw)
To: Frank Davis; +Cc: Linux Kernel Mailing List, trivial
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
2003-02-07 17:13 [PATCH] 2.5.59 : sound/oss/vidc.c Frank Davis
2003-02-07 19:43 ` Russell King
2003-02-07 23:54 ` Alan Cox
@ 2003-02-10 0:23 ` Rusty Russell
2 siblings, 0 replies; 12+ messages in thread
From: Rusty Russell @ 2003-02-10 0:23 UTC (permalink / raw)
To: Frank Davis; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] 2.5.59 : sound/oss/vidc.c
@ 2003-02-08 0:37 Alexei Podtelezhnikov
0 siblings, 0 replies; 12+ messages in thread
From: Alexei Podtelezhnikov @ 2003-02-08 0:37 UTC (permalink / raw)
To: john; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2003-02-11 7:43 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-02-07 17:13 [PATCH] 2.5.59 : sound/oss/vidc.c Frank Davis
2003-02-07 19:43 ` Russell King
2003-02-07 20:03 ` John Bradford
2003-02-07 20:47 ` Richard B. Johnson
2003-02-07 21:58 ` J.A. Magallon
2003-02-07 22:20 ` Russell King
2003-02-07 22:35 ` John Bradford
2003-02-10 22:03 ` Bill Davidsen
2003-02-11 7:49 ` Horst von Brand
2003-02-07 23:54 ` Alan Cox
2003-02-10 0:23 ` Rusty Russell
2003-02-08 0:37 Alexei Podtelezhnikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).