drivers/char/specialix.c: broken baud conversion

drivers/char/specialix.c: broken baud conversion

[Adrian Bunk]

Hi Alan,

your commit commit 67cc0161ecc9ebee6eba4af6cbfdba028090b1b9
"specialix - remove private speed decoding" converted the variable baud 
from an index in the array baud_table[] to containing the baud value 
itself.

Unfortunately, it contains at least two bugs:


The Coverity checker spotted that the following line was forgotten:

           baud = (baud_table[baud] + 5) / 10;   /* Estimated CPS */

BTW: After the trivial fix, baud_table[] could be removed.


While looking at the patch, I noticed it contains another bug that is 
not that easy to fix:

-       if (baud == 15) {
+       if (baud == 38400) {
                if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
                        baud ++;
                if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
                        baud += 2;
        }

Increasing the index for baud_table[] by 1 or 2 is quite different from 
increasing baud by 1 or 2.


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: drivers/char/specialix.c: broken baud conversion

[Rogier Wolff]

On Mon, Oct 09, 2006 at 12:18:19AM +0200, Adrian Bunk wrote:
> +       if (baud == 38400) {
>                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
>                         baud ++;
>                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
>                         baud += 2;
>         }
> 
> Increasing the index for baud_table[] by 1 or 2 is quite different from 
> increasing baud by 1 or 2.

In that range, 
	baud <<= 1; 
and
	baud <<= 2; 

should work. 

	Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ

[2.6.19 patch] drivers/char/specialix.c: fix the baud conversion

[Adrian Bunk]

On Mon, Oct 09, 2006 at 08:37:45AM +0200, Rogier Wolff wrote:
> On Mon, Oct 09, 2006 at 12:18:19AM +0200, Adrian Bunk wrote:
> > +       if (baud == 38400) {
> >                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> >                         baud ++;
> >                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> >                         baud += 2;
> >         }
> > 
> > Increasing the index for baud_table[] by 1 or 2 is quite different from 
> > increasing baud by 1 or 2.
> 
> In that range, 
> 	baud <<= 1; 
> and
> 	baud <<= 2; 
> 
> should work. 

Thanks for the hint.

What about the patch below?

> 	Roger. 

cu
Adrian


<--  snip  -->


This patch corrects the following bugs introduced by
commit 67cc0161ecc9ebee6eba4af6cbfdba028090b1b9:
- remove one remaining and now incorrect baud_table[] usage
- "baud +=" must become "baud <<="

The former bug was spotted by the Coverity checker.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

 drivers/char/specialix.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

--- linux-2.6/drivers/char/specialix.c.old	2006-10-10 08:04:48.000000000 +0200
+++ linux-2.6/drivers/char/specialix.c	2006-10-10 08:06:25.000000000 +0200
@@ -183,11 +183,6 @@
 
 static struct tty_driver *specialix_driver;
 
-static unsigned long baud_table[] =  {
-	0, 50, 75, 110, 134, 150, 200, 300, 600, 1200, 1800, 2400, 4800,
-	9600, 19200, 38400, 57600, 115200, 0,
-};
-
 static struct specialix_board sx_board[SX_NBOARD] =  {
 	{ 0, SX_IOBASE1,  9, },
 	{ 0, SX_IOBASE2, 11, },
@@ -1090,9 +1085,9 @@
 
 	if (baud == 38400) {
 		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
-			baud ++;
+			baud <<= 1;
 		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
-			baud += 2;
+			baud <<= 2;
 	}
 
 	if (!baud) {
@@ -1150,11 +1145,9 @@
 	sx_out(bp, CD186x_RBPRL, tmp & 0xff);
 	sx_out(bp, CD186x_TBPRL, tmp & 0xff);
 	spin_unlock_irqrestore(&bp->lock, flags);
-	if (port->custom_divisor) {
+	if (port->custom_divisor)
 		baud = (SX_OSCFREQ + port->custom_divisor/2) / port->custom_divisor;
-		baud = ( baud + 5 ) / 10;
-	} else
-		baud = (baud_table[baud] + 5) / 10;   /* Estimated CPS */
+	baud = (baud + 5) / 10;
 
 	/* Two timer ticks seems enough to wakeup something like SLIP driver */
 	tmp = ((baud + HZ/2) / HZ) * 2 - CD186x_NFIFO;

Re: [2.6.19 patch] drivers/char/specialix.c: fix the baud conversion

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

Adrian Bunk wrote:
> On Mon, Oct 09, 2006 at 08:37:45AM +0200, Rogier Wolff wrote:
> > On Mon, Oct 09, 2006 at 12:18:19AM +0200, Adrian Bunk wrote:
> > > +       if (baud == 38400) {
> > >                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> > >                         baud ++;
> > >                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> > >                         baud += 2;
> > >         }
> > >
> > > Increasing the index for baud_table[] by 1 or 2 is quite different from
> > > increasing baud by 1 or 2.
> >
> > In that range,
> > 	baud <<= 1;
> > and
> > 	baud <<= 2;
> >
> > should work.
>
> Thanks for the hint.
>
> What about the patch below?

> @@ -1090,9 +1085,9 @@
>
>  	if (baud == 38400) {
>  		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> -			baud ++;
> +			baud <<= 1;
>  		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> -			baud += 2;
> +			baud <<= 2;
>  	}
>
>  	if (!baud) {

Neither is 38400 <<= 1 == 57600 nor is 38400 <<= 2 == 115200. You should just 
set baud to the value you want instead of doing tricks here.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[2.6.19 patch] drivers/char/specialix.c: fix the baud conversion

[Adrian Bunk]

On Tue, Oct 10, 2006 at 02:01:19PM +0200, Rolf Eike Beer wrote:
> Adrian Bunk wrote:
> > On Mon, Oct 09, 2006 at 08:37:45AM +0200, Rogier Wolff wrote:
> > > On Mon, Oct 09, 2006 at 12:18:19AM +0200, Adrian Bunk wrote:
> > > > +       if (baud == 38400) {
> > > >                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> > > >                         baud ++;
> > > >                 if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> > > >                         baud += 2;
> > > >         }
> > > >
> > > > Increasing the index for baud_table[] by 1 or 2 is quite different from
> > > > increasing baud by 1 or 2.
> > >
> > > In that range,
> > > 	baud <<= 1;
> > > and
> > > 	baud <<= 2;
> > >
> > > should work.
> >
> > Thanks for the hint.
> >
> > What about the patch below?
> 
> > @@ -1090,9 +1085,9 @@
> >
> >  	if (baud == 38400) {
> >  		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
> > -			baud ++;
> > +			baud <<= 1;
> >  		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
> > -			baud += 2;
> > +			baud <<= 2;
> >  	}
> >
> >  	if (!baud) {
> 
> Neither is 38400 <<= 1 == 57600 nor is 38400 <<= 2 == 115200. You should just 
> set baud to the value you want instead of doing tricks here.

Damn, I should have checked the numbers myself...  :-(

Thanks for the correction, an updated patch is below.

> Eike

cu
Adrian


<--  snip  -->


This patch corrects the following bugs introduced by
commit 67cc0161ecc9ebee6eba4af6cbfdba028090b1b9:
- remove one remaining and now incorrect baud_table[] usage
- "baud +=" is no longer correct

The former bug was spotted by the Coverity checker.

Rolf Eike Beer spotted a bug in the initial version of my patch.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

 drivers/char/specialix.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

--- linux-2.6/drivers/char/specialix.c.old	2006-10-11 06:35:44.000000000 +0200
+++ linux-2.6/drivers/char/specialix.c	2006-10-11 06:36:52.000000000 +0200
@@ -183,11 +183,6 @@
 
 static struct tty_driver *specialix_driver;
 
-static unsigned long baud_table[] =  {
-	0, 50, 75, 110, 134, 150, 200, 300, 600, 1200, 1800, 2400, 4800,
-	9600, 19200, 38400, 57600, 115200, 0,
-};
-
 static struct specialix_board sx_board[SX_NBOARD] =  {
 	{ 0, SX_IOBASE1,  9, },
 	{ 0, SX_IOBASE2, 11, },
@@ -1090,9 +1085,9 @@
 
 	if (baud == 38400) {
 		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_HI)
-			baud ++;
+			baud = 57600;
 		if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_VHI)
-			baud += 2;
+			baud = 115200;
 	}
 
 	if (!baud) {
@@ -1150,11 +1145,9 @@
 	sx_out(bp, CD186x_RBPRL, tmp & 0xff);
 	sx_out(bp, CD186x_TBPRL, tmp & 0xff);
 	spin_unlock_irqrestore(&bp->lock, flags);
-	if (port->custom_divisor) {
+	if (port->custom_divisor)
 		baud = (SX_OSCFREQ + port->custom_divisor/2) / port->custom_divisor;
-		baud = ( baud + 5 ) / 10;
-	} else
-		baud = (baud_table[baud] + 5) / 10;   /* Estimated CPS */
+	baud = (baud + 5) / 10;		/* Estimated CPS */
 
 	/* Two timer ticks seems enough to wakeup something like SLIP driver */
 	tmp = ((baud + HZ/2) / HZ) * 2 - CD186x_NFIFO;