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
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.
--
** [email protected] ** 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
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 <[email protected]>
---
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;
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
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 <[email protected]>
---
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;