[PATCH] 8xx_io/uart.c

Hi

Found a bug in the serial driver. my_console_write() uses the wrong address in early console writes.
Removed some warnings as well.

Please can you change TX_NUM_FIFO to 8 and TX_BUF_SIZE to 96,
since chars are lost when pasting text into the console otherwise.

   Jocke

Index: arch/ppc/8xx_io/uart.c
===================================================================
RCS file: /home/cvsadmin/cvsroot/kernel/linuxppc/arch/ppc/8xx_io/uart.c,v
retrieving revision 1.3
diff -u -r1.3 uart.c

--- arch/ppc/8xx_io/uart.c	21 Nov 2002 15:16:27 -0000	1.3
+++ arch/ppc/8xx_io/uart.c	7 Feb 2003 13:23:23 -0000

@@ -2309,7 +2309,10 @@
 		/* if a LF, also do CR... */
 		if (*s == 10) {
 			while (bdp->cbd_sc & BD_SC_READY);
-			cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
+			if ((uint)(bdp->cbd_bufaddr) > (uint)IMAP_ADDR)
+				cp = (u_char *)(bdp->cbd_bufaddr);
+			else
+				cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 			*cp = 13;
 			bdp->cbd_datlen = 1;
 			bdp->cbd_sc |= BD_SC_READY;

@@ -3000,10 +3003,10 @@
 	bdp->cbd_bufaddr = iopa(mem_addr);
 	(bdp+1)->cbd_bufaddr = iopa(mem_addr+4);

-	consinfo.rx_va_base = mem_addr;
-	consinfo.rx_bd_base = bdp;
-	consinfo.tx_va_base = mem_addr + 4;
-	consinfo.tx_bd_base = bdp+1;
+	consinfo.rx_va_base = (unsigned char *)mem_addr;
+	consinfo.rx_bd_base = (cbd_t *)bdp;
+	consinfo.tx_va_base = (unsigned char *)(mem_addr + 4);
+	consinfo.tx_bd_base = (cbd_t *)(bdp+1);

 	/* For the receive, set empty and wrap.
 	 * For transmit, set wrap.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Joakim Tjernlund wrote:

Found a bug in the serial driver. my_console_write() uses the wrong address in early console writes.
Removed some warnings as well.

Well......console write doesn't get called this early.  The purpose
of these address modifications in other parts of the driver are for
kgdb using the serial port for early debugging.

I'd really like some supporting documentation (like a kernel panic
or other reproducable error) before you declare something a "bug",
along with showing the same test fixed the problem.

Please can you change TX_NUM_FIFO to 8 and TX_BUF_SIZE to 96,
since chars are lost when pasting text into the console otherwise.

What makes 96 a big enough number?  If you want to do this locally,
that's fine, but only flow control can guarantee you won't overflow
a serial port of any fifo depth.  I don't want to arbitrarily make
this fifo so large as there are other processing/latency/memory
tradeoffs.  The SMC is not a high performance interface and it
consumes lots of CPM cycles.

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Joakim Tjernlund wrote:

quoted

Found a bug in the serial driver. my_console_write() uses the wrong address in early console writes.
Removed some warnings as well.

Well......console write doesn't get called this early.  The purpose
of these address modifications in other parts of the driver are for
kgdb using the serial port for early debugging.

Early printk's will call it too.

I'd really like some supporting documentation (like a kernel panic
or other reproducable error) before you declare something a "bug",
along with showing the same test fixed the problem.

I can not reproduce an error. 3-4 times I experienced a hang after "calibrating delay loop ..."
which I can't explain.

Now, why do you do the same thing just a few lines up if it wasn't needed?

How about checking your own facts and provide an accurate analysis before
bitching at me for trying to help out?

quoted

Please can you change TX_NUM_FIFO to 8 and TX_BUF_SIZE to 96,
since chars are lost when pasting text into the console otherwise.

What makes 96 a big enough number?  If you want to do this locally,
that's fine, but only flow control can guarantee you won't overflow
a serial port of any fifo depth.  I don't want to arbitrarily make
this fifo so large as there are other processing/latency/memory
tradeoffs.  The SMC is not a high performance interface and it
consumes lots of CPM cycles.

I tested my how much I had increase them until I didn't lose any chars.
I don't think this has anything to do with HW flow control. The
smc runs out of BD's when trying to echo back the chars.

I don't see how this would consume more CPM cycles. Yes, it will use
a little more dpram but there is room for it.

 Jocke


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Joakim Tjernlund wrote:

Early printk's will call it too.

They shouldn't.  All printk's prior to serial device initialization
are queued in the log buffer.  After the serial device is initialized,
the debug console is configured and only at that time will the printk
information flow through the driver.  Anything queued up to that point
is blasted out the driver.  There are lots of console drivers that
will fail if Linux chooses to change this behavior.

Now, why do you do the same thing just a few lines up if it wasn't needed?

It is needed there.  These same functions support xmon and kgdb.  In order
to do that the serial port is used in three different phases.  One, as it
was configured by the boot rom/loader, two, by an early serial initialization
before general memory management and buffering is available, and finally
after the driver has been completely initialized.  None of this is needed
to simply support the Linux console.  This has all been discussed too many
times before....

How about checking your own facts and provide an accurate analysis before
bitching at me for trying to help out?

All I asked is you somehow show it is broken, and that something has
been fixed before we start patching things.  If there is really something
wrong with it, I would like to know what that is and how it is triggered.

I tested my how much I had increase them until I didn't lose any chars.
I don't think this has anything to do with HW flow control. The
smc runs out of BD's when trying to echo back the chars.

Then, someone upstream isn't watching return values from these functions
when the driver indicates there isn't space to output the characters.
It isn't the responsibility of the serial driver to provide sufficient
queueing for an arbitrary output request.  The depth of the fifo on the 8xx
is much deeper than most silicon uarts, and if that isn't enough I suspect
something outside of this driver is amiss (or interfaces have changed and
this driver wasn't updated).

Thanks.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Joakim Tjernlund wrote:

quoted

Early printk's will call it too.

They shouldn't.  All printk's prior to serial device initialization
are queued in the log buffer.  After the serial device is initialized,
the debug console is configured and only at that time will the printk
information flow through the driver.  Anything queued up to that point
is blasted out the driver.  There are lots of console drivers that
will fail if Linux chooses to change this behavior.

Still the first half of the printk's has the if expression true, then it switch
to false for the remaing one's and stayed there during boot up.
This is how I tested it:
remove the *cp = 13 line and add:
if ((uint)(bdp->cbd_bufaddr) > (uint)IMAP_ADDR){
   cp = (u_char *)(bdp->cbd_bufaddr);
   *cp = 64;
} else {
   cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
  *cp = 65;
}
Then just watch the a normal boot.

quoted

Now, why do you do the same thing just a few lines up if it wasn't needed?

It is needed there.  These same functions support xmon and kgdb.  In order
to do that the serial port is used in three different phases.  One, as it
was configured by the boot rom/loader, two, by an early serial initialization
before general memory management and buffering is available, and finally
after the driver has been completely initialized.  None of this is needed
to simply support the Linux console.  This has all been discussed too many
times before....

I don't buy that it is needed only in one place but not the other.
Either none of them is needed or both. After a little investigation it looks like
none of them is needed, but I need to test that first to be sure.

These comments are also in there:
/*
 * We need to gracefully shut down the transmitter, disable
 * interrupts, then send our bytes out.
 */
and
/*
 * Finally, Wait for transmitter & holding register to empty
 *  and restore the IER
 */
and yet there is no sign of trying to do any of that.
Once user space gets going won't printk's from the kernel risk confusing
the real serial driver since interrupts are active?
This driver is not very easy to understand.

Also, the last 'if(info)' is rendundant. 'info' will never be NULL.

quoted

How about checking your own facts and provide an accurate analysis before
bitching at me for trying to help out?

All I asked is you somehow show it is broken, and that something has
been fixed before we start patching things.  If there is really something
wrong with it, I would like to know what that is and how it is triggered.

OK, I was a little harsh, but your answer wasn't helping me much

quoted

I tested my how much I had increase them until I didn't lose any chars.
I don't think this has anything to do with HW flow control. The
smc runs out of BD's when trying to echo back the chars.

Then, someone upstream isn't watching return values from these functions
when the driver indicates there isn't space to output the characters.
It isn't the responsibility of the serial driver to provide sufficient
queueing for an arbitrary output request.  The depth of the fifo on the 8xx
is much deeper than most silicon uarts, and if that isn't enough I suspect
something outside of this driver is amiss (or interfaces have changed and
this driver wasn't updated).

Yes, Paul Ruhland has found a bug in drivers/char/n_tty.c that explains
some of the problems I have seen.

   Jocke
PS.
    Could you please also comment on the enet.c driver patch I sent.

Thanks.

-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

[mailto:owner-linuxppc-embedded@lists.linuxppc.org]On Behalf Of Dan Malek
Joakim Tjernlund wrote:

quoted

Please can you change TX_NUM_FIFO to 8 and TX_BUF_SIZE to 96,
since chars are lost when pasting text into the console otherwise.

What makes 96 a big enough number?  If you want to do this locally,
that's fine, but only flow control can guarantee you won't overflow
a serial port of any fifo depth.

The fix I posted for drivers/char/n_tty.c is buggy w.r.t looking and
noone wants to play with the tty layer since it is a mess.

So I have been looking into what can be done in the uart.c.
opost_block(in n_tty.c) will never write more that 80 chars in
one go so it makes sense if we can match that size(TX_BUF_SIZE >= 80).
That should not be a problem since this memory is allocated with
m8xx_cpm_hostalloc()and the only user that function is uart.c

Increasing TX_NUM_FIFO to 8 is not big deal either IMHO.
Possibly one could let serial_console_setup allocate all the
BDs that is needed and make rs_8xx_init() reuse them. That
will save 24 bytes(3 BDs) from dp memory. This may be tricky though.

Another thing that will help is to make rs_8xx_put_char() build
a transmit queue and impl. rs_8xx_flush_chars() to actually write
them. I have tested this and it helps but it won't solve all cases.

What do you think?

I don't want to arbitrarily make
this fifo so large as there are other processing/latency/memory
tradeoffs.  The SMC is not a high performance interface and it
consumes lots of CPM cycles.

  I can't see the processing and latency implications.

   Jocke


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/