[PATCH 0/3] usb console regression fixes - 2.6.31

[PATCH 0/3] usb console regression fixes - 2.6.31

[Jason Wessel]

This patch series is aimed to fix the performance and crash regression
with usb console devices.

The first patch in the series fixes the usb console reference counting
which led to a oops during boot.  The test case for this was to use
the FTDI usb serial device with the kernel boot argument
"console=ttyUSB0" and also using the device as an interactive terminal.

The second and third patch optimize the performance of the hot path
for usb serial input processing.

If Alan ack's or signs off on the patches, I'll issue a pull request
to Linus, or we can go another round of revision/review if needed.

Thanks,
Jason.

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

The following changes since commit 3fe0344faf7fdcb158bd5c1a9aec960a8d70c8e8:
  Linus Torvalds (1):
        Merge branch 'kmemleak' of git://linux-arm.org/linux-2.6

are available in the git repository at:

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

Jason Wessel (3):
      usb,console: Fix regression in usb console on kernel boot
      usb-serial: regression fix to move sysrq from hot path
      usb-serial: optimize sysrq function calls

 drivers/usb/serial/console.c    |   13 ++++++-----
 drivers/usb/serial/generic.c    |   41 +++++++++++---------------------------
 drivers/usb/serial/usb-serial.c |    3 +-
 include/linux/usb/serial.h      |   32 ++++++++++++++++++++++++++---
 4 files changed, 49 insertions(+), 40 deletions(-)

[PATCH 1/3] usb,console: Fix regression in usb console on kernel boot

[Jason Wessel]

The commit 335f8514f200e63d689113d29cb7253a5c282967 introduced a
regression which stopped usb consoles from working correctly as a
kernel boot console as well as interactive login device.

The addition of the serial_close() which in turn calls
tty_port_close_start() will change the reference count of port.count
and warn about it.  The usb console code had previously incremented
the port.count to indicate it was making use of the device as a
console and the forced change causes a double open on the usb device
which leads to a non obvious kernel oops later on when the tty is
freed.

To fix the problem instead make use of port->console to track if the
port is in fact an active console port to avoid double initialization
of the usb serial device.  The port.count is incremented and
decremented only with in the scope of usb_console_setup() for the
purpose of the low level driver initialization.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/serial/console.c    |   13 +++++++------
 drivers/usb/serial/usb-serial.c |    3 ++-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 247b61b..0e4f2e4 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -169,9 +169,11 @@ static int usb_console_setup(struct console *co, char *options)
 			kfree(tty);
 		}
 	}
-	/* So we know not to kill the hardware on a hangup on this
-	   port. We have also bumped the use count by one so it won't go
-	   idle */
+	/* Now that any required fake tty operations are completed restore
+	 * the tty port count */
+	--port->port.count;
+	/* The console is special in terms of closing the device so
+	 * indicate this port is now acting as a system console. */
 	port->console = 1;
 	retval = 0;
 
@@ -204,7 +206,7 @@ static void usb_console_write(struct console *co,
 
 	dbg("%s - port %d, %d byte(s)", __func__, port->number, count);
 
-	if (!port->port.count) {
+	if (!port->console) {
 		dbg("%s - port not opened", __func__);
 		return;
 	}
@@ -300,8 +302,7 @@ void usb_serial_console_exit(void)
 {
 	if (usbcons_info.port) {
 		unregister_console(&usbcons);
-		if (usbcons_info.port->port.count)
-			usbcons_info.port->port.count--;
+		usbcons_info.port->console = 0;
 		usbcons_info.port = NULL;
 	}
 }
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index d595aa5..329e098 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -220,7 +220,8 @@ static int serial_open (struct tty_struct *tty, struct file *filp)
 	tty->driver_data = port;
 	tty_port_tty_set(&port->port, tty);
 
-	if (port->port.count == 1) {
+	/* If the console is attached, the device is already open */
+	if (port->port.count == 1 && !port->console) {
 
 		/* lock this module before we call it
 		 * this may fail, which means we must bail out,
-- 
1.6.3.1.9.g95405b

[PATCH 2/3] usb-serial: regression fix to move sysrq from hot path

[Jason Wessel]

A performance regression was introduced by commit:
98fcb5f78165b8a3d93870ad7afd4d9ebbb8b43a

The sysrq handling should only get executed if the attached usb serial
device is acting as a serial system console.  A serial system console
has a very low input throughput vs a 3g usb modem which pushes bytes
through the same interface at a high rate.

Entire strings of output can be processed via the tty input when the
device is not a console.

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

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 932d624..82b8883 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -425,11 +425,19 @@ static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
 		goto done;
 
 	/* Push data to 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);
+	if (port->console) {
+		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);
+	} else if (urb->actual_length) {
+		i = tty_buffer_request_room(tty, urb->actual_length);
+		if (i) {
+			tty_insert_flip_string(tty, urb->transfer_buffer, i);
+			tty_flip_buffer_push(tty);
+		}
 	}
-	tty_flip_buffer_push(tty);
 	tty_kref_put(tty);
 done:
 	usb_serial_generic_resubmit_read_urb(port, GFP_ATOMIC);
-- 
1.6.3.1.9.g95405b

[PATCH 3/3] usb-serial: optimize sysrq function calls

[Jason Wessel]

There is no need to have external function calls for the sysrq
functions.  The compiler can inline the sysrq calls such that they are
entirely a NOP if CONFIG_MAGIC_SYSRQ is not set.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/serial/generic.c |   25 -------------------------
 include/linux/usb/serial.h   |   32 ++++++++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 82b8883..45515c0 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -535,31 +535,6 @@ void usb_serial_generic_unthrottle(struct tty_struct *tty)
 	}
 }
 
-int usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
-{
-	if (port->sysrq && port->console) {
-		if (ch && time_before(jiffies, port->sysrq)) {
-			handle_sysrq(ch, tty_port_tty_get(&port->port));
-			port->sysrq = 0;
-			return 1;
-		}
-		port->sysrq = 0;
-	}
-	return 0;
-}
-EXPORT_SYMBOL_GPL(usb_serial_handle_sysrq_char);
-
-int usb_serial_handle_break(struct usb_serial_port *port)
-{
-	if (!port->sysrq) {
-		port->sysrq = jiffies + HZ*5;
-		return 1;
-	}
-	port->sysrq = 0;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(usb_serial_handle_break);
-
 void usb_serial_generic_disconnect(struct usb_serial *serial)
 {
 	int i;
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 44801d2..4601e0d 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -317,10 +317,6 @@ extern int usb_serial_generic_register(int debug);
 extern void usb_serial_generic_deregister(void);
 extern void usb_serial_generic_resubmit_read_urb(struct usb_serial_port *port,
 						 gfp_t mem_flags);
-extern int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
-					unsigned int ch);
-extern int usb_serial_handle_break(struct usb_serial_port *port);
-
 
 extern int usb_serial_bus_register(struct usb_serial_driver *device);
 extern void usb_serial_bus_deregister(struct usb_serial_driver *device);
@@ -329,6 +325,34 @@ extern struct usb_serial_driver usb_serial_generic_device;
 extern struct bus_type usb_serial_bus_type;
 extern struct tty_driver *usb_serial_tty_driver;
 
+static inline int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
+					       unsigned int ch)
+{
+#ifdef CONFIG_MAGIC_SYSRQ
+	if (port->sysrq && port->console) {
+		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_MAGIC_SYSRQ */
+	return 0;
+}
+
+static inline int usb_serial_handle_break(struct usb_serial_port *port)
+{
+#ifdef CONFIG_MAGIC_SYSRQ
+	if (!port->sysrq) {
+		port->sysrq = jiffies + HZ*5;
+		return 1;
+	}
+	port->sysrq = 0;
+#endif /* CONFIG_MAGIC_SYSRQ */
+	return 0;
+}
+
 static inline void usb_serial_debug_data(int debug,
 					 struct device *dev,
 					 const char *function, int size,
-- 
1.6.3.1.9.g95405b

Re: [PATCH 1/3] usb,console: Fix regression in usb console on kernel boot

[Alan Cox]

On Wed, 17 Jun 2009 21:20:07 -0500
Jason Wessel <jason.wessel@windriver.com> wrote:

> The commit 335f8514f200e63d689113d29cb7253a5c282967 introduced a
> regression which stopped usb consoles from working correctly as a
> kernel boot console as well as interactive login device.
> 
> The addition of the serial_close() which in turn calls
> tty_port_close_start() will change the reference count of port.count
> and warn about it.  The usb console code had previously incremented
> the port.count to indicate it was making use of the device as a
> console and the forced change causes a double open on the usb device
> which leads to a non obvious kernel oops later on when the tty is
> freed.
> 
> To fix the problem instead make use of port->console to track if the
> port is in fact an active console port to avoid double initialization
> of the usb serial device.  The port.count is incremented and
> decremented only with in the scope of usb_console_setup() for the
> purpose of the low level driver initialization.

Acked-by: Alan Cox <alan@linux.intel.com>

Re: [PATCH 2/3] usb-serial: regression fix to move sysrq from hot path

[Alan Cox]

> +	if (port->console) {

	unlikely(port->console) I think is justified

> +		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);

But thats a minor twiddle so

Acked-by: Alan Cox <alan@linux.intel.com>

Alan

Re: [PATCH 2/3] usb-serial: regression fix to move sysrq from hot path

[Jason Wessel]

Alan Cox wrote:
>> +	if (port->console) {
>>     
>
> 	unlikely(port->console) I think is justified
>
>   

I pushed that change into my for_linus git branch.

That just leaves patch 3 in the series for Greg KH.

Thanks,
Jason.