RE: Problem of concurrency in arch/ppc/8260_io/uart.c

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Jean-Denis Boyer]

I have just tested your patch on 8260 and it fixes the problem when printk is called from interrupt context while data is printed from user mode application. GREAT!

I also ported the patch to 860 (as said earlier, code is almost the same), but it hung at boot up, just after printing "Calibrating delay loop..." :-(

I remember I had such problems in the past (on 860), when calling "spin_lock_irqsave/restore" from interrupt context. If I use function "in_interrupt()" to avoid calling them from interrupt, everything works fine, and the bug is effectively fixed.

I just don't know where is the problem??? It looks like there is a bug on 860 using "local_irq_save/restore" in interrupt context?!?

I'm working with 2.4.19.

--------------------------------------------
 Jean-Denis Boyer, Eng.
 Software Designer
 M5T Centre d'Excellence en Télécom Inc.
 4283 Garlock Street
 Sherbrooke (Québec)
 J1L 2C8  CANADA
 (819)829-3972 x241
--------------------------------------------


> -----Original Message-----
> From: Steffen Rumler [mailto:Steffen.Rumler@siemens.com]
> Sent: 15 septembre, 2003 06:18
> To: linuxppc
> Subject: Re: Problem of concurrency in arch/ppc/8260_io/uart.c
>
>
> Hi,
>
> I have seen a similar problem for the 2.4.20.
>
> When I force a lot of console output via the following command:
>
>    while true; do cd /; ls -R; done
>
> and type-in some letters in parallel, the console
> becomes crazy.
>
> I have added some instrumentation in order to dump the
> TX Buffer Descriptor Table. I have found that the
> hardware pointer (TBPTR) and the software pointer (tx_cur)
> are not more synchronized together:
>
>  >> make new rlogin session <<
> /root# cd /proc/driver
>
> /root# cat uart-bdtables; cat mpc82xx/smc1_pram | grep SMC1_PRAM_TBPTR
>
> TX BD table
>    (000 at 0xfff005f0) status: 0x1000 len: 0001 addr: 0x001bb084
>    (001 at 0xfff005f8) status: 0x1000 len: 0001 addr: 0x001bb0a4
> * (002 at 0xfff00600) status: 0x1000 len: 0001 addr: 0x001bb0c4
>    (003 at 0xfff00608) status: 0x3000 len: 0004 addr: 0x001bb0e4
>     SMC1_PRAM_TBPTR            0x20         2     0600
>
>     --> hardware and software pointer still synchronized
>
>     >> force console to become crazy (see above) <<
>
> /root# cat uart-bdtables; cat mpc82xx/smc1_pram | grep SMC1_PRAM_TBPTR
>
> TX BD table (tbptr: 0x00000088)
>    (000 at 0xfff005f0) status: 0x1000 len: 0003 addr: 0x001bb084
> * (001 at 0xfff005f8) status: 0x1000 len: 0021 addr: 0x001bb0a4
>    (002 at 0xfff00600) status: 0x1000 len: 0001 addr: 0x001bb0c4
>    (003 at 0xfff00608) status: 0x3000 len: 0001 addr: 0x001bb0e4
>     SMC1_PRAM_TBPTR            0x20         2     0600
>
>     --> hardware and software pointer NOT more synchronized
>
>     >> make additional console output: echo foo >/dev/console <<
>
> /root# cat uart-bdtables; cat mpc82xx/smc1_pram | grep SMC1_PRAM_TBPTR
>
> * (000 at 0xfff005f0) status: 0x1000 len: 0003 addr: 0x001bb084
>    (001 at 0xfff005f8) status: 0x9000 len: 0004 addr: 0x001bb0a4
>    (002 at 0xfff00600) status: 0x1000 len: 0001 addr: 0x001bb0c4
>    (003 at 0xfff00608) status: 0x3000 len: 0001 addr: 0x001bb0e4
>     SMC1_PRAM_TBPTR            0x20         2     05f0
>
>     --> hardware pointer hangs at 0x5f0 because R-Bit not set, but
>         at 0x5f8
>
> Inside uart.c, there are the following output routines:
>
>    rs_8xx_put_char()
>    rs_8xx_write()
>    rs_8xx_send_xchar()
>    my_console_write()
>
> I think there must be a synchronization accessing the
> TX BD table. I suggest the patch attached.
>
>
> Best Regards
> Steffen
> --
>
>
> --------------------------------------------------------------
>
> Steffen Rumler
> ICN CP D NT SW 7
> Siemens AG
> Hofmannstr. 51                 Email: Steffen.Rumler@siemens.com
> D-81359 Munich                 Phone: +49 89 722-44061
> Germany                        Fax  : +49 89 722-36703
>
> --------------------------------------------------------------
>
>
>

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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> I have just tested your patch on 8260 and it fixes the problem when printk is called from interrupt context while data is
> printed from user mode application. GREAT!
>
> I also ported the patch to 860 (as said earlier, code is almost the same), but it hung at boot up, just after printing
> "Calibrating delay loop..." :-(
>
> I remember I had such problems in the past (on 860), when calling "spin_lock_irqsave/restore" from interrupt context. If
> I use function "in_interrupt()" to avoid calling them from interrupt, everything works fine, and the bug is effectively fixed.
>
> I just don't know where is the problem??? It looks like there is a bug on 860 using "local_irq_save/restore" in interrupt
> context?!?

The patch is a little broken w.r.t my_console_write, the last "if(info)" should be a
"if(ser->info)". Don't know why it works on 82xx as is, it should not.

However, this patch also adds lots of IRQ latency. I think the idea is right, but one must
reduce time with IRQ's off. It looks easy for all functions, but not for my_console_write()

    Jocke

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dan Malek]

Jean-Denis Boyer wrote:

> I have just tested your patch on 8260 and it fixes the problem when printk is called ...

The use of printk is strictly a debug function.  If it is necessary to see
it print out pretty, then use a different I/O path for the application or
dump out the log buffer separately.  Serial ports don't guarantee any
transactional operation with multiple users, printk isn't any different
although it does the bad thing of blocking interrupts while it processes
the message.

Adding this patch adds lots of interrupt latency and I would recommend
against it.  It appears there is a bug that should be fixed to ensure
the proper synchronization of the BDs, but not with such large scale
blocking of interrupts.

Thanks.


	-- Dan


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> Jean-Denis Boyer wrote:
>
> > I have just tested your patch on 8260 and it fixes the problem when printk is called ...
>
> The use of printk is strictly a debug function.  If it is necessary to see
> it print out pretty, then use a different I/O path for the application or
> dump out the log buffer separately.  Serial ports don't guarantee any
> transactional operation with multiple users, printk isn't any different
> although it does the bad thing of blocking interrupts while it processes
> the message.
hmm, it does not block interrupts as is, does it? Only if you apply the patch?

>
> Adding this patch adds lots of interrupt latency and I would recommend
> against it.  It appears there is a bug that should be fixed to ensure
> the proper synchronization of the BDs, but not with such large scale
> blocking of interrupts.
Agreed, but is there another mechanism but to disable interrupts? I can't think of any.
It is possible to shrink the window with interrupts off dramatically.

   Jocke
>
> Thanks.
>
>
> -- Dan


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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > Jean-Denis Boyer wrote:
> >
> > > I have just tested your patch on 8260 and it fixes the problem when printk is called ...
> >
> > The use of printk is strictly a debug function.  If it is necessary to see
> > it print out pretty, then use a different I/O path for the application or
> > dump out the log buffer separately.  Serial ports don't guarantee any
> > transactional operation with multiple users, printk isn't any different
> > although it does the bad thing of blocking interrupts while it processes
> > the message.
> hmm, it does not block interrupts as is, does it? Only if you apply the patch?
>
> >
> > Adding this patch adds lots of interrupt latency and I would recommend
> > against it.  It appears there is a bug that should be fixed to ensure
> > the proper synchronization of the BDs, but not with such large scale
> > blocking of interrupts.
> Agreed, but is there another mechanism but to disable interrupts? I can't think of any.
> It is possible to shrink the window with interrupts off dramatically.
>
>    Jocke
> >
> > Thanks.
> >
> >
> > -- Dan

OK, here is my attempt to fix the 8xx uart driver. With this
patch I can no longer break the console from user space. Against fairly recent
linuxppc_devel snapshot. Please comment/apply.

Summary:

 - uses local_irq_save()/local_irq_restore() around tx_cur accesses.
   I haved tried to minimize the time between local_irq_save() and local_irq_restore()

 - Fixed a bug around copy_from_user() in rs_8xx_write().

 - my_console_write() is still buggy. It takes a little more effort to fix
   this function. It should be fixed since a spurious printk can screw/lock your console.

 - fixed 4 trivial warnings as well.

  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.7
diff -u -r1.7 uart.c
--- arch/ppc/8xx_io/uart.c	13 Jun 2003 09:04:43 -0000	1.7
+++ arch/ppc/8xx_io/uart.c	16 Sep 2003 12:17:01 -0000
@@ -1068,6 +1068,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t	*bdp;
 	unsigned char *cp;
+	unsigned long flags;

 	if (serial_paranoia_check(info, tty->device, "rs_put_char"))
 		return;
@@ -1075,7 +1076,16 @@
 	if (!tty)
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
+
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
@@ -1083,14 +1093,6 @@
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;

-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 static int rs_8xx_write(struct tty_struct * tty, int from_user,
@@ -1100,6 +1102,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t *bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 #ifdef CONFIG_KGDB_CONSOLE
         /* Try to let stub handle output. Returns true if it did. */
@@ -1113,44 +1116,46 @@
 	if (!tty)
 		return 0;

-	bdp = info->tx_cur;
-
 	while (1) {
 		c = MIN(count, TX_BUF_SIZE);

 		if (c <= 0)
 			break;

+		local_irq_save(flags);
+		bdp = info->tx_cur;
 		if (bdp->cbd_sc & BD_SC_READY) {
 			info->flags |= TX_WAKEUP;
+			local_irq_restore(flags);
 			break;
 		}

 		cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 		if (from_user) {
-			if (copy_from_user((void *)cp, buf, c)) {
+			c -= copy_from_user((void *)cp, buf, c);
+			if (!c) {
 				if (!ret)
 					ret = -EFAULT;
+				local_irq_restore(flags);
 				break;
 			}
 		} else {
 			memcpy((void *)cp, buf, c);
 		}
-
+		/* Get next BD.
+		 */
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = info->tx_bd_base;
+		else
+			info->tx_cur = (cbd_t *)bdp + 1;
+		local_irq_restore(flags);
+
 		bdp->cbd_datlen = c;
 		bdp->cbd_sc |= BD_SC_READY;

 		buf += c;
 		count -= c;
 		ret += c;
-
-		/* Get next BD.
-		*/
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = info->tx_bd_base;
-		else
-			bdp++;
-		info->tx_cur = (cbd_t *)bdp;
 	}
 	return ret;
 }
@@ -1210,28 +1215,28 @@
 {
 	volatile cbd_t	*bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 	ser_info_t *info = (ser_info_t *)tty->driver_data;

 	if (serial_paranoia_check(info, tty->device, "rs_send_char"))
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 	*cp = ch;
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;
-
-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 /*
@@ -1802,7 +1807,7 @@
 static void rs_8xx_wait_until_sent(struct tty_struct *tty, int timeout)
 {
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
-	unsigned long orig_jiffies, char_time;
+	unsigned long orig_jiffies, char_time, tst_res;
 	/*int lsr;*/
 	volatile cbd_t *bdp;

@@ -1837,6 +1842,7 @@
 	 * are empty.
 	 */
 	do {
+		unsigned long flags;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 		printk("lsr = %d (jiff=%lu)...", lsr, jiffies);
 #endif
@@ -1854,12 +1860,15 @@
 		 * is the buffer is available.  There are still characters
 		 * in the CPM FIFO.
 		 */
+		local_irq_save(flags);
 		bdp = info->tx_cur;
 		if (bdp == info->tx_bd_base)
 			bdp += (TX_NUM_FIFO-1);
 		else
 			bdp--;
-	} while (bdp->cbd_sc & BD_SC_READY);
+		tst_res = !!(bdp->cbd_sc & BD_SC_READY);
+		local_irq_restore(flags);
+	} while (tst_res);
 	current->state = TASK_RUNNING;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
@@ -3073,10 +3082,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/

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> OK, here is my attempt to fix the 8xx uart driver. With this
> patch I can no longer break the console from user space. Against fairly recent
> linuxppc_devel snapshot. Please comment/apply.
>
> Summary:
>
>  - uses local_irq_save()/local_irq_restore() around tx_cur accesses.
>    I haved tried to minimize the time between local_irq_save() and local_irq_restore()
>
>  - Fixed a bug around copy_from_user() in rs_8xx_write().
>
>  - my_console_write() is still buggy. It takes a little more effort to fix
>    this function. It should be fixed since a spurious printk can screw/lock your console.

I am taking a stab at my_console_write() as well. I have something that works well
for me, but i am unsure if I have broken KGDB/XMON support. In my new my_console_write()
I must know the length of the TX buffer in the TXBD. The TX buffer length when the console
is fully operational is TX_BUF_SIZE. Before that, in serial_console_setup, the length
is 4 bytes. But what is the TX buffer length before serial_console_setup has run(that's
when KGB/XMON is active)?

 Jocke

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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > OK, here is my attempt to fix the 8xx uart driver. With this
> > patch I can no longer break the console from user space. Against fairly recent
> > linuxppc_devel snapshot. Please comment/apply.
> >
> > Summary:
> >
> >  - uses local_irq_save()/local_irq_restore() around tx_cur accesses.
> >    I haved tried to minimize the time between local_irq_save() and local_irq_restore()
> >
> >  - Fixed a bug around copy_from_user() in rs_8xx_write().
> >
> >  - my_console_write() is still buggy. It takes a little more effort to fix
> >    this function. It should be fixed since a spurious printk can screw/lock your console.
>
> I am taking a stab at my_console_write() as well. I have something that works well
> for me, but i am unsure if I have broken KGDB/XMON support. In my new my_console_write()
> I must know the length of the TX buffer in the TXBD. The TX buffer length when the console
> is fully operational is TX_BUF_SIZE. Before that, in serial_console_setup, the length
> is 4 bytes. But what is the TX buffer length before serial_console_setup has run(that's
> when KGB/XMON is active)?
>
>  Jocke

Here is a version that fixes my_console_write as well. The KGDB/XMON question above is still open.
Currently TX buffer length assumption is >=4
Anyone intressted?

 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.7
diff -u -r1.7 uart.c
--- arch/ppc/8xx_io/uart.c	13 Jun 2003 09:04:43 -0000	1.7
+++ arch/ppc/8xx_io/uart.c	17 Sep 2003 13:33:47 -0000
@@ -1068,6 +1068,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t	*bdp;
 	unsigned char *cp;
+	unsigned long flags;

 	if (serial_paranoia_check(info, tty->device, "rs_put_char"))
 		return;
@@ -1075,7 +1076,16 @@
 	if (!tty)
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
+
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
@@ -1083,14 +1093,6 @@
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;

-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 static int rs_8xx_write(struct tty_struct * tty, int from_user,
@@ -1100,6 +1102,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t *bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 #ifdef CONFIG_KGDB_CONSOLE
         /* Try to let stub handle output. Returns true if it did. */
@@ -1113,44 +1116,46 @@
 	if (!tty)
 		return 0;

-	bdp = info->tx_cur;
-
 	while (1) {
 		c = MIN(count, TX_BUF_SIZE);

 		if (c <= 0)
 			break;

+		local_irq_save(flags);
+		bdp = info->tx_cur;
 		if (bdp->cbd_sc & BD_SC_READY) {
 			info->flags |= TX_WAKEUP;
+			local_irq_restore(flags);
 			break;
 		}

 		cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 		if (from_user) {
-			if (copy_from_user((void *)cp, buf, c)) {
+			c -= copy_from_user((void *)cp, buf, c);
+			if (!c) {
 				if (!ret)
 					ret = -EFAULT;
+				local_irq_restore(flags);
 				break;
 			}
 		} else {
 			memcpy((void *)cp, buf, c);
 		}
-
+		/* Get next BD.
+		 */
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = info->tx_bd_base;
+		else
+			info->tx_cur = (cbd_t *)bdp + 1;
+		local_irq_restore(flags);
+
 		bdp->cbd_datlen = c;
 		bdp->cbd_sc |= BD_SC_READY;

 		buf += c;
 		count -= c;
 		ret += c;
-
-		/* Get next BD.
-		*/
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = info->tx_bd_base;
-		else
-			bdp++;
-		info->tx_cur = (cbd_t *)bdp;
 	}
 	return ret;
 }
@@ -1210,28 +1215,28 @@
 {
 	volatile cbd_t	*bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 	ser_info_t *info = (ser_info_t *)tty->driver_data;

 	if (serial_paranoia_check(info, tty->device, "rs_send_char"))
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 	*cp = ch;
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;
-
-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 /*
@@ -1802,7 +1807,7 @@
 static void rs_8xx_wait_until_sent(struct tty_struct *tty, int timeout)
 {
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
-	unsigned long orig_jiffies, char_time;
+	unsigned long orig_jiffies, char_time, tst_res;
 	/*int lsr;*/
 	volatile cbd_t *bdp;

@@ -1837,6 +1842,7 @@
 	 * are empty.
 	 */
 	do {
+		unsigned long flags;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 		printk("lsr = %d (jiff=%lu)...", lsr, jiffies);
 #endif
@@ -1854,12 +1860,15 @@
 		 * is the buffer is available.  There are still characters
 		 * in the CPM FIFO.
 		 */
+		local_irq_save(flags);
 		bdp = info->tx_cur;
 		if (bdp == info->tx_bd_base)
 			bdp += (TX_NUM_FIFO-1);
 		else
 			bdp--;
-	} while (bdp->cbd_sc & BD_SC_READY);
+		tst_res = !!(bdp->cbd_sc & BD_SC_READY);
+		local_irq_restore(flags);
+	} while (tst_res);
 	current->state = TASK_RUNNING;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
@@ -2261,10 +2270,11 @@
 {
 	struct		serial_state	*ser;
 	ser_info_t			*info;
-	unsigned			i;
+	unsigned			i, c, cr_missing, max_tx_size;
 	volatile	cbd_t		*bdp, *bdbase;
 	volatile	smc_uart_t	*up;
 	volatile	u_char		*cp;
+	unsigned long                   flags;

 	ser = rs_table + idx;

@@ -2273,40 +2283,41 @@
 	 * we simply use the single buffer allocated.
 	 */
 	if ((info = (ser_info_t *)ser->info) != NULL) {
-		bdp = info->tx_cur;
 		bdbase = info->tx_bd_base;
-	}
-	else {
-		/* Pointer to UART in parameter ram.
-		*/
+		max_tx_size = TX_BUF_SIZE;
+	} else {
+		/* Pointer to UART in parameter ram. */
 		up = (smc_uart_t *)&cpmp->cp_dparam[ser->port];
-
-		/* Get the address of the host memory buffer.
-		 */
-		bdp = bdbase = (cbd_t *)&cpmp->cp_dpmem[up->smc_tbase];

+		/* Get the address of the host memory buffer.*/
 		info = &consinfo;
+		info->tx_bd_base = (cbd_t *)bdbase = (cbd_t *)&cpmp->cp_dpmem[up->smc_tbase];
+		info->tx_cur = (cbd_t *)bdbase;
+		max_tx_size = 4; /* Early serial acces has this */
 	}

-	/*
-	 * We need to gracefully shut down the transmitter, disable
-	 * interrupts, then send our bytes out.
-	 */
+	cr_missing = 0;
+	while (1){
+		c = MIN(max_tx_size, count);
+		if (c <= 0)
+			break;

-	/*
-	 * Now, do each character.  This is not as bad as it looks
-	 * since this is a holding FIFO and not a transmitting FIFO.
-	 * We could add the complexity of filling the entire transmit
-	 * buffer, but we would just wait longer between accesses......
-	 */
-	for (i = 0; i < count; i++, s++) {
+		local_irq_save(flags);
+		bdp = info->tx_cur;
+		bdbase = info->tx_bd_base;
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = (cbd_t *)bdbase;
+		else
+			info->tx_cur = (cbd_t *)(bdp+1);
+		local_irq_restore(flags);
+
 		/* Wait for transmitter fifo to empty.
 		 * Ready indicates output is ready, and xmt is doing
 		 * that, not that it is ready for us to send.
 		 */
 		while (bdp->cbd_sc & BD_SC_READY);

-		/* Send the character out.
+		/* Send the characters out.
 		 * If the buffer address is in the CPM DPRAM, don't
 		 * convert it.
 		 */
@@ -2314,55 +2325,32 @@
 			cp = (u_char *)(bdp->cbd_bufaddr);
 		else
 			cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
-		*cp = *s;
-
-		bdp->cbd_datlen = 1;
-		bdp->cbd_sc |= BD_SC_READY;
-
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = bdbase;
-		else
-			bdp++;
-
-		/* if a LF, also do CR... */
-		if (*s == 10) {
-			while (bdp->cbd_sc & BD_SC_READY);
-
-			/* This 'if' below will never be true, but a few
-			 * people argued with me that it was a "bug by
-			 * inspection" that is was easier to add the code
-			 * than continue the discussion.  The only time
-			 * the buffer address is in DPRAM is during early
-			 * use by kgdb/xmon, which format their own packets
-			 * and we never get here. -- Dan
-			 */
-			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;

-			if (bdp->cbd_sc & BD_SC_WRAP) {
-				bdp = bdbase;
-			}
-			else {
-				bdp++;
-			}
+		i=1; /* Keeps track of consumed TX buffer space */
+		if (cr_missing) {
+			/* Previus loop didn't have room for the CR, insert it first in this */
+			*cp++ = '\r';
+			i++;
 		}
-	}
-
-	/*
-	 * Finally, Wait for transmitter & holding register to empty
-	 *  and restore the IER
-	 */
-	while (bdp->cbd_sc & BD_SC_READY);
+		cr_missing = 0;
+		for (; i <= c; i++) {
+			if ((*cp++ = *s++) != '\n')
+				continue; /* Copy bytes until a NewLine is found */
+			/* NewLine found, see if there is space in the TX buffer to add a CR */
+			if (i < max_tx_size) {
+				*cp++ = '\r'; /* yes, there is space to add a CR */
+				i++;
+			} else
+				cr_missing = 1; /* No space in the TX buffer,
+						   rember it so it can be inserted in the next loop */
+		}
+		count -= (c-cr_missing);
+		bdp->cbd_datlen = i-1;
+		bdp->cbd_sc |= BD_SC_READY;

-	if (info)
-		info->tx_cur = (cbd_t *)bdp;
+	}
+	/* while (bdp->cbd_sc & BD_SC_READY); is this really needed? */
 }
-
 static void serial_console_write(struct console *c, const char *s,
 				unsigned count)
 {
@@ -3073,10 +3061,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/

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dan Malek]

Joakim Tjernlund wrote:

> I am taking a stab at my_console_write() as well. I have something that works well
> for me, but i am unsure if I have broken KGDB/XMON support.

You better check it.  The reason for all of the complexity in there is to
support that.  Lots of time was spent making kgdb/xmon work correctly, and
to break it or toss it away would kinda piss me off.

> ....In my new my_console_write()
> I must know the length of the TX buffer in the TXBD.

Why?

> ...But what is the TX buffer length before serial_console_setup has run(that's
> when KGB/XMON is active)?

Whatever the boot rom initializes.


	-- Dan


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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> Joakim Tjernlund wrote:
>
> > I am taking a stab at my_console_write() as well. I have something that works well
> > for me, but i am unsure if I have broken KGDB/XMON support.
>
> You better check it.  The reason for all of the complexity in there is to
> support that.  Lots of time was spent making kgdb/xmon work correctly, and
> to break it or toss it away would kinda piss me off.

Yes, so much I already figured :). I am trying to get it right. Care to test KGDB/XMON
when i am done?
>
> > ....In my new my_console_write()
> > I must know the length of the TX buffer in the TXBD.
>
> Why?

I don't want to disable interrupts for every char transmitted. I therefore fill
the TX BD as much as possible and that is why I need to know the length of the TX buffer.

>
> > ...But what is the TX buffer length before serial_console_setup has run(that's
> > when KGB/XMON is active)?
>
> Whatever the boot rom initializes.

OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
initialize it to 4.

 Jocke

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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > Whatever the boot rom initializes.
>
> OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
> initialize it to 4.
>
>  Jocke

Here we go again. This patch adds the above. I am unable to break it, please try to :)
Index: arch/ppc/8xx_io/uart.c
===================================================================
RCS file: /home/cvsadmin/cvsroot/kernel/linuxppc/arch/ppc/8xx_io/uart.c,v
retrieving revision 1.7
diff -u -r1.7 uart.c
--- arch/ppc/8xx_io/uart.c	13 Jun 2003 09:04:43 -0000	1.7
+++ arch/ppc/8xx_io/uart.c	17 Sep 2003 16:18:46 -0000
@@ -81,7 +81,11 @@
 #define TX_WAKEUP	ASYNC_SHARE_IRQ

 static char *serial_name = "CPM UART driver";
-static char *serial_version = "0.03";
+static char *serial_version = "0.04";
+
+/* TX buffer length used by my_console_write.
+   Assume minumun length until it gets set by this driver */
+static int console_tx_buf_len = 1;

 static DECLARE_TASK_QUEUE(tq_serial);

@@ -196,6 +200,7 @@

 /* The number of buffer descriptors and their sizes.
 */
+#define EARLY_BUF_SIZE	4
 #define RX_NUM_FIFO	4
 #define RX_BUF_SIZE	32
 #define TX_NUM_FIFO	4
@@ -1068,6 +1073,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t	*bdp;
 	unsigned char *cp;
+	unsigned long flags;

 	if (serial_paranoia_check(info, tty->device, "rs_put_char"))
 		return;
@@ -1075,7 +1081,16 @@
 	if (!tty)
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
+
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
@@ -1083,14 +1098,6 @@
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;

-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 static int rs_8xx_write(struct tty_struct * tty, int from_user,
@@ -1100,6 +1107,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t *bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 #ifdef CONFIG_KGDB_CONSOLE
         /* Try to let stub handle output. Returns true if it did. */
@@ -1113,44 +1121,46 @@
 	if (!tty)
 		return 0;

-	bdp = info->tx_cur;
-
 	while (1) {
 		c = MIN(count, TX_BUF_SIZE);

 		if (c <= 0)
 			break;

+		local_irq_save(flags);
+		bdp = info->tx_cur;
 		if (bdp->cbd_sc & BD_SC_READY) {
 			info->flags |= TX_WAKEUP;
+			local_irq_restore(flags);
 			break;
 		}

 		cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 		if (from_user) {
-			if (copy_from_user((void *)cp, buf, c)) {
+			c -= copy_from_user((void *)cp, buf, c);
+			if (!c) {
 				if (!ret)
 					ret = -EFAULT;
+				local_irq_restore(flags);
 				break;
 			}
 		} else {
 			memcpy((void *)cp, buf, c);
 		}
-
+		/* Get next BD.
+		 */
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = info->tx_bd_base;
+		else
+			info->tx_cur = (cbd_t *)bdp + 1;
+		local_irq_restore(flags);
+
 		bdp->cbd_datlen = c;
 		bdp->cbd_sc |= BD_SC_READY;

 		buf += c;
 		count -= c;
 		ret += c;
-
-		/* Get next BD.
-		*/
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = info->tx_bd_base;
-		else
-			bdp++;
-		info->tx_cur = (cbd_t *)bdp;
 	}
 	return ret;
 }
@@ -1210,28 +1220,28 @@
 {
 	volatile cbd_t	*bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 	ser_info_t *info = (ser_info_t *)tty->driver_data;

 	if (serial_paranoia_check(info, tty->device, "rs_send_char"))
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 	*cp = ch;
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;
-
-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 /*
@@ -1802,7 +1812,7 @@
 static void rs_8xx_wait_until_sent(struct tty_struct *tty, int timeout)
 {
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
-	unsigned long orig_jiffies, char_time;
+	unsigned long orig_jiffies, char_time, tst_res;
 	/*int lsr;*/
 	volatile cbd_t *bdp;

@@ -1837,6 +1847,7 @@
 	 * are empty.
 	 */
 	do {
+		unsigned long flags;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 		printk("lsr = %d (jiff=%lu)...", lsr, jiffies);
 #endif
@@ -1854,12 +1865,15 @@
 		 * is the buffer is available.  There are still characters
 		 * in the CPM FIFO.
 		 */
+		local_irq_save(flags);
 		bdp = info->tx_cur;
 		if (bdp == info->tx_bd_base)
 			bdp += (TX_NUM_FIFO-1);
 		else
 			bdp--;
-	} while (bdp->cbd_sc & BD_SC_READY);
+		tst_res = !!(bdp->cbd_sc & BD_SC_READY);
+		local_irq_restore(flags);
+	} while (tst_res);
 	current->state = TASK_RUNNING;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
@@ -2261,10 +2275,11 @@
 {
 	struct		serial_state	*ser;
 	ser_info_t			*info;
-	unsigned			i;
+	unsigned			i, c, cr_missing, max_tx_size;
 	volatile	cbd_t		*bdp, *bdbase;
 	volatile	smc_uart_t	*up;
 	volatile	u_char		*cp;
+	unsigned long                   flags;

 	ser = rs_table + idx;

@@ -2273,40 +2288,39 @@
 	 * we simply use the single buffer allocated.
 	 */
 	if ((info = (ser_info_t *)ser->info) != NULL) {
-		bdp = info->tx_cur;
 		bdbase = info->tx_bd_base;
-	}
-	else {
-		/* Pointer to UART in parameter ram.
-		*/
+	} else {
+		/* Pointer to UART in parameter ram. */
 		up = (smc_uart_t *)&cpmp->cp_dparam[ser->port];
-
-		/* Get the address of the host memory buffer.
-		 */
-		bdp = bdbase = (cbd_t *)&cpmp->cp_dpmem[up->smc_tbase];

+		/* Get the address of the host memory buffer.*/
 		info = &consinfo;
+		info->tx_bd_base = (cbd_t *)bdbase = (cbd_t *)&cpmp->cp_dpmem[up->smc_tbase];
+		info->tx_cur = (cbd_t *)bdbase;
 	}
+	max_tx_size = console_tx_buf_len;
+	cr_missing = 0;
+	while (1){
+		c = MIN(max_tx_size, count);
+		if (c <= 0)
+			break;

-	/*
-	 * We need to gracefully shut down the transmitter, disable
-	 * interrupts, then send our bytes out.
-	 */
-
-	/*
-	 * Now, do each character.  This is not as bad as it looks
-	 * since this is a holding FIFO and not a transmitting FIFO.
-	 * We could add the complexity of filling the entire transmit
-	 * buffer, but we would just wait longer between accesses......
-	 */
-	for (i = 0; i < count; i++, s++) {
+		local_irq_save(flags);
+		bdp = info->tx_cur;
+		bdbase = info->tx_bd_base;
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = (cbd_t *)bdbase;
+		else
+			info->tx_cur = (cbd_t *)(bdp+1);
+		local_irq_restore(flags);
+
 		/* Wait for transmitter fifo to empty.
 		 * Ready indicates output is ready, and xmt is doing
 		 * that, not that it is ready for us to send.
 		 */
 		while (bdp->cbd_sc & BD_SC_READY);

-		/* Send the character out.
+		/* Send the characters out.
 		 * If the buffer address is in the CPM DPRAM, don't
 		 * convert it.
 		 */
@@ -2314,55 +2328,32 @@
 			cp = (u_char *)(bdp->cbd_bufaddr);
 		else
 			cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
-		*cp = *s;
-
-		bdp->cbd_datlen = 1;
-		bdp->cbd_sc |= BD_SC_READY;
-
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = bdbase;
-		else
-			bdp++;
-
-		/* if a LF, also do CR... */
-		if (*s == 10) {
-			while (bdp->cbd_sc & BD_SC_READY);
-
-			/* This 'if' below will never be true, but a few
-			 * people argued with me that it was a "bug by
-			 * inspection" that is was easier to add the code
-			 * than continue the discussion.  The only time
-			 * the buffer address is in DPRAM is during early
-			 * use by kgdb/xmon, which format their own packets
-			 * and we never get here. -- Dan
-			 */
-			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;

-			if (bdp->cbd_sc & BD_SC_WRAP) {
-				bdp = bdbase;
-			}
-			else {
-				bdp++;
-			}
+		i=1; /* Keeps track of consumed TX buffer space */
+		if (cr_missing) {
+			/* Previus loop didn't have room for the CR, insert it first in this */
+			*cp++ = '\r';
+			i++;
+		}
+		cr_missing = 0;
+		for (; i <= c; i++) {
+			if ((*cp++ = *s++) != '\n')
+				continue; /* Copy bytes until a NewLine is found */
+			/* NewLine found, see if there is space in the TX buffer to add a CR */
+			if (i < max_tx_size) {
+				*cp++ = '\r'; /* yes, there is space to add a CR */
+				i++;
+			} else
+				cr_missing = 1; /* No space in the TX buffer,
+						   rember it so it can be inserted in the next loop */
 		}
-	}
-
-	/*
-	 * Finally, Wait for transmitter & holding register to empty
-	 *  and restore the IER
-	 */
-	while (bdp->cbd_sc & BD_SC_READY);
+		count -= (c-cr_missing);
+		bdp->cbd_datlen = i-1;
+		bdp->cbd_sc |= BD_SC_READY;

-	if (info)
-		info->tx_cur = (cbd_t *)bdp;
+	}
+	/* while (bdp->cbd_sc & BD_SC_READY); is this really needed? */
 }
-
 static void serial_console_write(struct console *c, const char *s,
 				unsigned count)
 {
@@ -3003,7 +2994,7 @@

 		}
 	}
-
+	console_tx_buf_len = TX_BUF_SIZE;
 	return 0;
 }

@@ -3059,7 +3050,7 @@
 	 * memory yet because vm allocator isn't initialized
 	 * during this early console init.
 	 */
-	dp_addr = m8xx_cpm_dpalloc(8);
+	dp_addr = m8xx_cpm_dpalloc(2*EARLY_BUF_SIZE);
 	mem_addr = (uint)(&cpmp->cp_dpmem[dp_addr]);

 	/* Allocate space for two buffer descriptors in the DP ram.
@@ -3073,10 +3064,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 + EARLY_BUF_SIZE);
+	consinfo.tx_bd_base = (cbd_t *) (bdp+1);

 	/* For the receive, set empty and wrap.
 	 * For transmit, set wrap.
@@ -3177,7 +3168,7 @@
 	/* Set up the baud rate generator.
 	*/
 	m8xx_cpm_setbrg((ser - rs_table), bd->bi_baudrate);
-
+	console_tx_buf_len = EARLY_BUF_SIZE;
 	return 0;
 }


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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > > Whatever the boot rom initializes.
> >
> > OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
> > initialize it to 4.
> >
> >  Jocke
>
> Here we go again. This patch adds the above. I am unable to break it, please try to :)
> Index: arch/ppc/8xx_io/uart.c

Almost a week has gone and no feedback so far. Anyone?

 Jocke

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Wed, Sep 17, 2003 at 06:33:06PM +0200, Joakim Tjernlund wrote:

>
> > > Whatever the boot rom initializes.
> >
> > OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
> > initialize it to 4.
> >
> >  Jocke
>
> Here we go again. This patch adds the above. I am unable to break it, please try to :)

What tree is this against, specifically?  It doesn't apply cleanly to
2_4_devel, 2_4 or kernel.org.

I've just moved a bunch of Dan's old changes from _devel out to a tree
Marcelo pulls from as well as linuxppc-2.4, so can you please re-do this
patch against that tree?  Thanks.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> On Wed, Sep 17, 2003 at 06:33:06PM +0200, Joakim Tjernlund wrote:
>
> >
> > > > Whatever the boot rom initializes.
> > >
> > > OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
> > > initialize it to 4.
> > >
> > >  Jocke
> >
> > Here we go again. This patch adds the above. I am unable to break it, please try to :)
>
> What tree is this against, specifically?  It doesn't apply cleanly to
> 2_4_devel, 2_4 or kernel.org.

It is against 2_4_devel minus the recent white space cleanup, as far as I can tell.
I should apply if you ignore white space differences. Can you try?

>
> I've just moved a bunch of Dan's old changes from _devel out to a tree
> Marcelo pulls from as well as linuxppc-2.4, so can you please re-do this
> patch against that tree?  Thanks.

I can only run & patch against 2_4_devel since I have made custom changes to my tree.
My tree is a bit behind right  now, sorry.

> --
> Tom Rini
> http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Fri, Sep 26, 2003 at 08:34:28PM +0200, Joakim Tjernlund wrote:
> > On Wed, Sep 17, 2003 at 06:33:06PM +0200, Joakim Tjernlund wrote:
> >
> > >
> > > > > Whatever the boot rom initializes.
> > > >
> > > > OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
> > > > initialize it to 4.
> > > >
> > > >  Jocke
> > >
> > > Here we go again. This patch adds the above. I am unable to break it, please try to :)
> >
> > What tree is this against, specifically?  It doesn't apply cleanly to
> > 2_4_devel, 2_4 or kernel.org.
>
> It is against 2_4_devel minus the recent white space cleanup, as far as I can tell.
> I should apply if you ignore white space differences. Can you try?

It does.  I'll test this out this afternoon.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Wed, Sep 17, 2003 at 06:33:06PM +0200, Joakim Tjernlund wrote:
>
> > > Whatever the boot rom initializes.
> >
> > OK, then I will assume TX buffer length of 1, until serial_console_setup has run and
> > initialize it to 4.
> >
> >  Jocke
>
> Here we go again. This patch adds the above. I am unable to break it, please try to :)

KGDB still works for me.  Dan, any final thoughts / comments /
objections?

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dan Malek]

Tom Rini wrote:

> KGDB still works for me.  Dan, any final thoughts / comments /
> objections?

In the last patch I saw, we are blocking interrupts for quite a while,
across a copy_from_user(), in 8xx_write().  This can cause large
interrupt latencies due to page faults, it would be nice if could fix that.

Thanks.


	-- Dan


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> Tom Rini wrote:
>
> > KGDB still works for me.  Dan, any final thoughts / comments /
> > objections?
>
> In the last patch I saw, we are blocking interrupts for quite a while,
> across a copy_from_user(), in 8xx_write().

Yes, thats correct. I did try different ways around that but the only thing that
would work is to make copy_from_user() copy to i tmp buf and then copy the
tmp buf into the BD. Would that be better?

 Jocke

> This can cause large
> interrupt latencies due to page faults, it would be nice if could fix that.
>
> -- Dan
>

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dan Malek]

Joakim Tjernlund wrote:

> Yes, thats correct. I did try different ways around that but the only thing that
> would work is to make copy_from_user() copy to i tmp buf and then copy the
> tmp buf into the BD. Would that be better?

I wonder what happens if we mark a buffer ready with a count of zero?
Since the typical case is no page fault, I would just update the BD ptr
early, use the buffer, mark it ready.  If you get a fault, put a zero
for the count and mark it ready.  If that freaks out the CPM, put one
byte of zero into the buffer and give it a count of 1.  Then you just have
to hold the lock across the update of the BD ptr, just like everywhere else.


	-- Dan


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> Joakim Tjernlund wrote:
>
> > Yes, thats correct. I did try different ways around that but the only thing that
> > would work is to make copy_from_user() copy to i tmp buf and then copy the
> > tmp buf into the BD. Would that be better?
>
> I wonder what happens if we mark a buffer ready with a count of zero?
> Since the typical case is no page fault, I would just update the BD ptr
> early, use the buffer, mark it ready.  If you get a fault, put a zero
> for the count and mark it ready.

I tried that and it broke the CPM.

> If that freaks out the CPM, put one
> byte of zero into the buffer and give it a count of 1.  Then you just have
> to hold the lock across the update of the BD ptr, just like everywhere else.

That will work, but wont the zero show up on the console? hmm, since
the zero count will be very unlikely case anyhow,  I can live with that.

 Jocke

>
> -- Dan


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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > If that freaks out the CPM, put one
> > byte of zero into the buffer and give it a count of 1.  Then you just have
> > to hold the lock across the update of the BD ptr, just like everywhere else.
>
> That will work, but wont the zero show up on the console? hmm, since
> the zero count will be very unlikely case anyhow,  I can live with that.

OK, this patch does the above. I left my test function(err_copy_from_user)
to simulate bad copy_from_user() copies every now and then. Just remove
it and change the call in rs_8xx_write when you have tested it.
Works great for me, not even jove(emacs like editor) will break it.

 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.7
diff -u -r1.7 uart.c
--- arch/ppc/8xx_io/uart.c	13 Jun 2003 09:04:43 -0000	1.7
+++ arch/ppc/8xx_io/uart.c	27 Sep 2003 13:29:53 -0000
@@ -81,7 +81,11 @@
 #define TX_WAKEUP	ASYNC_SHARE_IRQ

 static char *serial_name = "CPM UART driver";
-static char *serial_version = "0.03";
+static char *serial_version = "0.04";
+
+/* TX buffer length used by my_console_write.
+   Assume minumun length until it gets set by this driver */
+static int console_tx_buf_len = 1;

 static DECLARE_TASK_QUEUE(tq_serial);

@@ -196,6 +200,7 @@

 /* The number of buffer descriptors and their sizes.
 */
+#define EARLY_BUF_SIZE	4
 #define RX_NUM_FIFO	4
 #define RX_BUF_SIZE	32
 #define TX_NUM_FIFO	4
@@ -1068,6 +1073,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t	*bdp;
 	unsigned char *cp;
+	unsigned long flags;

 	if (serial_paranoia_check(info, tty->device, "rs_put_char"))
 		return;
@@ -1075,7 +1081,16 @@
 	if (!tty)
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
+
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
@@ -1083,14 +1098,20 @@
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;

-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
+}
+int err_copy_from_user(void * to, void* from, int len)
+{
+	static int err;
+	int res;
+
+	err = (err+1)%13;

-	info->tx_cur = (cbd_t *)bdp;
+	res = copy_from_user(to, from, len);
+	if(!err && len > 1)
+		return len-1; /* partial copy */
+	if(!err)
+		return len; /* nothing copied */
+	return res;
 }

 static int rs_8xx_write(struct tty_struct * tty, int from_user,
@@ -1100,6 +1121,7 @@
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t *bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 #ifdef CONFIG_KGDB_CONSOLE
         /* Try to let stub handle output. Returns true if it did. */
@@ -1113,44 +1135,47 @@
 	if (!tty)
 		return 0;

-	bdp = info->tx_cur;
-
 	while (1) {
 		c = MIN(count, TX_BUF_SIZE);

 		if (c <= 0)
 			break;

+		local_irq_save(flags);
+		bdp = info->tx_cur;
 		if (bdp->cbd_sc & BD_SC_READY) {
 			info->flags |= TX_WAKEUP;
+			local_irq_restore(flags);
 			break;
 		}
+		/* Get next BD.
+		 */
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = info->tx_bd_base;
+		else
+			info->tx_cur = (cbd_t *)bdp + 1;
+		local_irq_restore(flags);

 		cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
-		if (from_user) {
-			if (copy_from_user((void *)cp, buf, c)) {
-				if (!ret)
-					ret = -EFAULT;
-				break;
-			}
-		} else {
+		if (from_user)
+			c -= err_copy_from_user((void *)cp, buf, c);
+		else
 			memcpy((void *)cp, buf, c);
+
+		if (c) {
+			bdp->cbd_datlen = c;
+			bdp->cbd_sc |= BD_SC_READY;
+		} else {
+			bdp->cbd_datlen = 1;/* Need to TX at least 1 char to keep CPM sane */
+			*cp = 0;
+			bdp->cbd_sc |= BD_SC_READY;
+			if (!ret)
+				ret = -EFAULT;
+			break;
 		}
-
-		bdp->cbd_datlen = c;
-		bdp->cbd_sc |= BD_SC_READY;
-
 		buf += c;
 		count -= c;
 		ret += c;
-
-		/* Get next BD.
-		*/
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = info->tx_bd_base;
-		else
-			bdp++;
-		info->tx_cur = (cbd_t *)bdp;
 	}
 	return ret;
 }
@@ -1210,28 +1235,28 @@
 {
 	volatile cbd_t	*bdp;
 	unsigned char	*cp;
+	unsigned long flags;

 	ser_info_t *info = (ser_info_t *)tty->driver_data;

 	if (serial_paranoia_check(info, tty->device, "rs_send_char"))
 		return;

+	local_irq_save(flags);
 	bdp = info->tx_cur;
+	/* Get next BD.
+	*/
+	if (bdp->cbd_sc & BD_SC_WRAP)
+		info->tx_cur = info->tx_bd_base;
+	else
+		info->tx_cur = (cbd_t *)bdp + 1;
+	local_irq_restore(flags);
 	while (bdp->cbd_sc & BD_SC_READY);

 	cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
 	*cp = ch;
 	bdp->cbd_datlen = 1;
 	bdp->cbd_sc |= BD_SC_READY;
-
-	/* Get next BD.
-	*/
-	if (bdp->cbd_sc & BD_SC_WRAP)
-		bdp = info->tx_bd_base;
-	else
-		bdp++;
-
-	info->tx_cur = (cbd_t *)bdp;
 }

 /*
@@ -1802,7 +1827,7 @@
 static void rs_8xx_wait_until_sent(struct tty_struct *tty, int timeout)
 {
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
-	unsigned long orig_jiffies, char_time;
+	unsigned long orig_jiffies, char_time, tst_res;
 	/*int lsr;*/
 	volatile cbd_t *bdp;

@@ -1837,6 +1862,7 @@
 	 * are empty.
 	 */
 	do {
+		unsigned long flags;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 		printk("lsr = %d (jiff=%lu)...", lsr, jiffies);
 #endif
@@ -1854,12 +1880,15 @@
 		 * is the buffer is available.  There are still characters
 		 * in the CPM FIFO.
 		 */
+		local_irq_save(flags);
 		bdp = info->tx_cur;
 		if (bdp == info->tx_bd_base)
 			bdp += (TX_NUM_FIFO-1);
 		else
 			bdp--;
-	} while (bdp->cbd_sc & BD_SC_READY);
+		tst_res = !!(bdp->cbd_sc & BD_SC_READY);
+		local_irq_restore(flags);
+	} while (tst_res);
 	current->state = TASK_RUNNING;
 #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
 	printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);
@@ -2261,10 +2290,11 @@
 {
 	struct		serial_state	*ser;
 	ser_info_t			*info;
-	unsigned			i;
+	unsigned			i, c, cr_missing, max_tx_size;
 	volatile	cbd_t		*bdp, *bdbase;
 	volatile	smc_uart_t	*up;
 	volatile	u_char		*cp;
+	unsigned long                   flags;

 	ser = rs_table + idx;

@@ -2273,40 +2303,39 @@
 	 * we simply use the single buffer allocated.
 	 */
 	if ((info = (ser_info_t *)ser->info) != NULL) {
-		bdp = info->tx_cur;
 		bdbase = info->tx_bd_base;
-	}
-	else {
-		/* Pointer to UART in parameter ram.
-		*/
+	} else {
+		/* Pointer to UART in parameter ram. */
 		up = (smc_uart_t *)&cpmp->cp_dparam[ser->port];
-
-		/* Get the address of the host memory buffer.
-		 */
-		bdp = bdbase = (cbd_t *)&cpmp->cp_dpmem[up->smc_tbase];

+		/* Get the address of the host memory buffer.*/
 		info = &consinfo;
+		info->tx_bd_base = (cbd_t *)bdbase = (cbd_t *)&cpmp->cp_dpmem[up->smc_tbase];
+		info->tx_cur = (cbd_t *)bdbase;
 	}
+	max_tx_size = console_tx_buf_len;
+	cr_missing = 0;
+	while (1){
+		c = MIN(max_tx_size, count);
+		if (c <= 0)
+			break;

-	/*
-	 * We need to gracefully shut down the transmitter, disable
-	 * interrupts, then send our bytes out.
-	 */
-
-	/*
-	 * Now, do each character.  This is not as bad as it looks
-	 * since this is a holding FIFO and not a transmitting FIFO.
-	 * We could add the complexity of filling the entire transmit
-	 * buffer, but we would just wait longer between accesses......
-	 */
-	for (i = 0; i < count; i++, s++) {
+		local_irq_save(flags);
+		bdp = info->tx_cur;
+		bdbase = info->tx_bd_base;
+		if (bdp->cbd_sc & BD_SC_WRAP)
+			info->tx_cur = (cbd_t *)bdbase;
+		else
+			info->tx_cur = (cbd_t *)(bdp+1);
+		local_irq_restore(flags);
+
 		/* Wait for transmitter fifo to empty.
 		 * Ready indicates output is ready, and xmt is doing
 		 * that, not that it is ready for us to send.
 		 */
 		while (bdp->cbd_sc & BD_SC_READY);

-		/* Send the character out.
+		/* Send the characters out.
 		 * If the buffer address is in the CPM DPRAM, don't
 		 * convert it.
 		 */
@@ -2314,55 +2343,32 @@
 			cp = (u_char *)(bdp->cbd_bufaddr);
 		else
 			cp = info->tx_va_base + ((bdp - info->tx_bd_base) * TX_BUF_SIZE);
-		*cp = *s;
-
-		bdp->cbd_datlen = 1;
-		bdp->cbd_sc |= BD_SC_READY;
-
-		if (bdp->cbd_sc & BD_SC_WRAP)
-			bdp = bdbase;
-		else
-			bdp++;
-
-		/* if a LF, also do CR... */
-		if (*s == 10) {
-			while (bdp->cbd_sc & BD_SC_READY);
-
-			/* This 'if' below will never be true, but a few
-			 * people argued with me that it was a "bug by
-			 * inspection" that is was easier to add the code
-			 * than continue the discussion.  The only time
-			 * the buffer address is in DPRAM is during early
-			 * use by kgdb/xmon, which format their own packets
-			 * and we never get here. -- Dan
-			 */
-			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;

-			if (bdp->cbd_sc & BD_SC_WRAP) {
-				bdp = bdbase;
-			}
-			else {
-				bdp++;
-			}
+		i=1; /* Keeps track of consumed TX buffer space */
+		if (cr_missing) {
+			/* Previus loop didn't have room for the CR, insert it first in this */
+			*cp++ = '\r';
+			i++;
+		}
+		cr_missing = 0;
+		for (; i <= c; i++) {
+			if ((*cp++ = *s++) != '\n')
+				continue; /* Copy bytes until a NewLine is found */
+			/* NewLine found, see if there is space in the TX buffer to add a CR */
+			if (i < max_tx_size) {
+				*cp++ = '\r'; /* yes, there is space to add a CR */
+				i++;
+			} else
+				cr_missing = 1; /* No space in the TX buffer,
+						   rember it so it can be inserted in the next loop */
 		}
-	}
-
-	/*
-	 * Finally, Wait for transmitter & holding register to empty
-	 *  and restore the IER
-	 */
-	while (bdp->cbd_sc & BD_SC_READY);
+		count -= (c-cr_missing);
+		bdp->cbd_datlen = i-1;
+		bdp->cbd_sc |= BD_SC_READY;

-	if (info)
-		info->tx_cur = (cbd_t *)bdp;
+	}
+	/* while (bdp->cbd_sc & BD_SC_READY); is this really needed? */
 }
-
 static void serial_console_write(struct console *c, const char *s,
 				unsigned count)
 {
@@ -3003,7 +3009,7 @@

 		}
 	}
-
+	console_tx_buf_len = TX_BUF_SIZE;
 	return 0;
 }

@@ -3059,7 +3065,7 @@
 	 * memory yet because vm allocator isn't initialized
 	 * during this early console init.
 	 */
-	dp_addr = m8xx_cpm_dpalloc(8);
+	dp_addr = m8xx_cpm_dpalloc(2*EARLY_BUF_SIZE);
 	mem_addr = (uint)(&cpmp->cp_dpmem[dp_addr]);

 	/* Allocate space for two buffer descriptors in the DP ram.
@@ -3073,10 +3079,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 + EARLY_BUF_SIZE);
+	consinfo.tx_bd_base = (cbd_t *) (bdp+1);

 	/* For the receive, set empty and wrap.
 	 * For transmit, set wrap.
@@ -3177,7 +3183,7 @@
 	/* Set up the baud rate generator.
 	*/
 	m8xx_cpm_setbrg((ser - rs_table), bd->bi_baudrate);
-
+	console_tx_buf_len = EARLY_BUF_SIZE;
 	return 0;
 }


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Sat, Sep 27, 2003 at 03:43:51PM +0200, Joakim Tjernlund wrote:
> > > If that freaks out the CPM, put one
> > > byte of zero into the buffer and give it a count of 1.  Then you just have
> > > to hold the lock across the update of the BD ptr, just like everywhere else.
> >
> > That will work, but wont the zero show up on the console? hmm, since
> > the zero count will be very unlikely case anyhow,  I can live with that.
>
> OK, this patch does the above. I left my test function(err_copy_from_user)
> to simulate bad copy_from_user() copies every now and then. Just remove
> it and change the call in rs_8xx_write when you have tested it.
> Works great for me, not even jove(emacs like editor) will break it.

KGDB still works for me.  Dan, happy with this?

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dan Malek]

Tom Rini wrote:

> KGDB still works for me.  Dan, happy with this?

Go for it.

Thanks.


	-- Dan


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Kumar Gala]

Did I miss something or did is someone moving this patch from 8xx to
8260?  It looks like Tom's commit of Joakim's work was for 8xx.

- kumar

On Tuesday, September 30, 2003, at 10:28 AM, Dan Malek wrote:

>
> Tom Rini wrote:
>
>> KGDB still works for me.  Dan, happy with this?
>
> Go for it.
>
> Thanks.
>
>
> 	-- Dan
>
>


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Wed, Oct 01, 2003 at 09:26:29AM -0500, Kumar Gala wrote:

> Did I miss something or did is someone moving this patch from 8xx to
> 8260?  It looks like Tom's commit of Joakim's work was for 8xx.

No one has moved this to 8260 as no one with access to hw (not me,
easily) has time to do it.  If you can, I'll apply it.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Gary Thomas]

On Wed, 2003-10-01 at 08:32, Tom Rini wrote:
> On Wed, Oct 01, 2003 at 09:26:29AM -0500, Kumar Gala wrote:
>
> > Did I miss something or did is someone moving this patch from 8xx to
> > 8260?  It looks like Tom's commit of Joakim's work was for 8xx.
>
> No one has moved this to 8260 as no one with access to hw (not me,
> easily) has time to do it.  If you can, I'll apply it.
>

I'll be glad to test this as well (sadly I don't have time today to
help with moving the patch).  I have access to both 8xx and 82xx boards.

--
Gary Thomas <gary@mlbassoc.com>
MLB Associates


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> On Wed, 2003-10-01 at 08:32, Tom Rini wrote:
> > On Wed, Oct 01, 2003 at 09:26:29AM -0500, Kumar Gala wrote:
> >
> > > Did I miss something or did is someone moving this patch from 8xx to
> > > 8260?  It looks like Tom's commit of Joakim's work was for 8xx.
> >
> > No one has moved this to 8260 as no one with access to hw (not me,
> > easily) has time to do it.  If you can, I'll apply it.

Commited where? I can't see it in http://ppc.bitkeeper.com:8080/linuxppc_2_4_devel

 Jocke

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Wed, Oct 01, 2003 at 11:20:50PM +0200, Joakim Tjernlund wrote:
> > On Wed, 2003-10-01 at 08:32, Tom Rini wrote:
> > > On Wed, Oct 01, 2003 at 09:26:29AM -0500, Kumar Gala wrote:
> > >
> > > > Did I miss something or did is someone moving this patch from 8xx to
> > > > 8260?  It looks like Tom's commit of Joakim's work was for 8xx.
> > >
> > > No one has moved this to 8260 as no one with access to hw (not me,
> > > easily) has time to do it.  If you can, I'll apply it.
>
> Commited where? I can't see it in http://ppc.bitkeeper.com:8080/linuxppc_2_4_devel

It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
for 2.4.21-pre7.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > Commited where? I can't see it in http://ppc.bitkeeper.com:8080/linuxppc_2_4_devel
>
> It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> for 2.4.21-pre7.

Ahh, found it in http://ppc.bkbits.net:8080/linuxppc-2.4. I did look in
http://ppc.bitkeeper.com:8080/linuxppc_2_4 but it is not there. Is this tree obsolete?

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Wed, Oct 01, 2003 at 11:51:36PM +0200, Joakim Tjernlund wrote:

> > > Commited where? I can't see it in http://ppc.bitkeeper.com:8080/linuxppc_2_4_devel
> >
> > It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> > for 2.4.21-pre7.
>
> Ahh, found it in http://ppc.bkbits.net:8080/linuxppc-2.4. I did look in
> http://ppc.bitkeeper.com:8080/linuxppc_2_4 but it is not there. Is this tree obsolete?

It's not preferred, certainly.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> > > It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> > > for 2.4.21-pre7.
> >
> > Ahh, found it in http://ppc.bkbits.net:8080/linuxppc-2.4. I did look in
> > http://ppc.bitkeeper.com:8080/linuxppc_2_4 but it is not there. Is this tree obsolete?
>
> It's not preferred, certainly.

What about linuxppc_2_4_devel? Is that also "non preferred"?

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Thu, Oct 02, 2003 at 12:17:10AM +0200, Joakim Tjernlund wrote:
> > > > It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> > > > for 2.4.21-pre7.
> > >
> > > Ahh, found it in http://ppc.bkbits.net:8080/linuxppc-2.4. I did look in
> > > http://ppc.bitkeeper.com:8080/linuxppc_2_4 but it is not there. Is this tree obsolete?
> >
> > It's not preferred, certainly.
>
> What about linuxppc_2_4_devel? Is that also "non preferred"?

Yes.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Robin Gilks]

Tom Rini wrote:
> On Thu, Oct 02, 2003 at 12:17:10AM +0200, Joakim Tjernlund wrote:
>
>>>>>It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
>>>>>for 2.4.21-pre7.
>>>>
>>>>Ahh, found it in http://ppc.bkbits.net:8080/linuxppc-2.4. I did look in
>>>>http://ppc.bitkeeper.com:8080/linuxppc_2_4 but it is not there. Is this tree obsolete?
>>>
>>>It's not preferred, certainly.
>>
>>What about linuxppc_2_4_devel? Is that also "non preferred"?
>
>
> Yes.

Rather than lots of URLs that are NOT recommended, is there one that is?
Also, you refer to 2.4.21-pre but I thought we had moved onto 2.4.23-pre
now. Is the ppc tree really that much out-of-date?

Very much in need of getting rid of the CPM memory leak in 2.4.21 so I
have a very acute interest in getting the latest & greatest!!

--
Robin Gilks
Senior Design Engineer          Phone: (+64)(3) 357 1569
Tait Electronics                Fax  :  (+64)(3) 359 4632
PO Box 1645 Christchurch        Email : robin.gilks@tait.co.nz
New Zealand


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Thu, Oct 02, 2003 at 11:23:27AM +1200, Robin Gilks wrote:

> Tom Rini wrote:
> >On Thu, Oct 02, 2003 at 12:17:10AM +0200, Joakim Tjernlund wrote:
> >
> >>>>>It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> >>>>>for 2.4.21-pre7.
> >>>>
> >>>>Ahh, found it in http://ppc.bkbits.net:8080/linuxppc-2.4. I did look in
> >>>>http://ppc.bitkeeper.com:8080/linuxppc_2_4 but it is not there. Is this
> >>>>tree obsolete?
> >>>
> >>>It's not preferred, certainly.
> >>
> >>What about linuxppc_2_4_devel? Is that also "non preferred"?
> >
> >
> >Yes.
>
> Rather than lots of URLs that are NOT recommended, is there one that is?

For the moment it's "whatever you have now is probably fine".  For
2.4.23 I would like to get most of _devel cleaned out, wrt 8xx.  This
will probably leave out some 'new' boards that Wolfgang sent out a long
time ago (FWIW, once I get everything else out I do plan to ask Wolfgang
if he wants to re-submit those boards or verify there's nothing new
needed to what's in _devel or something).

If you have a new board, or a bug fix, current kernel.org (BK, snapshots
of BK, just official releases) is best.


> Also, you refer to 2.4.21-pre but I thought we had moved onto 2.4.23-pre
> now. Is the ppc tree really that much out-of-date?

Whoops, yes, 2.4.23-pre.

--
Tom Rini
http://gate.crashing.org/~trini/

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Wolfgang Denk]

In message <20031001235158.GD17768@ip68-0-152-218.tc.ph.cox.net> you wrote:
>
> For the moment it's "whatever you have now is probably fine".  For
> 2.4.23 I would like to get most of _devel cleaned out, wrt 8xx.  This
> will probably leave out some 'new' boards that Wolfgang sent out a long
> time ago (FWIW, once I get everything else out I do plan to ask Wolfgang
> if he wants to re-submit those boards or verify there's nothing new
> needed to what's in _devel or something).

It would be much easier for us if we knew which tree we should watch.
It seems this is changing every week or see. One  it  was  2.4,  then
_devel, then 2.5, then back _devel, now ...

I see no method in all this.

> If you have a new board, or a bug fix, current kernel.org (BK, snapshots
> of BK, just official releases) is best.

Ummm... Since when has this been the case?



Best regards,

Wolfgang Denk

--
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
"Plan to throw one away.  You will anyway."
                              - Fred Brooks, "The Mythical Man Month"

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dan Kegel]

Tom Rini wrote:
>>Commited where? I can't see it in http://ppc.bitkeeper.com:8080/linuxppc_2_4_devel
>
>
> It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> for 2.4.21-pre7.

Say, how up to date is Marcelo's tree in general
with respect to linuxppc_2_4_devel?  Does it have
the ppc405 support stuff?

We have been using a linuxppc_2_4_devel snapshot from
a year or so ago on the ppc405, and it'd be nice to be able to
move to a vanilla kernel someday.

Thanks,
Dan

--
Dan Kegel
http://www.kegel.com
http://counter.li.org/cgi-bin/runscript/display-person.cgi?user=78045


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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Tom Rini]

On Wed, Oct 01, 2003 at 11:03:42PM -0700, Dan Kegel wrote:
> Tom Rini wrote:
> >>Commited where? I can't see it in
> >>http://ppc.bitkeeper.com:8080/linuxppc_2_4_devel
> >
> >
> >It's in the linuxppc-2.4 tree.  I'll try and get it to Marcelo in time
> >for 2.4.21-pre7.
>
> Say, how up to date is Marcelo's tree in general
> with respect to linuxppc_2_4_devel?  Does it have
> the ppc405 support stuff?

Some, and the better things in 2.6 are being backported, slowly.

> We have been using a linuxppc_2_4_devel snapshot from
> a year or so ago on the ppc405, and it'd be nice to be able to
> move to a vanilla kernel someday.

I hear that...

--
Tom Rini
http://gate.crashing.org/~trini/

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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> I had a similar problem on my_console_write. Here I wite down two version of the code, the first one is the old
> and the second one is the new with the modification. As you can see, there was a spin_unlock without a spink_lock
> ;this hangs the startup in calibration loop.

Agostino,
check out the new 8xx_io/uart.c in BK. I wrote a patch that fixes the locking problems
but minimizes the lock time.

   Jocke

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

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Joakim Tjernlund]

> Hi,

Hi Steffen.

>
> I have seen a similar problem for the 2.4.20.
>
> When I force a lot of console output via the following command:
>
>    while true; do cd /; ls -R; done
>
> and type-in some letters in parallel, the console
> becomes crazy.

My custom mpc860 also becomes crazy or just locks up when I do this. The
drivers are pretty the same, so this is no surprise.

>
> I have added some instrumentation in order to dump the
> TX Buffer Descriptor Table. I have found that the
> hardware pointer (TBPTR) and the software pointer (tx_cur)
> are not more synchronized together:
> Inside uart.c, there are the following output routines:
[SNIP]
>
>    rs_8xx_put_char()
>    rs_8xx_write()
>    rs_8xx_send_xchar()
>    my_console_write()
>
> I think there must be a synchronization accessing the
> TX BD table. I suggest the patch attached.

Hmm, don't you turn off irq's for a long time with this patch?

 Jocke

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

Re: Problem of concurrency in arch/ppc/8260_io/uart.c

[Steffen Rumler]

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

Hi,

I have seen a similar problem for the 2.4.20.

When I force a lot of console output via the following command:

   while true; do cd /; ls -R; done

and type-in some letters in parallel, the console
becomes crazy.

I have added some instrumentation in order to dump the
TX Buffer Descriptor Table. I have found that the
hardware pointer (TBPTR) and the software pointer (tx_cur)
are not more synchronized together:

 >> make new rlogin session <<
/root# cd /proc/driver

/root# cat uart-bdtables; cat mpc82xx/smc1_pram | grep SMC1_PRAM_TBPTR

TX BD table
   (000 at 0xfff005f0) status: 0x1000 len: 0001 addr: 0x001bb084
   (001 at 0xfff005f8) status: 0x1000 len: 0001 addr: 0x001bb0a4
* (002 at 0xfff00600) status: 0x1000 len: 0001 addr: 0x001bb0c4
   (003 at 0xfff00608) status: 0x3000 len: 0004 addr: 0x001bb0e4
    SMC1_PRAM_TBPTR            0x20         2     0600

    --> hardware and software pointer still synchronized

    >> force console to become crazy (see above) <<

/root# cat uart-bdtables; cat mpc82xx/smc1_pram | grep SMC1_PRAM_TBPTR

TX BD table (tbptr: 0x00000088)
   (000 at 0xfff005f0) status: 0x1000 len: 0003 addr: 0x001bb084
* (001 at 0xfff005f8) status: 0x1000 len: 0021 addr: 0x001bb0a4
   (002 at 0xfff00600) status: 0x1000 len: 0001 addr: 0x001bb0c4
   (003 at 0xfff00608) status: 0x3000 len: 0001 addr: 0x001bb0e4
    SMC1_PRAM_TBPTR            0x20         2     0600

    --> hardware and software pointer NOT more synchronized

    >> make additional console output: echo foo >/dev/console <<

/root# cat uart-bdtables; cat mpc82xx/smc1_pram | grep SMC1_PRAM_TBPTR

* (000 at 0xfff005f0) status: 0x1000 len: 0003 addr: 0x001bb084
   (001 at 0xfff005f8) status: 0x9000 len: 0004 addr: 0x001bb0a4
   (002 at 0xfff00600) status: 0x1000 len: 0001 addr: 0x001bb0c4
   (003 at 0xfff00608) status: 0x3000 len: 0001 addr: 0x001bb0e4
    SMC1_PRAM_TBPTR            0x20         2     05f0

    --> hardware pointer hangs at 0x5f0 because R-Bit not set, but
        at 0x5f8

Inside uart.c, there are the following output routines:

   rs_8xx_put_char()
   rs_8xx_write()
   rs_8xx_send_xchar()
   my_console_write()

I think there must be a synchronization accessing the
TX BD table. I suggest the patch attached.


Best Regards
Steffen
--


--------------------------------------------------------------

Steffen Rumler
ICN CP D NT SW 7
Siemens AG
Hofmannstr. 51                 Email: Steffen.Rumler@siemens.com
D-81359 Munich                 Phone: +49 89 722-44061
Germany                        Fax  : +49 89 722-36703

--------------------------------------------------------------



[-- Attachment #2: uart.c.patch --]
[-- Type: text/plain, Size: 3270 bytes --]

diff -Naur old/uart.c new/uart.c
--- old/uart.c	Mon Sep 15 11:52:02 2003
+++ new/uart.c	Mon Sep 15 11:51:32 2003
@@ -44,6 +44,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/delay.h>
+#include <linux/spinlock.h>
 #include <asm/uaccess.h>
 #include <asm/immap_8260.h>
 #include <asm/mpc8260.h>
@@ -253,6 +254,11 @@
 	cbd_t			*rx_cur;
 	cbd_t			*tx_bd_base;
 	cbd_t			*tx_cur;
+
+        /*  for output synchronization
+         */
+        spinlock_t output_lock;
+        
 } ser_info_t;
 
 static void change_speed(ser_info_t *info);
@@ -1010,6 +1016,7 @@
 {
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t	*bdp;
+        unsigned long flags;
 
 	if (serial_paranoia_check(info, tty->device, "rs_put_char"))
 		return;
@@ -1017,6 +1024,8 @@
 	if (!tty)
 		return;
 
+        spin_lock_irqsave(&(info->output_lock), flags);
+
 	bdp = info->tx_cur;
 	while (bdp->cbd_sc & BD_SC_READY);
 
@@ -1033,6 +1042,8 @@
 
 	info->tx_cur = (cbd_t *)bdp;
 
+        spin_unlock_irqrestore(&(info->output_lock), flags);
+
 }
 
 static int rs_8xx_write(struct tty_struct * tty, int from_user,
@@ -1041,6 +1052,7 @@
 	int	c, ret = 0;
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 	volatile cbd_t *bdp;
+        unsigned long flags;
 
 	if (serial_paranoia_check(info, tty->device, "rs_write"))
 		return 0;
@@ -1048,6 +1060,8 @@
 	if (!tty) 
 		return 0;
 
+        spin_lock_irqsave(&(info->output_lock), flags);
+
 	bdp = info->tx_cur;
 
 	while (1) {
@@ -1086,6 +1100,9 @@
 			bdp++;
 		info->tx_cur = (cbd_t *)bdp;
 	}
+
+        spin_unlock_irqrestore(&(info->output_lock), flags);
+
 	return ret;
 }
 
@@ -1143,12 +1160,15 @@
 static void rs_8xx_send_xchar(struct tty_struct *tty, char ch)
 {
 	volatile cbd_t	*bdp;
+        unsigned long flags;
 
 	ser_info_t *info = (ser_info_t *)tty->driver_data;
 
 	if (serial_paranoia_check(info, tty->device, "rs_send_char"))
 		return;
 
+        spin_lock_irqsave(&(info->output_lock), flags);
+
 	bdp = info->tx_cur;
 	while (bdp->cbd_sc & BD_SC_READY);
 
@@ -1164,6 +1184,8 @@
 		bdp++;
 
 	info->tx_cur = (cbd_t *)bdp;
+
+        spin_unlock_irqrestore(&(info->output_lock), flags);
 }
 
 /*
@@ -2227,9 +2249,11 @@
 	volatile	cbd_t		*bdp, *bdbase;
 	volatile	smc_uart_t	*up;
 	volatile	u_char		*cp;
+        unsigned long   flags;
 
 	ser = rs_table + idx;
 
+
 	/* If the port has been initialized for general use, we have
 	 * to use the buffer descriptors allocated there.  Otherwise,
 	 * we simply use the single buffer allocated.
@@ -2237,6 +2261,7 @@
 	if ((info = (ser_info_t *)ser->info) != NULL) {
 		bdp = info->tx_cur;
 		bdbase = info->tx_bd_base;
+                spin_lock_irqsave(&(info->output_lock), flags);
 	}
 	else {
 		/* Pointer to UART in parameter ram.
@@ -2309,6 +2334,9 @@
 
 	if (info)
 		info->tx_cur = (cbd_t *)bdp;
+
+        if (info)
+            spin_unlock_irqrestore(&(info->output_lock), flags);
 }
 
 static void serial_console_write(struct console *c, const char *s,
@@ -2764,6 +2792,7 @@
 			info->tqueue_hangup.data = info;
 			info->line = i;
 			info->state = state;
+                        spin_lock_init(&(info->output_lock));
 			state->info = (struct async_struct *)info;
 
 			/* We need to allocate a transmit and receive buffer

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Jean-Denis Boyer]

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


> Oh, I almost forgot. Yes I would like to see your test 
> program. One of the
> reasons I haven't chased the problem is because it keeps 
> disappearing;-)
> 
> Dean Dayton

Here is, in attachement, the test program.
There are two files in the tar:

  -- testprintk.c --
    A module that adds a device /dev/tpk.
    Once a file descriptor is obtained on this device,
    an internal timer starts, which calls "printk",
    each 10 ticks (100ms in my setup). When the file descriptor
    is released, the timer stops.

  -- main.c --
    A simple user mode program that opens /dev/tpk,
    and then performs an infinite loop printing to the stdout.
    When the program stops (Ctrl-C), the descriptor will be
    released by Linux, stopping the internal timer.

With that test, the problem almost immediatly appears.

BTW, the same problem is on the 860 also, which can be diagnosed the same way.

Regards,
--------------------------------------------
 Jean-Denis Boyer, B.Eng., Technical Leader
 Mediatrix Telecom Inc.
 4229 Garlock Street
 Sherbrooke (Québec)
 J1L 2C8  CANADA
 (819)829-8749 x241
--------------------------------------------

[-- Attachment #2: testprintk.tar.gz --]
[-- Type: application/x-gzip, Size: 1230 bytes --]

RE: Problem of concurrency in arch/ppc/8260_io/uart.c

[Dayton, Dean]

Yes, I have seen this problem when I have a lot of printks from interrupt
context on my 8255 board using an SCC as the console. It's been on my list
for awhile but I haven't done anything about it yet. I think the easy
approach would be to disable interrupts when manipulating the buffer
descriptors (in fact, there are comments in the driver that make me suspect
this used to be the done). But there would be a performance hit for this.
Anyone got a more elegant suggestion?

Dean Dayton

> -----Original Message-----
> From: Jean-Denis Boyer [mailto:jdboyer@mediatrix.com]
> Sent: Thursday, March 06, 2003 4:39 PM
> To: Linux PPC embedded
> Subject: Problem of concurrency in arch/ppc/8260_io/uart.c
>
>
>
> I have a problem of concurrency on my 8250 based custom
> board. I'm using kernel 2.4.19, and the output on the serial
> port goes crappy.
>
> It seems to appear when printk is called from interrupt
> context, while a user mode process is writing to stdout.
>
> I wrote a user mode program + a kernel module to reproduce
> the problem. The kernel module starts a timer that calls
> printk (34 chars) each 100ms. The user mode program performs
> a while(1) printf(...); That combination almost immediatly
> mess the serial driver, that begins to output characters in
> an unpredictable order, until we reboot. It never crashes,
> however, but it is annoying.
>
> Has anyone encoutered that problem?
> or has any suggestion before I go further?
> or wish to have the test program to give it a try?
>
> Thanks,
> --------------------------------------------
>  Jean-Denis Boyer, B.Eng., Technical Leader
>  Mediatrix Telecom Inc.
>  4229 Garlock Street
>  Sherbrooke (Québec)
>  J1L 2C8  CANADA
>  (819)829-8749 x241
> --------------------------------------------
>

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

Problem of concurrency in arch/ppc/8260_io/uart.c

[Jean-Denis Boyer]

I have a problem of concurrency on my 8250 based custom board.
I'm using kernel 2.4.19, and the output on the serial port goes crappy.

It seems to appear when printk is called from interrupt context,
while a user mode process is writing to stdout.

I wrote a user mode program + a kernel module to reproduce the problem.
The kernel module starts a timer that calls printk (34 chars) each 100ms.
The user mode program performs a while(1) printf(...);
That combination almost immediatly mess the serial driver,
that begins to output characters in an unpredictable order, until we reboot.
It never crashes, however, but it is annoying.

Has anyone encoutered that problem?
or has any suggestion before I go further?
or wish to have the test program to give it a try?

Thanks,
--------------------------------------------
 Jean-Denis Boyer, B.Eng., Technical Leader
 Mediatrix Telecom Inc.
 4229 Garlock Street
 Sherbrooke (Québec)
 J1L 2C8  CANADA
 (819)829-8749 x241
--------------------------------------------

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