[PATCH 0/5] usb_debug driver improvements

[PATCH 0/5] usb_debug driver improvements

[Jason Wessel]

After using a USB 2.0 debug device along with the early and standard
usb_debug driver, I came up with some patches to help improve the
functionality of using it with the linux kernel.

The last patch in this series is not meant for upstream merging, it is
marked experimental along with questions.  The first 4 patches in the
series are a request for review, comments and future merge
consideration.

The idea behind this patch series is to allow the USB 2.0 debug device
to operate as both an early printk device and a system console with
the full capability for sysrq.

After resolving the issue with the last patch in the series work can
progress against adding a debugger interface via this connection as
well.

Thanks,
Jason.


The short log and git info is below, which is against the 2.6.30 development tree.

The following changes since commit a425a638c858fd10370b573bde81df3ba500e271:
are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/jwessel/linux-2.6-kgdb.git for_gregkh

Jason Wessel (5):
      usb_debug: implement multi urb write
      usb_debug,usb_generic_serial: implement sysrq and serial break
      usb,early_printk: insert cr prior to nl as needed
      usb,early_printk: unregister early usb before rest_init()
      usb_debug: EXPERIMENTAL - poll hcd device to force writes

 arch/x86/kernel/early_printk.c |   44 +++++++-
 drivers/usb/core/hcd.c         |    1 +
 drivers/usb/serial/Kconfig     |    7 ++
 drivers/usb/serial/generic.c   |   26 +++--
 drivers/usb/serial/usb_debug.c |  230 ++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/serial.h     |   34 ++++++-
 6 files changed, 323 insertions(+), 19 deletions(-)

[PATCH 1/5] usb_debug: implement multi urb write

[Jason Wessel]

The usb_debug driver, when used as the console, will always fail to
insert the carriage return and new line sequence as well as randomly
drop console output.  This is a result of only having the single
write_urb and that the tty layer will have a lock that prevents the
processing of the back to back urb requests.

The solution is to allow more than one urb to be outstanding and have
a slightly deeper transmit queue.  The idea and some code is borrowed
from the ftdi_sio usb driver.

This patch does not solve the problem of lost console writes during
the kernel boot, but it does allow the usb_debug driver to function as
a usable interactive console.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/serial/usb_debug.c |  177 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 177 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/serial/usb_debug.c b/drivers/usb/serial/usb_debug.c
index 6c9cbb5..ec88c26 100644
--- a/drivers/usb/serial/usb_debug.c
+++ b/drivers/usb/serial/usb_debug.c
@@ -2,6 +2,7 @@
  * USB Debug cable driver
  *
  * Copyright (C) 2006 Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright (C) 2009 Jason Wessel <jason.wessel@windriver.com>
  *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License version
@@ -15,7 +16,9 @@
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
 
+#define URB_UPPER_LIMIT			42
 #define USB_DEBUG_MAX_PACKET_SIZE	8
+static int debug;
 
 static struct usb_device_id id_table [] = {
 	{ USB_DEVICE(0x0525, 0x127a) },
@@ -23,6 +26,11 @@ static struct usb_device_id id_table [] = {
 };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+struct usb_debug_private {
+	spinlock_t tx_lock;
+	unsigned long tx_outstanding_urbs;
+};
+
 static struct usb_driver debug_driver = {
 	.name =		"debug",
 	.probe =	usb_serial_probe,
@@ -38,6 +46,170 @@ static int usb_debug_open(struct tty_struct *tty, struct usb_serial_port *port,
 	return usb_serial_generic_open(tty, port, filp);
 }
 
+static int usb_debug_port_remove(struct usb_serial_port *port)
+{
+	struct udb_debug_private *priv = usb_get_serial_port_data(port);
+
+	if (priv) {
+		usb_set_serial_port_data(port, NULL);
+		kfree(priv);
+	}
+	return 0;
+}
+static int usb_debug_port_probe(struct usb_serial_port *port)
+{
+	struct usb_debug_private *priv;
+
+	priv = kzalloc(sizeof(struct usb_debug_private), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&port->dev, "%s- kmalloc(%Zd) failed.\n", __func__,
+					sizeof(struct usb_debug_private));
+		return -ENOMEM;
+	}
+	spin_lock_init(&priv->tx_lock);
+	if (port->write_urb) {
+		usb_free_urb(port->write_urb);
+		port->write_urb = NULL;
+	}
+	if (port->bulk_out_buffer) {
+		kfree(port->bulk_out_buffer);
+		port->bulk_out_buffer = NULL;
+	}
+
+	usb_set_serial_port_data(port, priv);
+	return 0;
+}
+
+static int usb_debug_write_room(struct tty_struct *tty)
+{
+	unsigned long flags;
+	struct usb_serial_port *port = tty->driver_data;
+	struct usb_debug_private *priv = usb_get_serial_port_data(port);
+	int room = 0;
+
+	dbg("%s - port %d", __func__, port->number);
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	if (priv->tx_outstanding_urbs < URB_UPPER_LIMIT)
+		room = port->bulk_out_size;
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	dbg("%s - returns %d", __func__, room);
+	return room;
+}
+
+static void usb_debug_write_bulk_callback(struct urb *urb)
+{
+	unsigned long flags;
+	struct usb_serial_port *port = urb->context;
+	struct usb_debug_private *priv;
+	int status = urb->status;
+
+	/* free up the transfer buffer, as usb_free_urb() does not do this */
+	kfree(urb->transfer_buffer);
+
+	dbg("%s - port %d", __func__, port->number);
+
+	if (status) {
+		dbg("nonzero write bulk status received: %d", status);
+		return;
+	}
+
+	priv = usb_get_serial_port_data(port);
+	if (!priv) {
+		dbg("%s - bad port private data pointer - exiting", __func__);
+		return;
+	}
+
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	--priv->tx_outstanding_urbs;
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	usb_serial_port_softint(port);
+}
+
+static int usb_debug_write(struct tty_struct *tty,
+	struct usb_serial_port *port, const unsigned char *buf, int count)
+{
+	struct usb_debug_private *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	struct urb *urb;
+	unsigned char *buffer;
+	int status;
+	int towrite;
+	int bwrite = 0;
+
+	dbg("%s - port %d", __func__, port->number);
+
+	if (count == 0)
+		dbg("%s - write request of 0 bytes", __func__);
+
+	while (count > 0) {
+		spin_lock_irqsave(&priv->tx_lock, flags);
+		if (priv->tx_outstanding_urbs > URB_UPPER_LIMIT) {
+			spin_unlock_irqrestore(&priv->tx_lock, flags);
+			dbg("%s - write limit hit\n", __func__);
+			return bwrite;
+		}
+
+		priv->tx_outstanding_urbs++;
+		spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+		towrite = (count > port->bulk_out_size) ?
+			port->bulk_out_size : count;
+
+		buffer = kmalloc(towrite, GFP_ATOMIC);
+		if (!buffer) {
+			dev_err(&port->dev,
+			"%s ran out of kernel memory for urb ...\n", __func__);
+			goto error_no_buffer;
+		}
+
+		urb = usb_alloc_urb(0, GFP_ATOMIC);
+		if (!urb) {
+			dev_err(&port->dev, "%s - no more free urbs\n",
+				__func__);
+			goto error_no_urb;
+		}
+
+		/* Copy data */
+		memcpy(buffer, buf + bwrite, towrite);
+		usb_serial_debug_data(debug, &port->dev, __func__,
+				      towrite, buffer);
+		/* fill the buffer and send it */
+		usb_fill_bulk_urb(urb, port->serial->dev,
+			usb_sndbulkpipe(port->serial->dev,
+					port->bulk_out_endpointAddress),
+			buffer, towrite,
+			usb_debug_write_bulk_callback, port);
+
+		status = usb_submit_urb(urb, GFP_ATOMIC);
+		if (status) {
+			dev_err(&port->dev,
+				"%s - failed submitting write urb, error %d\n",
+				__func__, status);
+			goto error;
+		}
+
+		/* This urb is the responsibility of the host driver now */
+		usb_free_urb(urb);
+		dbg("%s write: %d", __func__, towrite);
+		count -= towrite;
+		bwrite += towrite;
+	}
+	return bwrite;
+
+error:
+	usb_free_urb(urb);
+error_no_urb:
+	kfree(buffer);
+error_no_buffer:
+	spin_lock_irqsave(&priv->tx_lock, flags);
+	priv->tx_outstanding_urbs--;
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+	return bwrite;
+}
+
 static struct usb_serial_driver debug_device = {
 	.driver = {
 		.owner =	THIS_MODULE,
@@ -46,6 +218,11 @@ static struct usb_serial_driver debug_device = {
 	.id_table =		id_table,
 	.num_ports =		1,
 	.open =			usb_debug_open,
+	.port_probe = 		usb_debug_port_probe,
+	.port_remove = 		usb_debug_port_remove,
+	.write = 		usb_debug_write,
+	.write_bulk_callback = 	usb_debug_write_bulk_callback,
+	.write_room = 		usb_debug_write_room,
 };
 
 static int __init debug_init(void)
-- 
1.6.3.rc0.1.gf800

[PATCH 2/5] usb_debug,usb_generic_serial: implement sysrq and serial break

[Jason Wessel]

The usb_debug driver was modified to implement serial break handling
by using a "magic" data packet comprised of the sequence:

       0x00 0xff 0x01 0xfe   0x00 0xfe 0x01 0xff

When the tty layer requests a serial break the usb_debug driver sends
the magic packet.  On the receiving side the magic packet is thrown
away or a sysrq is activated depending on what kernel .config options
have been set.

The generic serial driver was modified as well as the usb serial
headers to generically implement sysrq processing in the same way the
non usb uart based drivers implement the sysrq handling.  This will
allow other usb serial devices to implement sysrq handling as desired.

The new usb serial functions are named similarly and implemented
similarly to the uart functions as follows:

usb_serial_handle_break <-> uart_handle_break
usb_serial_handle_sysrq_char <-> uart_handle_sysrq_char

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/serial/Kconfig     |    7 +++++++
 drivers/usb/serial/generic.c   |   26 +++++++++++++++-----------
 drivers/usb/serial/usb_debug.c |   39 +++++++++++++++++++++++++++++++++++++++
 include/linux/usb/serial.h     |   34 ++++++++++++++++++++++++++++++++--
 4 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index c480ea4..3229321 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -42,6 +42,13 @@ config USB_SERIAL_CONSOLE
 
 	  If unsure, say N.
 
+config USB_SERIAL_SYSRQ
+       bool "Support inbound sysrq"
+       depends on USB_SERIAL_CONSOLE && MAGIC_SYSRQ
+       help
+         Say y here to enable support for sysrq if the usb serial
+         driver implements support for sysrq.
+
 config USB_EZUSB
 	bool "Functions for loading firmware on EZUSB chips"
 	help
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 4cec990..a4fe909 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -291,7 +291,8 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct *tty)
 }
 
 
-static void resubmit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+void usb_serial_generic_resubmit_read_urb(struct usb_serial_port *port,
+			gfp_t mem_flags)
 {
 	struct urb *urb = port->read_urb;
 	struct usb_serial *serial = port->serial;
@@ -312,25 +313,28 @@ static void resubmit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
 			"%s - failed resubmitting read urb, error %d\n",
 							__func__, result);
 }
+EXPORT_SYMBOL_GPL(usb_serial_generic_resubmit_read_urb);
 
 /* Push data to tty layer and resubmit the bulk read URB */
 static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
 {
 	struct urb *urb = port->read_urb;
 	struct tty_struct *tty = tty_port_tty_get(&port->port);
-	int room;
+	char *ch = (char *)urb->transfer_buffer;
+	int i;
+
+	if (!tty)
+		goto done;
 
 	/* Push data to tty */
-	if (tty && urb->actual_length) {
-		room = tty_buffer_request_room(tty, urb->actual_length);
-		if (room) {
-			tty_insert_flip_string(tty, urb->transfer_buffer, room);
-			tty_flip_buffer_push(tty);
-		}
+	for (i = 0; i < urb->actual_length; i++, ch++) {
+		if (!usb_serial_handle_sysrq_char(port, *ch))
+			tty_insert_flip_char(tty, *ch, TTY_NORMAL);
 	}
+	tty_flip_buffer_push(tty);
 	tty_kref_put(tty);
-
-	resubmit_read_urb(port, GFP_ATOMIC);
+done:
+	usb_serial_generic_resubmit_read_urb(port, GFP_ATOMIC);
 }
 
 void usb_serial_generic_read_bulk_callback(struct urb *urb)
@@ -409,7 +413,7 @@ void usb_serial_generic_unthrottle(struct tty_struct *tty)
 
 	if (was_throttled) {
 		/* Resume reading from device */
-		resubmit_read_urb(port, GFP_KERNEL);
+		usb_serial_generic_resubmit_read_urb(port, GFP_KERNEL);
 	}
 }
 
diff --git a/drivers/usb/serial/usb_debug.c b/drivers/usb/serial/usb_debug.c
index ec88c26..58feab9 100644
--- a/drivers/usb/serial/usb_debug.c
+++ b/drivers/usb/serial/usb_debug.c
@@ -18,6 +18,17 @@
 
 #define URB_UPPER_LIMIT			42
 #define USB_DEBUG_MAX_PACKET_SIZE	8
+#define USB_DEBUG_BRK_SIZE		8
+static char USB_DEBUG_BRK[8] = {
+	0x00,
+	0xff,
+	0x01,
+	0xfe,
+	0x00,
+	0xfe,
+	0x01,
+	0xff,
+};
 static int debug;
 
 static struct usb_device_id id_table [] = {
@@ -210,6 +221,32 @@ error_no_buffer:
 	return bwrite;
 }
 
+/* This HW really does not support a serial break, so one will be
+ * emulated when ever the break state is set to true.
+ */
+static void usb_debug_break_ctl(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	if (!break_state)
+		return;
+	usb_debug_write(tty, port, USB_DEBUG_BRK, USB_DEBUG_BRK_SIZE);
+}
+
+static void usb_debug_read_bulk_callback(struct urb *urb)
+{
+	struct usb_serial_port *port = urb->context;
+
+	if (urb->actual_length == USB_DEBUG_BRK_SIZE &&
+	    memcmp(urb->transfer_buffer, USB_DEBUG_BRK,
+		   USB_DEBUG_BRK_SIZE) == 0) {
+		usb_serial_handle_break(port);
+		usb_serial_generic_resubmit_read_urb(port, GFP_ATOMIC);
+		return;
+	}
+
+	usb_serial_generic_read_bulk_callback(urb);
+}
+
 static struct usb_serial_driver debug_device = {
 	.driver = {
 		.owner =	THIS_MODULE,
@@ -223,6 +260,8 @@ static struct usb_serial_driver debug_device = {
 	.write = 		usb_debug_write,
 	.write_bulk_callback = 	usb_debug_write_bulk_callback,
 	.write_room = 		usb_debug_write_room,
+	.break_ctl =			usb_debug_break_ctl,
+	.read_bulk_callback =		usb_debug_read_bulk_callback,
 };
 
 static int __init debug_init(void)
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 625e9e4..b00cb54 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -15,6 +15,7 @@
 
 #include <linux/kref.h>
 #include <linux/mutex.h>
+#include <linux/sysrq.h>
 
 #define SERIAL_TTY_MAJOR	188	/* Nice legal number now */
 #define SERIAL_TTY_MINORS	254	/* loads of devices :) */
@@ -96,6 +97,9 @@ struct usb_serial_port {
 	char			throttled;
 	char			throttle_req;
 	char			console;
+#ifdef CONFIG_USB_SERIAL_SYSRQ
+	unsigned long		sysrq; /* sysrq timeout */
+#endif /* CONFIG_USB_SERIAL_SYSRQ */
 	struct device		dev;
 };
 #define to_usb_serial_port(d) container_of(d, struct usb_serial_port, dev)
@@ -295,6 +299,8 @@ extern void usb_serial_generic_unthrottle(struct tty_struct *tty);
 extern void usb_serial_generic_shutdown(struct usb_serial *serial);
 extern int usb_serial_generic_register(int debug);
 extern void usb_serial_generic_deregister(void);
+void usb_serial_generic_resubmit_read_urb(struct usb_serial_port *port,
+			gfp_t mem_flags);
 
 extern int usb_serial_bus_register(struct usb_serial_driver *device);
 extern void usb_serial_bus_deregister(struct usb_serial_driver *device);
@@ -328,7 +334,31 @@ static inline void usb_serial_debug_data(int debug,
 				## arg); \
 	} while (0)
 
-
-
+static inline int
+usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
+{
+#ifdef CONFIG_USB_SERIAL_SYSRQ
+	if (port->sysrq) {
+		if (ch && time_before(jiffies, port->sysrq)) {
+			handle_sysrq(ch, tty_port_tty_get(&port->port));
+			port->sysrq = 0;
+			return 1;
+		}
+		port->sysrq = 0;
+	}
+#endif /* CONFIG_USB_SERIAL_SYSRQ */
+	return 0;
+}
+static inline int usb_serial_handle_break(struct usb_serial_port *port)
+{
+#ifdef CONFIG_USB_SERIAL_SYSRQ
+	if (!port->sysrq) {
+		port->sysrq = jiffies + HZ*5;
+		return 1;
+	}
+	port->sysrq = 0;
+#endif /* CONFIG_USB_SERIAL_SYSRQ */
+	return 0;
+}
 #endif /* __LINUX_USB_SERIAL_H */
 
-- 
1.6.3.rc0.1.gf800

[PATCH 3/5] usb,early_printk: insert cr prior to nl as needed

[Jason Wessel]

The rs232 drivers send a carriage return prior to a new line in the
early printk code.

The usb debug driver should do the same because you want to be able to
use the same terminal programs and tools for analysis of early printk
data.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 arch/x86/kernel/early_printk.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 335f049..e38c467 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -853,17 +853,27 @@ static int __init early_dbgp_init(char *s)
 static void early_dbgp_write(struct console *con, const char *str, u32 n)
 {
 	int chunk, ret;
+	char buf[DBGP_MAX_PACKET];
+	int use_cr = 0;
 
 	if (!ehci_debug)
 		return;
 	while (n > 0) {
-		chunk = n;
-		if (chunk > DBGP_MAX_PACKET)
-			chunk = DBGP_MAX_PACKET;
+		for (chunk = 0; chunk < DBGP_MAX_PACKET && n > 0;
+		     str++, chunk++, n--) {
+			if (!use_cr && *str == '\n') {
+				use_cr = 1;
+				buf[chunk] = '\r';
+				str--;
+				n++;
+				continue;
+			}
+			if (use_cr)
+				use_cr = 0;
+			buf[chunk] = *str;
+		}
 		ret = dbgp_bulk_write(USB_DEBUG_DEVNUM,
-			dbgp_endpoint_out, str, chunk);
-		str += chunk;
-		n -= chunk;
+			dbgp_endpoint_out, buf, chunk);
 	}
 }
 
-- 
1.6.3.rc0.1.gf800

[PATCH 4/5] usb,early_printk: unregister early usb before rest_init()

[Jason Wessel]

The early_printk EHCI debug driver can hang the system or cause
unpredictable results because two separate APIs will attempt to write
to the mapped PCI region.

The safe route is to unregister the early USB console at the end of
the core kernel init, which is before the PCI initialization.  Later,
after the USB initialization completes it is then possible to register
the console against the usb debug device and receive the printk
messages that were missed in the period between early printk
availability and the USB console device.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 arch/x86/kernel/early_printk.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index e38c467..ef177d4 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -952,4 +952,26 @@ static int __init setup_early_printk(char *buf)
 	return 0;
 }
 
+#ifdef CONFIG_EARLY_PRINTK_DBGP
+/*
+ * The early console must get unregistered prior to the ECHI
+ * controller getting reset else the debug device cannot be used until
+ * a subsequent EHCI reset, or the kernel hangs.
+ */
+static int __init usb_early_debug_cleanup(void)
+{
+	if (early_dbgp_console.index >= 0 &&
+		early_dbgp_console.flags & CON_PRINTBUFFER) {
+		console_stop(early_console);
+		unregister_console(early_console);
+		early_dbgp_console.index = -1;
+		early_dbgp_console.flags = 0;
+		ehci_debug = NULL;
+		printk(KERN_INFO "Early USB console unregistered\n");
+	}
+	return 0;
+}
+postcore_initcall(usb_early_debug_cleanup);
+#endif /* CONFIG_EARLY_PRINTK_DBGP */
+
 early_param("earlyprintk", setup_early_printk);
-- 
1.6.3.rc0.1.gf800

[PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Jason Wessel]

The problem that this patch tries to solve is that data is lost
because there are too many outstanding transmit urb's.

The question is what is the right way to force the controller to catch
up, and ideally let the kernel do some other things in the mean time?

This patch takes the route of forcibly polling the hcd device to drain
the urb queue and initiate the bulk write call backs.

NOTE this patch is not signed off, it is a question of what is the
right way to do this?
---
 drivers/usb/core/hcd.c         |    1 +
 drivers/usb/serial/usb_debug.c |   23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 42b93da..57c09c2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1760,6 +1760,7 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
 	local_irq_restore(flags);
 	return rc;
 }
+EXPORT_SYMBOL_GPL(usb_hcd_irq);
 
 /*-------------------------------------------------------------------------*/
 
diff --git a/drivers/usb/serial/usb_debug.c b/drivers/usb/serial/usb_debug.c
index 58feab9..8838418 100644
--- a/drivers/usb/serial/usb_debug.c
+++ b/drivers/usb/serial/usb_debug.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
+#include "../core/hcd.h"
 
 #define URB_UPPER_LIMIT			42
 #define USB_DEBUG_MAX_PACKET_SIZE	8
@@ -91,6 +92,23 @@ static int usb_debug_port_probe(struct usb_serial_port *port)
 	return 0;
 }
 
+/* If the driver is close to running of tx urb's try to free some up
+ * by asking the hcd device to do some processing.  XXX This needs
+ * review as to if this is a reasonable way to implement forcing the
+ * HCD device to run.
+ */
+static void poll_hcd_irq(struct urb *urb)
+{
+	struct usb_hcd *hcd;
+
+	if (!urb)
+		return;
+
+	hcd = bus_to_hcd(urb->dev->bus);
+	if (hcd)
+		usb_hcd_irq(0, hcd);
+}
+
 static int usb_debug_write_room(struct tty_struct *tty)
 {
 	unsigned long flags;
@@ -156,10 +174,15 @@ static int usb_debug_write(struct tty_struct *tty,
 		dbg("%s - write request of 0 bytes", __func__);
 
 	while (count > 0) {
+		int loops = 1000;
+try_again:
 		spin_lock_irqsave(&priv->tx_lock, flags);
 		if (priv->tx_outstanding_urbs > URB_UPPER_LIMIT) {
 			spin_unlock_irqrestore(&priv->tx_lock, flags);
 			dbg("%s - write limit hit\n", __func__);
+			poll_hcd_irq(port->read_urb);
+			if (loops-- > 0)
+				goto try_again;
 			return bwrite;
 		}
 
-- 
1.6.3.rc0.1.gf800

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Stern]

On Tue, 5 May 2009, Jason Wessel wrote:

> The problem that this patch tries to solve is that data is lost
> because there are too many outstanding transmit urb's.
> 
> The question is what is the right way to force the controller to catch
> up, and ideally let the kernel do some other things in the mean time?

What do standard serial port drivers do?

> This patch takes the route of forcibly polling the hcd device to drain
> the urb queue and initiate the bulk write call backs.
> 
> NOTE this patch is not signed off, it is a question of what is the
> right way to do this?

This patch is highly questionable.

Is the idea to force the HCD to search for completed URBs before the
host controller has issued a completion interrupt?  Wouldn't it be
better instead to use more URBs?

Besides, why should there be too many outstanding transmit URBs?  A
normal serial debugging port running at 115200 baud can transmit no
more than 12 bytes per ms.  You should be able to surpass that using
only three URBs!  In fact, if you buffer up to 8 bytes per URB then
with only two URBs you could send 64 bytes per ms, which is equivalent
to 640000 baud.  Do you really need to go more than (say) ten times
faster than that?

Even if people do agree this is a reasonable thing to do (which I
doubt), the poll_hcd_irq() code doesn't belong hidden away in
serial/usb_debug.c.  It belongs in core/hcd.c.

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Greg KH]

On Wed, May 06, 2009 at 11:18:07AM -0400, Alan Stern wrote:
> On Tue, 5 May 2009, Jason Wessel wrote:
> 
> > The problem that this patch tries to solve is that data is lost
> > because there are too many outstanding transmit urb's.
> > 
> > The question is what is the right way to force the controller to catch
> > up, and ideally let the kernel do some other things in the mean time?
> 
> What do standard serial port drivers do?

Not this :)

They use the proper flow control hooks that the tty layer provides.

> > This patch takes the route of forcibly polling the hcd device to drain
> > the urb queue and initiate the bulk write call backs.
> > 
> > NOTE this patch is not signed off, it is a question of what is the
> > right way to do this?
> 
> This patch is highly questionable.

I agree.

> Is the idea to force the HCD to search for completed URBs before the
> host controller has issued a completion interrupt?  Wouldn't it be
> better instead to use more URBs?
> 
> Besides, why should there be too many outstanding transmit URBs?  A
> normal serial debugging port running at 115200 baud can transmit no
> more than 12 bytes per ms.  You should be able to surpass that using
> only three URBs!  In fact, if you buffer up to 8 bytes per URB then
> with only two URBs you could send 64 bytes per ms, which is equivalent
> to 640000 baud.  Do you really need to go more than (say) ten times
> faster than that?

Yeah, something seems wrong here.

Jason, why is this really needed?  With your ring buffer, you shouldn't
need this at all anymore.

Or if you do, just bump up the number outstanding urbs that are
possible.  Or the urb buffer size.

This patch isn't acceptable as-is.

thanks,

greg k-h

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Cox]

> Or if you do, just bump up the number outstanding urbs that are
> possible.  Or the urb buffer size.

Simply growing the urb size and urb count isn't the right way to
fix this, it just uses more memory to pretend to paper over it.

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Jason Wessel]

Greg KH wrote:
>>> This patch takes the route of forcibly polling the hcd device to drain
>>> the urb queue and initiate the bulk write call backs.
>>>
>>> NOTE this patch is not signed off, it is a question of what is the
>>> right way to do this?
>>>       
>> This patch is highly questionable.
>>     
>
> I agree.
>
>   

That is why there is no signed-off on this patch.  It is an
RFC/EXPERIMENTAL because it is not clear how to fix the problem.  See
below for more discussion.

>> Is the idea to force the HCD to search for completed URBs before the
>> host controller has issued a completion interrupt?  Wouldn't it be
>> better instead to use more URBs?
>>
>> Besides, why should there be too many outstanding transmit URBs?  A
>> normal serial debugging port running at 115200 baud can transmit no
>> more than 12 bytes per ms.  You should be able to surpass that using
>> only three URBs!  In fact, if you buffer up to 8 bytes per URB then
>> with only two URBs you could send 64 bytes per ms, which is equivalent
>> to 640000 baud.  Do you really need to go more than (say) ten times
>> faster than that?
>>     
>
> Yeah, something seems wrong here.
>
> Jason, why is this really needed?  With your ring buffer, you shouldn't
> need this at all anymore.
>
> Or if you do, just bump up the number outstanding urbs that are
> possible.  Or the urb buffer size.
>
>   

The URB queue has to be unacceptably large to make it work correctly
(>4000)   The issue here stems from the register_console() function.  
As a part of the registration with register_console, the usb_debug
device driver gets a huge flood of printk backlog.  Because the urb
queue is not that deep we loose valuable printk logs.  I would very much
like a way to force it to work correctly.

To answer Alan's question, the rs232 uart drivers don't process the
printk back log asynchronously.  It is a direct write to the hardware
(see serial8250_console_putchar - drivers/serial/8250.c).   That is why
there is no problem with lost printk data in the case of the uart drivers.

I could find no obvious safe way do to this synchronously with the USB
api, which is why I put together some way to "force it to work" with the
poll hook until we can figure out the right way to do this.  The
mechanism for a console write is not the same as the standard tty
service, or at least that is the way it is today in the kernel.

Essentially I am seeking a synchronous write and wait for the usb serial
drivers when used as a system console.  Right now in the usb serial API
there is no distinction between a console write and a tty write. 
Perhaps that is what needs to change, to allow for some synchronous
behavior in usb_console_write()?

The usb_debug driver is not the only usb serial device that loses data
from the printk backlog, the ftdi_sio driver suffers the same problem.

Jason.

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Cox]

> > The question is what is the right way to force the controller to catch
> > up, and ideally let the kernel do some other things in the mean time?
> 
> What do standard serial port drivers do?

Stuff data into the chip and if need be maintain an internal buffer. One
of the problems with the serial stuff is that the buffering algorithms
you need to packetize serial streams without poor behaviour or overruns
are non trivial and the USB serial layer doesn't provide anything
remotely resembling sanity.

Secondly most USB serial drivers lie about their write room which causes
overruns.

What should happen is something like this

	foo_write_room()
	{
		if(no_urbs_left) return 0;
		if(one_urb_left) return bytes_in_urb_left;
		else return size_of_one_urb;
	}

	foo_write()
	{
		stuff bytes into urb
		if (urbs_left > 1 &&
		some_function_of_bytes_queued_and_speed())
			send_urb_now();
	}

In other words never lie in the write_room method. Try to send data as
early as possible, but don't queue data inefficiently when the port has
plenty left to send.

The rules for write_room are simple

	If you say you have space for X bytes you can't say you have room
	for less bytes unless those bytes have actually been written - no
	window shrinking so to speak.

If your write_room method works properly, and you fix the buffering logic
then the rest will work. Probably we need usb/serial/buffer.c which has a
generic implementation in then that can be used to replace the completely
bogus hacks in most of the drivers.

Alan

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Greg KH]

On Wed, May 06, 2009 at 04:41:41PM +0100, Alan Cox wrote:
> In other words never lie in the write_room method. Try to send data as
> early as possible, but don't queue data inefficiently when the port has
> plenty left to send.
> 
> The rules for write_room are simple
> 
> 	If you say you have space for X bytes you can't say you have room
> 	for less bytes unless those bytes have actually been written - no
> 	window shrinking so to speak.
> 
> If your write_room method works properly, and you fix the buffering logic
> then the rest will work. Probably we need usb/serial/buffer.c which has a
> generic implementation in then that can be used to replace the completely
> bogus hacks in most of the drivers.

I agree, this is the correct solution.

thanks,

greg k-h

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Oliver Neukum]

Am Mittwoch, 6. Mai 2009 17:41:41 schrieb Alan Cox:
> Stuff data into the chip and if need be maintain an internal buffer. One
> of the problems with the serial stuff is that the buffering algorithms
> you need to packetize serial streams without poor behaviour or overruns
> are non trivial and the USB serial layer doesn't provide anything
> remotely resembling sanity.

Something a bit a like this:

commit a02639fe7d3f9788263305cff0669eac91f54002
Author: Oliver Neukum <oneukum@linux-d698.(none)>
Date:   Wed May 6 19:14:30 2009 +0200

    terrible implementation of usb serial write buffering

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3a09777..83acde4 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -801,8 +801,13 @@ kevent (struct work_struct *work)
 	if (test_bit(EVENT_DEV_WAKING, &dev->flags)) {
 		status = usb_autopm_get_interface(dev->intf);
 		clear_bit(EVENT_DEV_WAKING, &dev->flags);
-		if (!status)
+		if (!status) {
 			usb_autopm_put_interface(dev->intf);
+		} else {
+			dev_kfree_skb_any((struct sk_buff*)dev->deferred->context);
+			usb_free_urb(dev->deferred);
+			netif_start_queue(dev->net);
+		}
 	}
 	/* usb_clear_halt() needs a thread context */
 	if (test_bit (EVENT_TX_HALT, &dev->flags)) {
@@ -1351,12 +1356,13 @@ int usbnet_resume (struct usb_interface *intf)
 		clear_bit(EVENT_DEV_ASLEEP, &dev->flags);
 		spin_unlock_irq(&dev->txq.lock);
 		if (res) {
+			skb = (struct sk_buff *)res->context;
 			retval = usb_submit_urb(res, GFP_NOIO);
 			if (retval < 0) {
+				dev_kfree_skb_any(skb);
 				usb_free_urb(res);
 				netif_start_queue(dev->net);
 			} else {
-				skb = (struct sk_buff *)res->context;
 				dev->net->trans_start = jiffies;
 				__skb_queue_tail (&dev->txq, skb);
 				if (!(dev->txq.qlen >= TX_QLEN(dev)))
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 4cec990..31dcbf7 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -191,12 +191,14 @@ void usb_serial_generic_close(struct tty_struct *tty,
 	generic_cleanup(port);
 }
 
+#define UPPER_WRITE_LIMIT 4
 int usb_serial_generic_write(struct tty_struct *tty,
 	struct usb_serial_port *port, const unsigned char *buf, int count)
 {
 	struct usb_serial *serial = port->serial;
+	struct urb *write_urb;
 	int result;
-	unsigned char *data;
+	u8 *data;
 
 	dbg("%s - port %d", __func__, port->number);
 
@@ -209,41 +211,66 @@ int usb_serial_generic_write(struct tty_struct *tty,
 	if (serial->num_bulk_out) {
 		unsigned long flags;
 		spin_lock_irqsave(&port->lock, flags);
-		if (port->write_urb_busy) {
+		if (port->writes_in_flight >= UPPER_WRITE_LIMIT) {
+			if (!port->reserve_buffer) {
+				spin_unlock_irqrestore(&port->lock, flags);
+				return 0;
+			}
+			count = (count > port->bulk_out_size - port->reserve_filled) ?
+					port->bulk_out_size - port->reserve_filled
+					: count;
+			memcpy(port->reserve_buffer + port->reserve_filled,
+				buf, count);
+			port->reserve_filled += count;
 			spin_unlock_irqrestore(&port->lock, flags);
-			dbg("%s - already writing", __func__);
-			return 0;
+			dbg("%s - hitting reserves %d bytes left",
+				__func__, port->bulk_out_size - port->reserve_filled);
+			return count;
+		} else {
+			count = (count > port->bulk_out_size) ?
+					port->bulk_out_size : count;		
+			write_urb = usb_alloc_urb(0, GFP_ATOMIC);
+			if (!write_urb) {
+				spin_unlock_irqrestore(&port->lock, flags);
+				return 0;
+			}
+			data = kmalloc(count, GFP_ATOMIC);
+			if (!data) {
+				usb_free_urb(write_urb);
+				spin_unlock_irqrestore(&port->lock, flags);
+				return 0;
+			}
 		}
-		port->write_urb_busy = 1;
+		port->writes_in_flight++;
 		spin_unlock_irqrestore(&port->lock, flags);
 
-		count = (count > port->bulk_out_size) ?
-					port->bulk_out_size : count;
-
-		memcpy(port->write_urb->transfer_buffer, buf, count);
-		data = port->write_urb->transfer_buffer;
+		memcpy(data, buf, count);
+		write_urb->transfer_buffer = data;
 		usb_serial_debug_data(debug, &port->dev, __func__, count, data);
 
 		/* set up our urb */
-		usb_fill_bulk_urb(port->write_urb, serial->dev,
+		usb_fill_bulk_urb(write_urb, serial->dev,
 				   usb_sndbulkpipe(serial->dev,
 					port->bulk_out_endpointAddress),
-				   port->write_urb->transfer_buffer, count,
+				   data, count,
 				   ((serial->type->write_bulk_callback) ?
 				     serial->type->write_bulk_callback :
 				     usb_serial_generic_write_bulk_callback),
 				   port);
 
 		/* send the data out the bulk port */
-		port->write_urb_busy = 1;
-		result = usb_submit_urb(port->write_urb, GFP_ATOMIC);
+		result = usb_submit_urb(write_urb, GFP_ATOMIC);
 		if (result) {
 			dev_err(&port->dev,
 				"%s - failed submitting write urb, error %d\n",
 							__func__, result);
 			/* don't have to grab the lock here, as we will
 			   retry if != 0 */
-			port->write_urb_busy = 0;
+			kfree(data);
+			usb_free_urb(write_urb);
+			spin_lock_irqsave(&port->lock, flags);
+			port->writes_in_flight--;
+			spin_unlock_irqrestore(&port->lock, flags);
 		} else
 			result = count;
 
@@ -264,7 +291,7 @@ int usb_serial_generic_write_room(struct tty_struct *tty)
 
 	/* FIXME: Locking */
 	if (serial->num_bulk_out) {
-		if (!(port->write_urb_busy))
+		if (port->writes_in_flight < UPPER_WRITE_LIMIT)
 			room = port->bulk_out_size;
 	}
 
@@ -282,8 +309,10 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct 
*tty)
 
 	/* FIXME: Locking */
 	if (serial->num_bulk_out) {
-		if (port->write_urb_busy)
-			chars = port->write_urb->transfer_buffer_length;
+		if (port->writes_in_flight < UPPER_WRITE_LIMIT)
+			chars = port->bulk_out_size * UPPER_WRITE_LIMIT / 2;
+		else
+			chars = port->bulk_out_size * UPPER_WRITE_LIMIT;
 	}
 
 	dbg("%s - returns %d", __func__, chars);
@@ -369,7 +398,12 @@ void usb_serial_generic_write_bulk_callback(struct urb 
*urb)
 
 	dbg("%s - port %d", __func__, port->number);
 
-	port->write_urb_busy = 0;
+	kfree(urb->transfer_buffer);
+	spin_lock(&port->lock);
+	if (!port->reserve_filled || status)
+		port->writes_in_flight--;
+	spin_unlock(&port->lock);
+
 	if (status) {
 		dbg("%s - nonzero write bulk status received: %d",
 		    __func__, status);
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 0a566ee..33d6ea4 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -519,10 +519,49 @@ static void usb_serial_port_work(struct work_struct 
*work)
 {
 	struct usb_serial_port *port =
 		container_of(work, struct usb_serial_port, work);
+	struct usb_serial *serial = port->serial;
+	struct urb *urb;
 	struct tty_struct *tty;
+	u8 *new_reserve;
+	int err;
+	unsigned long flags;
 
 	dbg("%s - port %d", __func__, port->number);
 
+	if (port->reserve_filled) {
+		/*
+		 * this cannot be done any later and here we can sleep
+		 * to allocate memory
+		 */
+		new_reserve = kmalloc(port->bulk_out_size, GFP_KERNEL);
+		urb = usb_alloc_urb(0, GFP_KERNEL);
+		if (!new_reserve || !urb) {
+			/* now we drop data */
+			kfree(new_reserve);
+failure:
+			usb_free_urb(urb);
+			spin_lock_irqsave(&port->lock, flags);
+			port->writes_in_flight--;
+			port->reserve_filled = 0;
+			spin_unlock_irqrestore(&port->lock, flags);
+			goto bail_buffer_exhaustion;
+		}
+		urb->transfer_buffer = port->reserve_buffer;
+		usb_fill_bulk_urb(urb, serial->dev,
+				   usb_rcvbulkpipe(serial->dev,
+						port->bulk_in_endpointAddress),
+				   port->reserve_buffer,
+				   port->reserve_filled,
+				   usb_serial_generic_write_bulk_callback,
+				   port);
+		port->reserve_buffer = new_reserve;
+		err = usb_submit_urb(urb, GFP_KERNEL);
+		if (err < 0)
+			goto failure;
+		port->reserve_filled = 0;
+	}
+
+bail_buffer_exhaustion:
 	tty = tty_port_tty_get(&port->port);
 	if (!tty)
 		return;
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 625e9e4..4e0afed 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -88,7 +88,9 @@ struct usb_serial_port {
 	unsigned char		*bulk_out_buffer;
 	int			bulk_out_size;
 	struct urb		*write_urb;
-	int			write_urb_busy;
+	int			writes_in_flight;
+	int			reserve_filled;
+	u8			*reserve_buffer;
 	__u8			bulk_out_endpointAddress;
 
 	wait_queue_head_t	write_wait;

Just for discussion.

	Regards
		Oliver

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Stern]

On Wed, 6 May 2009, Oliver Neukum wrote:

> Something a bit a like this:
> 
> commit a02639fe7d3f9788263305cff0669eac91f54002
> Author: Oliver Neukum <oneukum@linux-d698.(none)>
> Date:   Wed May 6 19:14:30 2009 +0200
> 
>     terrible implementation of usb serial write buffering

For algorithm discussions like this, I find reading code rather
difficult.  English or pseudo-code presentations are a lot more
intelligible.

A little thought yielded the following algorithm.  It assumes there is
a fixed set of URBs allocated, unlike what you have done.  Does it make
sense to take this approach?

Let N be the total number of URBs allocated, each capable of holding up 
to B bytes.  Let NIF be the number of URBs in flight at any time, so 
the number of available URBs is N - NIF.  The number of available bytes 
might be < (N - NIF)*B because the next URB might be partially full.

P is an adjustable parameter of the algorithm.  For simplicity you can
take P = 1, but increasing P (any value below N is okay) would yield
reduced latency at the cost of more partially-filled URB submissions
(so possibly reduced throughput).

    Write routine:
	Copy bytes into the available URB buffers, submitting URBs as 
	they get filled.  At the end, if the next URB is partially full 
	then submit it only if NIF < P.

    Completion routine:
	If the next URB to send is partially filled, submit it.

    write_room routine:
	Return the actual number of bytes remaining in the available 
	URBs, but no more than (N-P)*B.

How does that sound?  Converting \n to \r\n will add some complication 
but not too much.  

Allocating URBs on the fly adds a lot of complication.  There has to be 
a minimum number of pre-allocated URBs; otherwise write_room could 
never return a positive value.  If you allocate additional URBs 
later on, when would you free them?

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Oliver Neukum]

Am Mittwoch, 6. Mai 2009 21:24:56 schrieb Alan Stern:
> On Wed, 6 May 2009, Oliver Neukum wrote:

> A little thought yielded the following algorithm.  It assumes there is
> a fixed set of URBs allocated, unlike what you have done.  Does it make

No, it does not ;-) Your approach is more general than you think.
The only important constraint is that the number of URBs in flight
be limited. It doesn't matter when they are allocated.

> sense to take this approach?

Yes.

> Let N be the total number of URBs allocated, each capable of holding up
> to B bytes.  Let NIF be the number of URBs in flight at any time, so
> the number of available URBs is N - NIF.  The number of available bytes
> might be < (N - NIF)*B because the next URB might be partially full.
>
> P is an adjustable parameter of the algorithm.  For simplicity you can
> take P = 1, but increasing P (any value below N is okay) would yield
> reduced latency at the cost of more partially-filled URB submissions
> (so possibly reduced throughput).
>
>     Write routine:
> 	Copy bytes into the available URB buffers, submitting URBs as
> 	they get filled.  At the end, if the next URB is partially full
> 	then submit it only if NIF < P.

I did so. In principle. I am leaving the iteration to the tty layer.

>     Completion routine:
> 	If the next URB to send is partially filled, submit it.

Much easier if P = N - 1

>     write_room routine:
> 	Return the actual number of bytes remaining in the available
> 	URBs, but no more than (N-P)*B.

Yes.

> How does that sound?  Converting \n to \r\n will add some complication
> but not too much.
>
> Allocating URBs on the fly adds a lot of complication.  There has to be
> a minimum number of pre-allocated URBs; otherwise write_room could

Why? You can always calculate with the number of URBs you'd
allocate as a maximum.

> never return a positive value.  If you allocate additional URBs
> later on, when would you free them?

Like you free all URBs, on completion.

	Regards
		Oliver

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Stern]

On Wed, 6 May 2009, Oliver Neukum wrote:

> Am Mittwoch, 6. Mai 2009 21:24:56 schrieb Alan Stern:
> > On Wed, 6 May 2009, Oliver Neukum wrote:
> 
> > A little thought yielded the following algorithm.  It assumes there is
> > a fixed set of URBs allocated, unlike what you have done.  Does it make
> 
> No, it does not ;-) Your approach is more general than you think.
> The only important constraint is that the number of URBs in flight
> be limited. It doesn't matter when they are allocated.

You're optimistically assuming that URB allocations will succeed.  I 
guess that's okay -- dropping characters when there's insufficient 
memory seems like a good thing to do.

What's the point of that "reserve_buffer" thing?  Why not just use the 
next URB's transfer buffer?

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Oliver Neukum]

Am Mittwoch, 6. Mai 2009 22:24:13 schrieb Alan Stern:
> On Wed, 6 May 2009, Oliver Neukum wrote:
> > Am Mittwoch, 6. Mai 2009 21:24:56 schrieb Alan Stern:
> > > On Wed, 6 May 2009, Oliver Neukum wrote:
> > >
> > > A little thought yielded the following algorithm.  It assumes there is
> > > a fixed set of URBs allocated, unlike what you have done.  Does it make
> >
> > No, it does not ;-) Your approach is more general than you think.
> > The only important constraint is that the number of URBs in flight
> > be limited. It doesn't matter when they are allocated.
>
> You're optimistically assuming that URB allocations will succeed.  I
> guess that's okay -- dropping characters when there's insufficient
> memory seems like a good thing to do.

I also have to assume submission works.

> What's the point of that "reserve_buffer" thing?  Why not just use the
> next URB's transfer buffer?

I am toying with the idea of reusing the last URB.

On a fundamental note, thinking about this in terms of numbers of URBs
is strictly speaking wrong. We need to limit data in flight. For efficiency
we should make buffers as large as possible within that limit.

	Regards
		Oliver

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Stern]

On Thu, 7 May 2009, Oliver Neukum wrote:

> On a fundamental note, thinking about this in terms of numbers of URBs
> is strictly speaking wrong. We need to limit data in flight. For efficiency
> we should make buffers as large as possible within that limit.

But for latency you should submit URBs as soone as possible within that 
limit, which generally means small buffers.

How about setting the upper limit to URBs in flight based on the baud
rate?  Faster transfers deserve more URBs, right?  Assuming some
minimum number of bytes per URB (4? 8?), there should be enough URBs to
fill a pipeline whose length is around 5 ms or so (interrupt latency).

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Oliver Neukum]

Am Donnerstag, 7. Mai 2009 16:35:57 schrieb Alan Stern:
> On Thu, 7 May 2009, Oliver Neukum wrote:
> > On a fundamental note, thinking about this in terms of numbers of URBs
> > is strictly speaking wrong. We need to limit data in flight. For
> > efficiency we should make buffers as large as possible within that limit.
>
> But for latency you should submit URBs as soone as possible within that
> limit, which generally means small buffers.

This is true only if the device's buffer would run dry. But if no URB is
in flight, a URB should be written right away.

> How about setting the upper limit to URBs in flight based on the baud
> rate?  Faster transfers deserve more URBs, right?  Assuming some
> minimum number of bytes per URB (4? 8?), there should be enough URBs to
> fill a pipeline whose length is around 5 ms or so (interrupt latency).

Hm, you say many URBs can complete before an interrupt handler
can react?

	Regards
		Oliver

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Stern]

On Thu, 7 May 2009, Oliver Neukum wrote:

> > How about setting the upper limit to URBs in flight based on the baud
> > rate?  Faster transfers deserve more URBs, right?  Assuming some
> > minimum number of bytes per URB (4? 8?), there should be enough URBs to
> > fill a pipeline whose length is around 5 ms or so (interrupt latency).
> 
> Hm, you say many URBs can complete before an interrupt handler
> can react?

How long can interrupts remain disabled?  On a non-RT system, it might 
be several milliseconds.

In any case, a full-speed host controller won't issue IRQs more often 
than once per ms.  Quite a few URBs can complete in that time.

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Jason Wessel]

Oliver Neukum wrote:
> Am Mittwoch, 6. Mai 2009 21:24:56 schrieb Alan Stern:
>   
>> Converting \n to \r\n will add some complication
>> but not too much.
>>     

This conversion is done in the tty layer "the high level".  By the time
the low level driver gets it character stream via the tty call backs
everything is already processed.

>> Allocating URBs on the fly adds a lot of complication.  There has to be
>> a minimum number of pre-allocated URBs; otherwise write_room could
>>     
>
> Why? You can always calculate with the number of URBs you'd
> allocate as a maximum.
>
>   

In the scheme originally posted with the cap of 42 "in flight" urbs, I
had not seen it run out while using the standard tty driver.   The tty
driver really is not really the key problem I was after with the forced
poll patch.

The console driver is where all the character loss is going on.  There
is no tty interface, and there are no "write room" checks.  The present
kernel interface to the uart console callbacks is synchronous.

Here is a back trace to illustrate the typical 8250 driver.

#0  serial8250_console_putchar at drivers/serial/8250.c:2690
#1  0xc03977d4 in uart_console_write at drivers/serial/serial_core.c:1810
#2  0xc039cbb8 in serial8250_console_write at drivers/serial/8250.c:2729
#3  0xc022802b in __call_console_drivers at kernel/printk.c:419
#4  0xc022808b in _call_console_drivers at kernel/printk.c:449
#5  0xc0228269 in call_console_drivers at kernel/printk.c:499
#6  release_console_sem at kernel/printk.c:1020
#7  0xc022872b in vprintk at kernel/printk.c:749
#8  0xc022889b in printk at kernel/printk.c:582

Doing enough printk's such as in register_console() will cause loss
because you run out of URBs unless you can force them to complete.

Is there a valid way we can force the urbs to complete short of "polling
the heck out of the host controller" or have a synchronous interface in
the case of the console write call back?

There are two different call paths into the USB serial drivers just like
the uart drivers.  One is for the tty callbacks and other is for the
console call backs.  I am most interested how to fix the console writes
(with respect to the original experimental patch).

Jason.

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Greg KH]

On Wed, May 06, 2009 at 03:24:33PM -0500, Jason Wessel wrote:
> Oliver Neukum wrote:
> > Am Mittwoch, 6. Mai 2009 21:24:56 schrieb Alan Stern:
> >   
> >> Converting \n to \r\n will add some complication
> >> but not too much.
> >>     
> 
> This conversion is done in the tty layer "the high level".  By the time
> the low level driver gets it character stream via the tty call backs
> everything is already processed.
> 
> >> Allocating URBs on the fly adds a lot of complication.  There has to be
> >> a minimum number of pre-allocated URBs; otherwise write_room could
> >>     
> >
> > Why? You can always calculate with the number of URBs you'd
> > allocate as a maximum.
> >
> >   
> 
> In the scheme originally posted with the cap of 42 "in flight" urbs, I
> had not seen it run out while using the standard tty driver.   The tty
> driver really is not really the key problem I was after with the forced
> poll patch.
> 
> The console driver is where all the character loss is going on.  There
> is no tty interface, and there are no "write room" checks.  The present
> kernel interface to the uart console callbacks is synchronous.
> 
> Here is a back trace to illustrate the typical 8250 driver.
> 
> #0  serial8250_console_putchar at drivers/serial/8250.c:2690
> #1  0xc03977d4 in uart_console_write at drivers/serial/serial_core.c:1810
> #2  0xc039cbb8 in serial8250_console_write at drivers/serial/8250.c:2729
> #3  0xc022802b in __call_console_drivers at kernel/printk.c:419
> #4  0xc022808b in _call_console_drivers at kernel/printk.c:449
> #5  0xc0228269 in call_console_drivers at kernel/printk.c:499
> #6  release_console_sem at kernel/printk.c:1020
> #7  0xc022872b in vprintk at kernel/printk.c:749
> #8  0xc022889b in printk at kernel/printk.c:582
> 
> Doing enough printk's such as in register_console() will cause loss
> because you run out of URBs unless you can force them to complete.

Why not just make the upper bound of urbs very large?  We used to have
an unlimited number of in-flight urbs for some drivers until it was
pointed out that a simple:
	cat /dev/null > /dev/ttyUSB0
would cause a DoS :)

What happens if your upper bound is 400?  4000?  Will that work
properly?

thanks,

greg k-h

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to forcewrites

[Jason Wessel]

Greg KH wrote:
>> Doing enough printk's such as in register_console() will cause loss
>> because you run out of URBs unless you can force them to complete.
>>     
>
> Why not just make the upper bound of urbs very large?  We used to have
> an unlimited number of in-flight urbs for some drivers until it was
> pointed out that a simple:
> 	cat /dev/null > /dev/ttyUSB0
> would cause a DoS :)
>
> What happens if your upper bound is 400?  4000?  Will that work
> properly?
>   
Sure, if you have enough of a urb available, ultimately you get all the
printk's.

If I want to printk from the nmi_watchdog out to this device, am I going
to get anything if the write is not synchronous, because in theory the
hcd device is never going to get accessed again?

Ideally I want to make sure that somehow I get my debug output out to
the console, even if it means we do some polling after the oops state is
set.

Jason.

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to forcewrites

[Greg KH]

On Wed, May 06, 2009 at 03:51:14PM -0500, Jason Wessel wrote:
> Greg KH wrote:
> >> Doing enough printk's such as in register_console() will cause loss
> >> because you run out of URBs unless you can force them to complete.
> >>     
> >
> > Why not just make the upper bound of urbs very large?  We used to have
> > an unlimited number of in-flight urbs for some drivers until it was
> > pointed out that a simple:
> > 	cat /dev/null > /dev/ttyUSB0
> > would cause a DoS :)
> >
> > What happens if your upper bound is 400?  4000?  Will that work
> > properly?
> >   
> Sure, if you have enough of a urb available, ultimately you get all the
> printk's.

Good, then you just solved your issue with no nasty host driver hack.

> If I want to printk from the nmi_watchdog out to this device, am I going
> to get anything if the write is not synchronous, because in theory the
> hcd device is never going to get accessed again?

No you are not, sorry, USB doesn't work without interrupts in this kind
of mode at all.

> Ideally I want to make sure that somehow I get my debug output out to
> the console, even if it means we do some polling after the oops state is
> set.

That's going to be "interesting" to try to accomplish :)

good luck,

greg k-h

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to forcewrites

[Alan Stern]

On Wed, 6 May 2009, Greg KH wrote:

> > If I want to printk from the nmi_watchdog out to this device, am I going
> > to get anything if the write is not synchronous, because in theory the
> > hcd device is never going to get accessed again?
> 
> No you are not, sorry, USB doesn't work without interrupts in this kind
> of mode at all.

This really is an interesting point.  Provided the hardware is 
functioning correctly and enough free memory is available, Jason's 
polling approach would indeed allow USB to work without interrupts -- 
to a limited extent, which is all one needs for console support.  
Especially when the system has just crashed and you want to see that 
final "oops" message...

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Cox]

>     Write routine:
> 	Copy bytes into the available URB buffers, submitting URBs as 
> 	they get filled.  At the end, if the next URB is partially full 
> 	then submit it only if NIF < P.

There are good known algorithms for this and essentially the logic is
about speed and time estimation. You don't need to queue a partially
filled URB until you reach the time point that the hardware is in danger
of having sent everything already queued. So under load once you've
stuffed 4K down the pipe you know at 38400 baud that you can relax for
quite some time.

You could even entirely decouple write() from sending URBs.

>     Completion routine:
> 	If the next URB to send is partially filled, submit it.
> 
>     write_room routine:
> 	Return the actual number of bytes remaining in the available 
> 	URBs, but no more than (N-P)*B.

Providing your URBs can be allocated to say 512 bytes an URB then it'll
work fine to simply return the bytes left in the last URB (max the bytes
in the last URB). The tty code will then end up going

	room ?
		512 bytes
		fire away
	room ? 
		512 bytes
		fire away
	room ?
		...

until you run short of space

> 
> How does that sound?  Converting \n to \r\n will add some complication 
> but not too much.  
> 
> Allocating URBs on the fly adds a lot of complication.  There has to be 
> a minimum number of pre-allocated URBs; otherwise write_room could 
> never return a positive value.  If you allocate additional URBs 
> later on, when would you free them?

write_room can allocate URBs I think (would need to be atomic allocations)

	write_room()
		if no_urbs && urbs_queued < max_queued
			allocate an urb
		return space_in_last_urb;

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Alan Stern]

On Thu, 7 May 2009, Alan Cox wrote:

> >     Write routine:
> > 	Copy bytes into the available URB buffers, submitting URBs as 
> > 	they get filled.  At the end, if the next URB is partially full 
> > 	then submit it only if NIF < P.
> 
> There are good known algorithms for this and essentially the logic is
> about speed and time estimation. You don't need to queue a partially
> filled URB until you reach the time point that the hardware is in danger
> of having sent everything already queued. So under load once you've
> stuffed 4K down the pipe you know at 38400 baud that you can relax for
> quite some time.

Agreed.  An implicit point of my earlier message was that we might be
able to avoid tricky time-and-speed estimates by relying on the fact
that the USB-Serial device will NAK an URB until it has sufficient
buffer space to accept the data (i.e., flow control is built in at the
USB protocol level).

So instead of relaxing for some difficult-to-estimate time, we can
simply relax until the next URB completion occurs.  As long as a 
certain minimum number of URBs are still in flight, we can assume that 
the hardware isn't in danger of sending all the queued data.

This assumption might not be valid at the start of a transfer, since 
an URB can't complete for a minimum of 1 ms (at full speed), but it 
should stabilize quickly.

> > Allocating URBs on the fly adds a lot of complication.  There has to be 
> > a minimum number of pre-allocated URBs; otherwise write_room could 
> > never return a positive value.  If you allocate additional URBs 
> > later on, when would you free them?
> 
> write_room can allocate URBs I think (would need to be atomic allocations)
> 
> 	write_room()
> 		if no_urbs && urbs_queued < max_queued
> 			allocate an urb
> 		return space_in_last_urb;

That would work.  It doesn't seem to be much better than allocating 
URBs in the write routine.  After all, as long as allocations always 
succeed the details don't matter much.

A more interesting question is whether to deallocate URBs as they 
complete or to reuse them.  A simple approach (which Oliver no doubt 
has already considered) is:

	write_complete(urb):
		if (write_urb->transfer_length > 0)
			submit(write_urb);
			write_urb = urb;
		else
			urb_free(urb);

Alan Stern

Re: [PATCH 5/5] usb_debug: EXPERIMENTAL - poll hcd device to force writes

[Oliver Neukum]

Am Donnerstag, 7. Mai 2009 16:27:12 schrieb Alan Stern:
> A more interesting question is whether to deallocate URBs as they
> complete or to reuse them.  A simple approach (which Oliver no doubt
> has already considered) is:
>
>         write_complete(urb):
>                 if (write_urb->transfer_length > 0)
>                         submit(write_urb);
>                         write_urb = urb;
>                 else
>                         urb_free(urb);

It seems to me that if I don't know the true speed of a device I should
always keep two URBs in flight. That is if we decide that the generic
implementation needs to be trimmed for extreme performance.

Two URBs ensure that the finishing of one URB can trigger new data
and an URB is still ready to fill the gap. While two URBs are in flight
the driver should concentrate on filling large buffers. If we want to get
fancy we'd even consider interrupt mitigation here.

Will the tty layer make large writes if it can? I always assumed that
the use case that cares about performances is ppp and will deliver
network packets if it has enough write room. Is that always true?

	Regards
		Oliver

Re: [PATCH 4/5] usb,early_printk: unregister early usb before rest_init()

[Ingo Molnar]

* Jason Wessel <jason.wessel@windriver.com> wrote:

> The early_printk EHCI debug driver can hang the system or cause
> unpredictable results because two separate APIs will attempt to write
> to the mapped PCI region.
> 
> The safe route is to unregister the early USB console at the end of
> the core kernel init, which is before the PCI initialization.  Later,
> after the USB initialization completes it is then possible to register
> the console against the usb debug device and receive the printk
> messages that were missed in the period between early printk
> availability and the USB console device.
> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> ---
>  arch/x86/kernel/early_printk.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index e38c467..ef177d4 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -952,4 +952,26 @@ static int __init setup_early_printk(char *buf)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_EARLY_PRINTK_DBGP
> +/*
> + * The early console must get unregistered prior to the ECHI
> + * controller getting reset else the debug device cannot be used until
> + * a subsequent EHCI reset, or the kernel hangs.
> + */
> +static int __init usb_early_debug_cleanup(void)
> +{
> +	if (early_dbgp_console.index >= 0 &&
> +		early_dbgp_console.flags & CON_PRINTBUFFER) {
> +		console_stop(early_console);
> +		unregister_console(early_console);
> +		early_dbgp_console.index = -1;
> +		early_dbgp_console.flags = 0;
> +		ehci_debug = NULL;
> +		printk(KERN_INFO "Early USB console unregistered\n");
> +	}
> +	return 0;
> +}
> +postcore_initcall(usb_early_debug_cleanup);
> +#endif /* CONFIG_EARLY_PRINTK_DBGP */

We already have CON_BOOT which allows the unregistering of early 
consoles, via disable_boot_consoles() initcall in kernel/printk.c.

The hang should be solved differently: either by calling 
disable_boot_consoles() explicitly after console_init() - or by 
introducing another CON_ flag that marks the console for early 
forced unregistering.

	Ingo

Re: [PATCH 4/5] usb,early_printk: unregister early usb before rest_init()

[Jason Wessel]

Ingo Molnar wrote:
> * Jason Wessel <jason.wessel@windriver.com> wrote:
>
>> The early_printk EHCI debug driver can hang the system or cause
>> unpredictable results because two separate APIs will attempt to write
>> to the mapped PCI region.
[clip]
>> +postcore_initcall(usb_early_debug_cleanup);
>
> We already have CON_BOOT which allows the unregistering of early
> consoles, via disable_boot_consoles() initcall in kernel/printk.c.
>
> The hang should be solved differently: either by calling
> disable_boot_consoles() explicitly after console_init() - or by
> introducing another CON_ flag that marks the console for early
> forced unregistering.

I thought about adding another flag in console_init() such that an
early console which cannot safely be used can elect to unregister.
There are two problems with this.

1) We actually need a call back to unset ehci_debug, or the
   console_unregister needs to set/clear a flag which then must be
   checked on each early_printk write, else any further call laying
   around for early_printk can crash the system.

2) Ideally you want to keep printk alive for as long as possible,
   which means you can have it all the way through start_kernel()
   before the "blackout period" while waiting for pci and usb init.

Given these two issues, is it your preference still to see a patch
that changes console_init or add another call back to start_kernel() ?

An earlier iteration of this patch added a call back in start_kernel
just before rest_init(), but it seemed like it was not needed since we
can hook on to part of the relevent function init chain.

I am open to fixing this in what ever manner is acceptable.

Cheers,
Jason.

Re: [PATCH 4/5] usb,early_printk: unregister early usb before rest_init()

[Ingo Molnar]

* Jason Wessel <jason.wessel@windriver.com> wrote:

> Ingo Molnar wrote:
> > * Jason Wessel <jason.wessel@windriver.com> wrote:
> >
> >> The early_printk EHCI debug driver can hang the system or cause
> >> unpredictable results because two separate APIs will attempt to write
> >> to the mapped PCI region.
> [clip]
> >> +postcore_initcall(usb_early_debug_cleanup);
> >
> > We already have CON_BOOT which allows the unregistering of early
> > consoles, via disable_boot_consoles() initcall in kernel/printk.c.
> >
> > The hang should be solved differently: either by calling
> > disable_boot_consoles() explicitly after console_init() - or by
> > introducing another CON_ flag that marks the console for early
> > forced unregistering.
> 
> I thought about adding another flag in console_init() such that an
> early console which cannot safely be used can elect to unregister.
> There are two problems with this.
> 
> 1) We actually need a call back to unset ehci_debug, or the
>    console_unregister needs to set/clear a flag which then must be
>    checked on each early_printk write, else any further call laying
>    around for early_printk can crash the system.

yeah, it should definitely be unregistered in the 'blackout' period.

> 
> 2) Ideally you want to keep printk alive for as long as possible,
>    which means you can have it all the way through start_kernel()
>    before the "blackout period" while waiting for pci and usb init.
> 
> Given these two issues, is it your preference still to see a patch 
> that changes console_init or add another call back to 
> start_kernel() ?
> 
> An earlier iteration of this patch added a call back in 
> start_kernel just before rest_init(), but it seemed like it was 
> not needed since we can hook on to part of the relevent function 
> init chain.
> 
> I am open to fixing this in what ever manner is acceptable.

My main point is to please use clean init sequences, instead of 
relying on the current (pretty random) semantics of postcore_init().

It is a property of the USB code that it has trouble with the early 
console - so an explicit call to unregister_usb_early_consoles() 
call would solve it.

Probably followed by a reregister_usb_early_consoles() call - your 
patch AFAICS breaks earlyprint=dbg,...,keep functionality, right?

	Ingo

Re: [PATCH 3/5] usb,early_printk: insert cr prior to nl as needed

[Ingo Molnar]

* Jason Wessel <jason.wessel@windriver.com> wrote:

> The rs232 drivers send a carriage return prior to a new line in the
> early printk code.
> 
> The usb debug driver should do the same because you want to be able to
> use the same terminal programs and tools for analysis of early printk
> data.
> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> ---
>  arch/x86/kernel/early_printk.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)

hm, would be better to carry this in the x86 tree - where most of 
the early-debug functionality has been authored and maintained. 

Greg, is that fine with you?

	Ingo

Re: [PATCH 3/5] usb,early_printk: insert cr prior to nl as needed

[Greg KH]

On Wed, May 06, 2009 at 09:30:25AM +0200, Ingo Molnar wrote:
> 
> * Jason Wessel <jason.wessel@windriver.com> wrote:
> 
> > The rs232 drivers send a carriage return prior to a new line in the
> > early printk code.
> > 
> > The usb debug driver should do the same because you want to be able to
> > use the same terminal programs and tools for analysis of early printk
> > data.
> > 
> > Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> > ---
> >  arch/x86/kernel/early_printk.c |   22 ++++++++++++++++------
> >  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> hm, would be better to carry this in the x86 tree - where most of 
> the early-debug functionality has been authored and maintained. 
> 
> Greg, is that fine with you?

Yes, but it needs a few more rounds to fix the bugs pointed out.  So
please don't take this one yet.

thanks,

greg k-h

Re: [PATCH 3/5] usb,early_printk: insert cr prior to nl as needed

[Ingo Molnar]

* Greg KH <greg@kroah.com> wrote:

> On Wed, May 06, 2009 at 09:30:25AM +0200, Ingo Molnar wrote:
> > 
> > * Jason Wessel <jason.wessel@windriver.com> wrote:
> > 
> > > The rs232 drivers send a carriage return prior to a new line in the
> > > early printk code.
> > > 
> > > The usb debug driver should do the same because you want to be able to
> > > use the same terminal programs and tools for analysis of early printk
> > > data.
> > > 
> > > Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> > > ---
> > >  arch/x86/kernel/early_printk.c |   22 ++++++++++++++++------
> > >  1 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > hm, would be better to carry this in the x86 tree - where most of 
> > the early-debug functionality has been authored and maintained. 
> > 
> > Greg, is that fine with you?
> 
> Yes, but it needs a few more rounds to fix the bugs pointed out.  
> So please don't take this one yet.

Sure, will wait for your Acked-by to show up in the changelog.

	Ingo

Re: [PATCH 1/5] usb_debug: implement multi urb write

[Oliver Neukum]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 775 bytes --]

Am Mittwoch, 6. Mai 2009 04:00:01 schrieb Jason Wessel:
in static void usb_debug_write_bulk_callback(struct urb *urb)> +       if (status) {> +               dbg("nonzero write bulk status received: %d", status);> +               return;> +       }
[..]> +       spin_lock_irqsave(&priv->tx_lock, flags);> +       --priv->tx_outstanding_urbs;> +       spin_unlock_irqrestore(&priv->tx_lock, flags);
That's a clear bug. If a URB finishes, you must decrease the counter, alwaysand without exception, even if status indicates an error.
	Regards		Oliver
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/5] usb_debug: implement multi urb write

[Jason Wessel]

Oliver Neukum wrote:
> Am Mittwoch, 6. Mai 2009 04:00:01 schrieb Jason Wessel:
>
> in static void usb_debug_write_bulk_callback(struct urb *urb)
>   
>> +       if (status) {
>> +               dbg("nonzero write bulk status received: %d", status);
>> +               return;
>> +       }
>>     
>
> [..]
>   
>> +       spin_lock_irqsave(&priv->tx_lock, flags);
>> +       --priv->tx_outstanding_urbs;
>> +       spin_unlock_irqrestore(&priv->tx_lock, flags);
>>     
>
> That's a clear bug. If a URB finishes, you must decrease the counter, always
> and without exception, even if status indicates an error.
>
>   
Thanks Oliver,

I would agree with you on that.  It also means that the ftdi_sio.c
driver has the same bug, because that is where it was derived from.  You
led me to see another flaw upon further inspection in that the usb_debug
driver must also implement the chars_in_buffer() call back because the
generic serial code will cause an oops with a null pointer dereference
of the write_urb.

Cheers,
Jason.

Re: [PATCH 1/5] usb_debug: implement multi urb write

[Oliver Neukum]

Am Mittwoch, 6. Mai 2009 13:57:31 schrieb Jason Wessel:

> I would agree with you on that.  It also means that the ftdi_sio.c
> driver has the same bug, because that is where it was derived from.  You

Greg will happily take a patch.

> led me to see another flaw upon further inspection in that the usb_debug
> driver must also implement the chars_in_buffer() call back because the
> generic serial code will cause an oops with a null pointer dereference
> of the write_urb.

Good.

	Regards
		Oliver

Re: [PATCH 1/5] usb_debug: implement multi urb write

[Greg KH]

On Tue, May 05, 2009 at 09:00:01PM -0500, Jason Wessel wrote:
> The usb_debug driver, when used as the console, will always fail to
> insert the carriage return and new line sequence as well as randomly
> drop console output.  This is a result of only having the single
> write_urb and that the tty layer will have a lock that prevents the
> processing of the back to back urb requests.
> 
> The solution is to allow more than one urb to be outstanding and have
> a slightly deeper transmit queue.  The idea and some code is borrowed
> from the ftdi_sio usb driver.

Almost all usb-serial drivers need this functionality.

As you are adding it to the usb_debug driver, why not just add this kind
of functionality to the usb-serial core instead, that way we can share
it with all of the drivers?  I'd much rather see that happen instead of
us doing it individually again.

thanks,

greg k-h