[RFC PATCH 0/5] usb polling / reliable usb console / kdb usb keyboards

[RFC PATCH 0/5] usb polling / reliable usb console / kdb usb keyboards

[Jason Wessel]

-- The summary --

This patch series has 5 patches.  The two from Dmitry were cooked up
at the kernel summit to once and for all fix the keyboard release
issues.  These 2 patches are included in the usb series for
completeness and fact I like to post series you could actually try.
The usb keyboard patches for kdb rely upon the new API introduced in
Dmitry's patches.

The 3 usb patches do the following:
  * Introduce a specific device usb polling API mechanism though the hcd
  * Add polling support to the generic usb serial console
  * Implement usb keyboard support for kdb with the usb polling api

-- The story --

If we jump in the time machine and go back to LPC 2009, I presented a
todo list for kernel debugging.  I really think it absolutely amazing
the amount of progress we have made for kernel debugging in the last
year.  Especially if you consider kdb was nothing more than a rough
prototype this time last year.  If I pull out the high level todo
bullets out of the 2009 presentation it looks like this:

  *(done 2.6.33) HW breakpoints work with perf/ptrace/kgdb
  *(done 2.6.34) EHCI debug port reliable printk/early_printk
  *(done 2.6.35) Early debug support for kdb & EHCI debug port
  *(done 2.6.35) Move beyond the kdb prototype and merge kdb
  *(done 2.6.35) Low level exception trap support
  *(done) Finish atomic kernel mode setting kdb integration and merge it
     ** 2.6.36 mainline atomic kms for nouveau, radeon and i915
  * Design an atomic keyboard reset for entry exit
  * Reliable usb console
  * Redesign kdb usb keyboard implementation
  * kgdboe v2
  * Displaced stepping and kprobe integration
  * kgdb for usb serial

With this patch series, only the last 3 items in the list remain.
There has been some definite progress on each of these items, but
there is no definitive time table as to when they will complete due to
the complexity of the issues involved and more over, the need to get
some other folks in the community to work on these items.

At the kernel summit 2010, Dmitry and I were able to come to a final
consensus on how to solve the keyboard key release problems related to entry
and exit from the kernel debugger.  This turned my attention to
talking to Alan Stern to try to resolve the outstanding design issues
with implementing a generic form of usb device polling.  By the time
the LPC 2010 ended we had a complete design on a sheet of paper and
several hours later a preliminary implementation.

With the kernel debug core API now well established the focus will
change a bit in the coming year and I hope that we can post a new
public todo list and the status of the kernel debugger on the kgdb/kdb
website: http://kgdb.wiki.kernel.org/

After attending the most recent tracing summit, I am encouraged by the
progress.  The goal for the 2011 time frame is to begin the journey of
integrating debugging and tracing to aid in general understanding of
the kernel as well as to speed up how quickly and easily you can root
cause a problem.

Last but not least, I want to thank all the people that submit
patches, report problems and review patches. THANK YOU!

--
The following changes since commit e99d11d19977c74b18411cdb59cdebb788237a6e:
  Paul Mundt (1):
        fs: logfs: Fix up MTD=y build.

are available in the git repository at:

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

Dmitry Torokhov (2):
      Input: input_reset()
      kgdboc: reset input devices (keyboards) when exiting debugger

Jason Wessel (3):
      usb-hcd: implement polling a specific usb device
      usb-serial-console: try to poll the hcd vs dropping data
      usb,keyboard,kdb: Implement HID keyboard polling

 drivers/hid/usbhid/hid-core.c   |   18 +++-
 drivers/input/input.c           |   50 +++++++---
 drivers/serial/kgdboc.c         |   74 +++++++++++++++
 drivers/usb/core/hcd.c          |   57 +++++++++++
 drivers/usb/serial/console.c    |   42 ++++++---
 include/linux/input.h           |    4 +-
 include/linux/kdb.h             |   13 +++
 include/linux/usb.h             |    5 +
 kernel/debug/kdb/kdb_keyboard.c |  195 ++++++++++++++++++++++++++++++++++++++-
 lib/Kconfig.kgdb                |    7 ++
 10 files changed, 430 insertions(+), 35 deletions(-)

[RFC PATCH 1/5] usb-hcd: implement polling a specific usb device

[Jason Wessel]

This patch adds a generic capability to poll a specific usb device and
run its completion handler.

There will be two users of this functionality.
 1) usb serial port console devices
 2) usb keyboards for use with kdb

Any time the usb_irq_poll() function is called it has the possibility
to queue some urbs for completion at a later time.  Any code that uses
the usb_irq_poll() function must call usb_poll_irq_schedule_flush()
before all the other usb devices will start working again.

CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Alan Cox <alan@linux.intel.com>
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Oliver Neukum <oliver@neukum.org>
CC: linux-usb@vger.kernel.org
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/core/hcd.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h    |    5 ++++
 2 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 61800f7..2e07097 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -96,6 +96,10 @@ struct usb_busmap {
 };
 static struct usb_busmap busmap;
 
+/* Support polling for a single device's urbs */
+static struct usb_device *usb_poll_hcd_device;
+static LIST_HEAD(delayed_usb_complete_queue);
+
 /* used when updating list of hcds */
 DEFINE_MUTEX(usb_bus_list_lock);	/* exported only for usbfs */
 EXPORT_SYMBOL_GPL (usb_bus_list_lock);
@@ -1528,6 +1532,7 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
 }
 
 /*-------------------------------------------------------------------------*/
+static DEFINE_MUTEX(usb_poll_irq_flush_mutex);
 
 /**
  * usb_hcd_giveback_urb - return URB from HCD to device driver
@@ -1562,6 +1567,17 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 
 	/* pass ownership to the completion handler */
 	urb->status = status;
+	if (unlikely(usb_poll_hcd_device)) {
+		/*
+		 * Any other device than the one being polled should get
+		 * queued for a later flush.
+		 */
+		if (usb_poll_hcd_device != urb->dev) {
+			INIT_LIST_HEAD(&urb->poll_list);
+			list_add(&urb->poll_list, &delayed_usb_complete_queue);
+			return;
+		}
+	}
 	urb->complete (urb);
 	atomic_dec (&urb->use_count);
 	if (unlikely(atomic_read(&urb->reject)))
@@ -2425,6 +2441,47 @@ usb_hcd_platform_shutdown(struct platform_device* dev)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_platform_shutdown);
 
+void usb_poll_irq(struct usb_device *udev)
+{
+	struct usb_hcd *hcd;
+
+	hcd = bus_to_hcd(udev->bus);
+	usb_poll_hcd_device = udev;
+	usb_hcd_irq(0, hcd);
+	usb_poll_hcd_device = NULL;
+}
+EXPORT_SYMBOL_GPL(usb_poll_irq);
+
+static void usb_poll_irq_flush_helper(struct work_struct *dummy)
+{
+	struct urb *urb;
+	struct urb *scratch;
+	unsigned long flags;
+
+	mutex_lock(&usb_poll_irq_flush_mutex);
+	local_irq_save(flags);
+	list_for_each_entry_safe(urb, scratch, &delayed_usb_complete_queue,
+				 poll_list) {
+		list_del(&urb->poll_list);
+		urb->complete(urb);
+		atomic_dec(&urb->use_count);
+		if (unlikely(atomic_read(&urb->reject)))
+			wake_up(&usb_kill_urb_queue);
+		usb_put_urb(urb);
+	}
+	local_irq_restore(flags);
+	mutex_unlock(&usb_poll_irq_flush_mutex);
+}
+
+static DECLARE_WORK(usb_poll_irq_flush_work, usb_poll_irq_flush_helper);
+
+
+void usb_poll_irq_schedule_flush(void)
+{
+	schedule_work(&usb_poll_irq_flush_work);
+}
+EXPORT_SYMBOL_GPL(usb_poll_irq_schedule_flush);
+
 /*-------------------------------------------------------------------------*/
 
 #if defined(CONFIG_USB_MON) || defined(CONFIG_USB_MON_MODULE)
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 35fe6ab..47ab24e 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -558,6 +558,10 @@ static inline void usb_mark_last_busy(struct usb_device *udev)
 
 /*-------------------------------------------------------------------------*/
 
+/* for polling the hcd device */
+extern void usb_poll_irq(struct usb_device *udev);
+extern void usb_poll_irq_schedule_flush(void);
+
 /* for drivers using iso endpoints */
 extern int usb_get_current_frame_number(struct usb_device *usb_dev);
 
@@ -1188,6 +1192,7 @@ struct urb {
 	struct list_head urb_list;	/* list head for use by the urb's
 					 * current owner */
 	struct list_head anchor_list;	/* the URB may be anchored */
+	struct list_head poll_list;	/* Added to the poll queue */
 	struct usb_anchor *anchor;
 	struct usb_device *dev;		/* (in) pointer to associated device */
 	struct usb_host_endpoint *ep;	/* (internal) pointer to endpoint */
-- 
1.6.3.3

[RFC PATCH 2/5] usb-serial-console: try to poll the hcd vs dropping data

[Jason Wessel]

This patch tries to solve the problem that printk data is dropped
because there are too many outstanding transmit urb's while trying to
execute printk's to a console.  You can observe this by booting a
kernel with a serial console and cross checking the dmesg output with
what you received on the console or doing something like
"echo t > /proc/sysrq-trigger".

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

A few millisecond penalty will get incurred to allow the hcd
controller to complete a write urb, else the console data is thrown
away.  Even with any kind of delay, the latency is still lower than
using a traditional serial port uart due to the fact that there are
bigger buffer for use with the USB serial console kfifo
implementation.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/serial/console.c |   42 +++++++++++++++++++++++++++---------------
 1 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 1ee6b2a..b09832f 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -197,13 +197,37 @@ static int usb_console_setup(struct console *co, char *options)
 	return retval;
 }
 
+static void usb_do_console_write(struct usb_serial *serial,
+				 struct usb_serial_port *port,
+				 const char *buf, unsigned count)
+{
+	int retval;
+	int loops = 100;
+try_again:
+	/* pass on to the driver specific version of this function if
+	   it is available */
+	if (serial->type->write)
+		retval = serial->type->write(NULL, port, buf, count);
+	else
+		retval = usb_serial_generic_write(NULL, port, buf, count);
+	if (retval < count && retval >= 0 && loops--) {
+		/* poll the hcd device because the queue is full */
+		count -= retval;
+		buf += retval;
+		udelay(100);
+		usb_poll_irq(serial->dev);
+		usb_poll_irq_schedule_flush();
+		goto try_again;
+	}
+	dbg("%s - return value : %d", __func__, retval);
+}
+
 static void usb_console_write(struct console *co,
 					const char *buf, unsigned count)
 {
 	static struct usbcons_info *info = &usbcons_info;
 	struct usb_serial_port *port = info->port;
 	struct usb_serial *serial;
-	int retval = -ENODEV;
 
 	if (!port || port->serial->dev->state == USB_STATE_NOTATTACHED)
 		return;
@@ -230,23 +254,11 @@ static void usb_console_write(struct console *co,
 				break;
 			}
 		}
-		/* pass on to the driver specific version of this function if
-		   it is available */
-		if (serial->type->write)
-			retval = serial->type->write(NULL, port, buf, i);
-		else
-			retval = usb_serial_generic_write(NULL, port, buf, i);
-		dbg("%s - return value : %d", __func__, retval);
+		usb_do_console_write(serial, port, buf, i);
 		if (lf) {
 			/* append CR after LF */
 			unsigned char cr = 13;
-			if (serial->type->write)
-				retval = serial->type->write(NULL,
-								port, &cr, 1);
-			else
-				retval = usb_serial_generic_write(NULL,
-								port, &cr, 1);
-			dbg("%s - return value : %d", __func__, retval);
+			usb_do_console_write(serial, port, &cr, 1);
 		}
 		buf += i;
 		count -= i;
-- 
1.6.3.3

[RFC PATCH 3/5] Input: input_reset()

[Jason Wessel]

From: Dmitry Torokhov <dtor@mail.ru>

Expose part of the logic used to resume from suspend to clear a
keyboard state for use from an exported function called
input_reset_device().  The intended use of the function is allow
reseting the input state after resuming kernel execution from the
kernel debugger.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Acked-by: Jason Wessel <jason.wessel@windriver.com>
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/input/input.c |   50 ++++++++++++++++++++++++++++++++++--------------
 include/linux/input.h |    4 ++-
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index d092ef9..75bed63 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1565,8 +1565,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 		}							\
 	} while (0)
 
-#ifdef CONFIG_PM
-static void input_dev_reset(struct input_dev *dev, bool activate)
+static void input_dev_toggle(struct input_dev *dev, bool activate)
 {
 	if (!dev->event)
 		return;
@@ -1580,12 +1579,44 @@ static void input_dev_reset(struct input_dev *dev, bool activate)
 	}
 }
 
+/**
+ * input_reset_device() - reset/restore the state of input device
+ * @dev: input device whose state needs to be reset
+ *
+ * This function tries to reset the state of an opened input device and
+ * bring internal state and state if the hardware in sync with each other.
+ * We mark all keys as released, restore LED state, repeat rate, etc.
+ */
+void input_reset_device(struct input_dev *dev)
+{
+	mutex_lock(&dev->mutex);
+
+	if (dev->users) {
+		input_dev_toggle(dev, true);
+
+		/*
+		 * Keys that have been pressed at suspend time are unlikely
+		 * to be still pressed when we resume.
+		 */
+		spin_lock_irq(&dev->event_lock);
+		input_dev_release_keys(dev);
+		spin_unlock_irq(&dev->event_lock);
+	}
+
+	mutex_unlock(&dev->mutex);
+}
+EXPORT_SYMBOL(input_reset_device);
+
+#ifdef CONFIG_PM
 static int input_dev_suspend(struct device *dev)
 {
 	struct input_dev *input_dev = to_input_dev(dev);
 
 	mutex_lock(&input_dev->mutex);
-	input_dev_reset(input_dev, false);
+
+	if (input_dev->users)
+		input_dev_toggle(input_dev, false);
+
 	mutex_unlock(&input_dev->mutex);
 
 	return 0;
@@ -1595,18 +1626,7 @@ static int input_dev_resume(struct device *dev)
 {
 	struct input_dev *input_dev = to_input_dev(dev);
 
-	mutex_lock(&input_dev->mutex);
-	input_dev_reset(input_dev, true);
-
-	/*
-	 * Keys that have been pressed at suspend time are unlikely
-	 * to be still pressed when we resume.
-	 */
-	spin_lock_irq(&input_dev->event_lock);
-	input_dev_release_keys(input_dev);
-	spin_unlock_irq(&input_dev->event_lock);
-
-	mutex_unlock(&input_dev->mutex);
+	input_reset_device(input_dev);
 
 	return 0;
 }
diff --git a/include/linux/input.h b/include/linux/input.h
index 51af441..6ef4446 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -1406,6 +1406,8 @@ static inline void input_set_drvdata(struct input_dev *dev, void *data)
 int __must_check input_register_device(struct input_dev *);
 void input_unregister_device(struct input_dev *);
 
+void input_reset_device(struct input_dev *);
+
 int __must_check input_register_handler(struct input_handler *);
 void input_unregister_handler(struct input_handler *);
 
@@ -1421,7 +1423,7 @@ void input_release_device(struct input_handle *);
 int input_open_device(struct input_handle *);
 void input_close_device(struct input_handle *);
 
-int input_flush_device(struct input_handle* handle, struct file* file);
+int input_flush_device(struct input_handle *handle, struct file *file);
 
 void input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value);
 void input_inject_event(struct input_handle *handle, unsigned int type, unsigned int code, int value);
-- 
1.6.3.3

[RFC PATCH 4/5] kgdboc: reset input devices (keyboards) when exiting debugger

[Jason Wessel]

From: Dmitry Torokhov <dtor@mail.ru>

Allow all keys to be released when resuming kernel execution to avoid
problems with auto repeated keystrokes and the need to press a key an
extra time to reset its state.

[jason.wessel@windriver.com: fix compile without keyboard input driver]
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/serial/kgdboc.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
index d4b711c..3374618 100644
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -18,6 +18,7 @@
 #include <linux/tty.h>
 #include <linux/console.h>
 #include <linux/vt_kern.h>
+#include <linux/input.h>
 
 #define MAX_CONFIG_LEN		40
 
@@ -37,6 +38,61 @@ static struct tty_driver	*kgdb_tty_driver;
 static int			kgdb_tty_line;
 
 #ifdef CONFIG_KDB_KEYBOARD
+static int kgdboc_reset_connect(struct input_handler *handler,
+				struct input_dev *dev,
+				const struct input_device_id *id)
+{
+	input_reset_device(dev);
+
+	/* Retrun an error - we do not want to bind, just to reset */
+	return -ENODEV;
+}
+
+static void kgdboc_reset_disconnect(struct input_handle *handle)
+{
+	/* We do not expect anyone to actually bind to us */
+	BUG();
+}
+
+static const struct input_device_id kgdboc_reset_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
+		.evbit = { BIT_MASK(EV_KEY) },
+	},
+	{ }
+};
+
+static struct input_handler kgdboc_reset_handler = {
+	.connect	= kgdboc_reset_connect,
+	.disconnect	= kgdboc_reset_disconnect,
+	.name		= "kgdboc_reset",
+	.id_table	= kgdboc_reset_ids,
+};
+
+static DEFINE_MUTEX(kgdboc_reset_mutex);
+
+static void kgdboc_restore_input_helper(struct work_struct *dummy)
+{
+	/*
+	 * We need to take a mutex to prevent several instances of
+	 * this work running on different CPUs so they don't try
+	 * to register again already registered handler.
+	 */
+	mutex_lock(&kgdboc_reset_mutex);
+
+	if (input_register_handler(&kgdboc_reset_handler) == 0)
+		input_unregister_handler(&kgdboc_reset_handler);
+
+	mutex_unlock(&kgdboc_reset_mutex);
+}
+
+static DECLARE_WORK(kgdboc_restore_input_work, kgdboc_restore_input_helper);
+
+static void kgdboc_restore_input(void)
+{
+	schedule_work(&kgdboc_restore_input_work);
+}
+
 static int kgdboc_register_kbd(char **cptr)
 {
 	if (strncmp(*cptr, "kbd", 3) == 0) {
@@ -64,10 +120,12 @@ static void kgdboc_unregister_kbd(void)
 			i--;
 		}
 	}
+	flush_work_sync(&kgdboc_restore_input_work);
 }
 #else /* ! CONFIG_KDB_KEYBOARD */
 #define kgdboc_register_kbd(x) 0
 #define kgdboc_unregister_kbd()
+#define kgdboc_restore_input()
 #endif /* ! CONFIG_KDB_KEYBOARD */
 
 static int kgdboc_option_setup(char *opt)
@@ -231,6 +289,7 @@ static void kgdboc_post_exp_handler(void)
 		dbg_restore_graphics = 0;
 		con_debug_leave();
 	}
+	kgdboc_restore_input();
 }
 
 static struct kgdb_io kgdboc_io_ops = {
-- 
1.6.3.3

[RFC PATCH 5/5] usb,keyboard,kdb: Implement HID keyboard polling

[Jason Wessel]

This patch adds support to kdb for usb keyboards using the in kernel
API for polling a specific device on a usb hcd.

The usb hid-core requires two types of modifications:

  1) The hid-core must call back to kdb anytime a keyboard is attached
     or removed in order to allow kdb to know what keyboard devices
     should be polled.

  2) If the debugger_core is active the urb packet data needs to get
     sent directly to kdb.

The modifications to kgdboc were simply to add the additional keyboard
poll hook type as well as to call the function to flush usb queue when
nearing the time to restart normal kernel execution.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
CC: Greg Kroah-Hartman <gregkh@suse.de>
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/hid/usbhid/hid-core.c   |   18 +++-
 drivers/serial/kgdboc.c         |   15 +++
 include/linux/kdb.h             |   13 +++
 kernel/debug/kdb/kdb_keyboard.c |  195 ++++++++++++++++++++++++++++++++++++++-
 lib/Kconfig.kgdb                |    7 ++
 5 files changed, 244 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5489eab..db4f3b7 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -35,6 +35,8 @@
 #include <linux/hiddev.h>
 #include <linux/hid-debug.h>
 #include <linux/hidraw.h>
+#include <linux/kgdb.h>
+#include <linux/kdb.h>
 #include "usbhid.h"
 
 /*
@@ -245,9 +247,17 @@ static void hid_irq_in(struct urb *urb)
 	case 0:			/* success */
 		usbhid_mark_busy(usbhid);
 		usbhid->retry_delay = 0;
-		hid_input_report(urb->context, HID_INPUT_REPORT,
-				 urb->transfer_buffer,
-				 urb->actual_length, 1);
+		if (unlikely(in_dbg_master() && hid && hid->driver &&
+			     urb->actual_length)) {
+			kdb_put_usb_char(usbhid->inbuf,
+					 interface_to_usbdev(usbhid->intf));
+			break;
+		} else {
+			hid_input_report(urb->context, HID_INPUT_REPORT,
+					 urb->transfer_buffer,
+					 urb->actual_length, 1);
+		}
+
 		/*
 		 * autosuspend refused while keys are pressed
 		 * because most keyboards don't wake up when
@@ -1055,6 +1065,7 @@ static int usbhid_start(struct hid_device *hid)
 				USB_INTERFACE_PROTOCOL_KEYBOARD) {
 		usbhid_set_leds(hid);
 		device_set_wakeup_enable(&dev->dev, 1);
+		kdb_keyboard_attach(dev);
 	}
 	return 0;
 
@@ -1076,6 +1087,7 @@ static void usbhid_stop(struct hid_device *hid)
 	if (WARN_ON(!usbhid))
 		return;
 
+	kdb_keyboard_detach(hid_to_usb_dev(hid));
 	clear_bit(HID_STARTED, &usbhid->iofl);
 	spin_lock_irq(&usbhid->lock);	/* Sync with error handler */
 	set_bit(HID_DISCONNECTED, &usbhid->iofl);
diff --git a/drivers/serial/kgdboc.c b/drivers/serial/kgdboc.c
index 3374618..99014d9 100644
--- a/drivers/serial/kgdboc.c
+++ b/drivers/serial/kgdboc.c
@@ -19,6 +19,7 @@
 #include <linux/console.h>
 #include <linux/vt_kern.h>
 #include <linux/input.h>
+#include <linux/usb.h>
 
 #define MAX_CONFIG_LEN		40
 
@@ -91,6 +92,9 @@ static DECLARE_WORK(kgdboc_restore_input_work, kgdboc_restore_input_helper);
 static void kgdboc_restore_input(void)
 {
 	schedule_work(&kgdboc_restore_input_work);
+#ifdef CONFIG_KDB_USB
+	usb_poll_irq_schedule_flush();
+#endif /* CONFIG_KDB_USB */
 }
 
 static int kgdboc_register_kbd(char **cptr)
@@ -99,6 +103,12 @@ static int kgdboc_register_kbd(char **cptr)
 		if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
 			kdb_poll_funcs[kdb_poll_idx] = kdb_get_kbd_char;
 			kdb_poll_idx++;
+#ifdef CONFIG_KDB_USB
+			if (kdb_poll_idx < KDB_POLL_FUNC_MAX) {
+				kdb_poll_funcs[kdb_poll_idx] = kdb_get_usb_char;
+				kdb_poll_idx++;
+			}
+#endif /* CONFIG_KDB_USB */
 			if (cptr[0][3] == ',')
 				*cptr += 4;
 			else
@@ -113,7 +123,12 @@ static void kgdboc_unregister_kbd(void)
 	int i;
 
 	for (i = 0; i < kdb_poll_idx; i++) {
+#ifdef CONFIG_KDB_USB
+		if (kdb_poll_funcs[i] == kdb_get_kbd_char ||
+			kdb_poll_funcs[i] == kdb_get_usb_char) {
+#else /* ! CONFIG_KDB_USB */
 		if (kdb_poll_funcs[i] == kdb_get_kbd_char) {
+#endif /* CONFIG_KDB_USB */
 			kdb_poll_idx--;
 			kdb_poll_funcs[i] = kdb_poll_funcs[kdb_poll_idx];
 			kdb_poll_funcs[kdb_poll_idx] = NULL;
diff --git a/include/linux/kdb.h b/include/linux/kdb.h
index aadff7c..62546f1 100644
--- a/include/linux/kdb.h
+++ b/include/linux/kdb.h
@@ -128,6 +128,19 @@ typedef int (*get_char_func)(void);
 extern get_char_func kdb_poll_funcs[];
 extern int kdb_get_kbd_char(void);
 
+#ifdef CONFIG_KDB_USB
+extern int kdb_no_usb;
+extern int kdb_get_usb_char(void);
+struct usb_device;
+void kdb_keyboard_attach(struct usb_device *dev);
+void kdb_keyboard_detach(struct usb_device *dev);
+extern void kdb_put_usb_char(char *buffer, struct usb_device *dev);
+#else /* ! CONFIG_KDB_USB */
+#define kdb_put_usb_char(x)
+#define kdb_keyboard_attach(x)
+#define kdb_keyboard_detach(x)
+#endif /* ! CONFIG_KDB_USB */
+
 static inline
 int kdb_process_cpu(const struct task_struct *p)
 {
diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
index 4bca634..1d41dc6 100644
--- a/kernel/debug/kdb/kdb_keyboard.c
+++ b/kernel/debug/kdb/kdb_keyboard.c
@@ -5,7 +5,7 @@
  * License.
  *
  * Copyright (c) 1999-2006 Silicon Graphics, Inc.  All Rights Reserved.
- * Copyright (c) 2009 Wind River Systems, Inc.  All Rights Reserved.
+ * Copyright (c) 2009-2010 Wind River Systems, Inc.  All Rights Reserved.
  */
 
 #include <linux/kdb.h>
@@ -13,6 +13,7 @@
 #include <linux/ctype.h>
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/usb.h>
 
 /* Keyboard Controller Registers on normal PCs. */
 
@@ -210,3 +211,195 @@ int kdb_get_kbd_char(void)
 	return keychar & 0xff;
 }
 EXPORT_SYMBOL_GPL(kdb_get_kbd_char);
+
+#ifdef CONFIG_KDB_USB
+static unsigned char kdb_usb_keycode[256] = {
+	  0,   0,   0,   0,  30,  48,  46,  32,  18,  33,  34,  35,  23,
+	 36,  37,  38,  50,  49,  24,  25,  16,  19,  31,  20,  22,  47,
+	 17,  45,  21,  44,   2,   3,   4,   5,   6,   7,   8,   9,  10,
+	 11,  28,   1,  14,  15,  57,  12,  13,  26,  27,  43,  84,  39,
+	 40,  41,  51,  52,  53,  58,  59,  60,  61,  62,  63,  64,  65,
+	 66,  67,  68,  87,  88,  99,  70, 119, 110, 102, 104, 111, 107,
+	109, 106, 105, 108, 103,  69,  98,  55,  74,  78,  96,  79,  80,
+	 81,  75,  76,  77,  71,  72,  73,  82,  83,  86, 127, 116, 117,
+	 85,  89,  90,  91,  92,  93,  94,  95, 120, 121, 122, 123, 134,
+	138, 130, 132, 128, 129, 131, 137, 133, 135, 136, 113, 115, 114,
+	  0,   0,   0, 124,   0, 181, 182, 183, 184, 185, 186, 187, 188,
+	189, 190, 191, 192, 193, 194, 195, 196, 197, 198,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
+	  0,   0,   0,  29,  42,  56, 125,  97,  54, 100, 126, 164, 166,
+	165, 163, 161, 115, 114, 113, 150, 158, 159, 128, 136, 177, 178,
+	176, 142, 152, 173, 140,
+};
+
+#define MAX_KEYBOARDS	8
+static struct usb_device *kdb_keyboard_dev[8];
+
+#define KDB_USB_RING_MAX 4
+static int kdb_usbkbd_head;
+static int kdb_usbkbd_tail;
+static int kdb_usbkbd_ring[KDB_USB_RING_MAX];
+static bool kdb_usbkbd_caps_lock;
+
+static void push_usbkbd_key(int keycode)
+{
+	int next = kdb_usbkbd_head + 1;
+
+	if (next >= KDB_USB_RING_MAX)
+		next = 0;
+	if (next == kdb_usbkbd_tail)
+		return;
+
+	kdb_usbkbd_ring[kdb_usbkbd_head] = keycode;
+	kdb_usbkbd_head = next;
+}
+
+static int kdb_pop_usbkbd_key(void)
+{
+	int next = kdb_usbkbd_tail + 1;
+	int ret = kdb_usbkbd_ring[kdb_usbkbd_tail];
+
+	if (kdb_usbkbd_tail == kdb_usbkbd_head)
+		return -1;
+	if (next >= KDB_USB_RING_MAX)
+		next = 0;
+	kdb_usbkbd_tail = next;
+	return ret;
+}
+
+#define MAX_KEYS_DOWN 4
+static int kbdusb_keys_down[MAX_KEYS_DOWN];
+static int kbdusb_idx;
+
+/*
+ * This function receive input from usb keyboard devices
+ */
+void kdb_put_usb_char(char *buffer, struct usb_device *dev)
+{
+	unsigned char keycode, spec;
+	int i, j, found;
+
+	/* Mark keys up if they are no longer down */
+	for (i = 0; i < kbdusb_idx; i++) {
+		for (j = 0; j < MAX_KEYS_DOWN; j++) {
+			if (kbdusb_keys_down[i] == buffer[2+j])
+				break;
+		}
+		if (j == MAX_KEYS_DOWN) {
+			kbdusb_idx--;
+			kbdusb_keys_down[i] = kbdusb_keys_down[kbdusb_idx];
+			i--;
+			if (kbdusb_idx == 0)
+				break;
+		}
+	}
+
+	for (i = 0; i < MAX_KEYS_DOWN; i++) {
+		if (!buffer[2+i])
+			break;
+
+		keycode = buffer[2+i];
+		buffer[2+i] = 0;
+		spec = buffer[0];
+		buffer[0] = 0;
+
+		/* if the key was previously down, skip it */
+		found = 0;
+		for (j = 0; j < kbdusb_idx; j++)
+			if (keycode == kbdusb_keys_down[j]) {
+				found = 1;
+				break;
+			}
+		if (found)
+			continue;
+
+		if (kbdusb_idx < MAX_KEYS_DOWN) {
+			kbdusb_keys_down[kbdusb_idx] = keycode;
+			kbdusb_idx++;
+		}
+		/* A normal key is pressed, decode it */
+		if (keycode)
+			keycode = kdb_usb_keycode[keycode];
+
+		/* 2 Keys pressed at one time ? */
+		if (spec && keycode) {
+			switch (spec) {
+			case 0x2:
+			case 0x20: /* Shift */
+				push_usbkbd_key(key_maps[1][keycode]);
+				break;
+			case 0x1:
+			case 0x10: /* Ctrl */
+				push_usbkbd_key(key_maps[4][keycode]);
+				break;
+			case 0x4:
+			case 0x40: /* Alt */
+				break;
+			}
+		} else if (keycode) { /* If only one key pressed */
+			switch (keycode) {
+			case 0x1C: /* Enter */
+				push_usbkbd_key(13);
+				break;
+			case 0x3A: /* Capslock */
+				kdb_usbkbd_caps_lock = !kdb_usbkbd_caps_lock;
+				break;
+			case 0x0E: /* Backspace */
+				push_usbkbd_key(8);
+				break;
+			case 0x0F: /* TAB */
+				push_usbkbd_key(9);
+				break;
+			case 0x77: /* Pause */
+				break;
+			default:
+				if (!kdb_usbkbd_caps_lock)
+					push_usbkbd_key(plain_map[keycode]);
+				else
+					push_usbkbd_key(key_maps[1][keycode]);
+				break;
+			}
+		}
+	}
+}
+
+void kdb_keyboard_attach(struct usb_device *dev)
+{
+	int i;
+
+	for (i = 0; i < MAX_KEYBOARDS; i++)
+		if (kdb_keyboard_dev[i] == NULL) {
+			kdb_keyboard_dev[i] = dev;
+			break;
+		}
+}
+
+void kdb_keyboard_detach(struct usb_device *dev)
+{
+	int i;
+
+	for (i = 0; i < MAX_KEYBOARDS; i++)
+		if (kdb_keyboard_dev[i] == dev) {
+			kdb_keyboard_dev[i] = NULL;
+			break;
+		}
+}
+
+int kdb_get_usb_char(void)
+{
+	int ret = kdb_pop_usbkbd_key();
+	int i;
+
+	if (ret >= 0)
+		return ret;
+	for (i = 0; i < MAX_KEYBOARDS; i++)
+		if (kdb_keyboard_dev[i])
+			usb_poll_irq(kdb_keyboard_dev[i]);
+	return kdb_pop_usbkbd_key();
+}
+
+#endif /* CONFIG_KDB_USB */
diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
index 43cb93f..a13e113 100644
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -79,4 +79,11 @@ config KDB_KEYBOARD
 	help
 	  KDB can use a PS/2 type keyboard for an input device
 
+config KDB_USB
+	bool "KGDB_KDB: Allow usb input device devices"
+	depends on VT && KGDB_KDB && KDB_KEYBOARD
+	default y
+	help
+	  KDB can use a USB keyboard for an input device
+
 endif # KGDB
-- 
1.6.3.3

Re: [RFC PATCH 1/5] usb-hcd: implement polling a specific usb device

[Alan Stern]

On Fri, 5 Nov 2010, Jason Wessel wrote:

> This patch adds a generic capability to poll a specific usb device and
> run its completion handler.
> 
> There will be two users of this functionality.
>  1) usb serial port console devices
>  2) usb keyboards for use with kdb
> 
> Any time the usb_irq_poll() function is called it has the possibility
> to queue some urbs for completion at a later time.  Any code that uses
> the usb_irq_poll() function must call usb_poll_irq_schedule_flush()
> before all the other usb devices will start working again.

...

> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c

> @@ -1562,6 +1567,17 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>  
>  	/* pass ownership to the completion handler */
>  	urb->status = status;
> +	if (unlikely(usb_poll_hcd_device)) {
> +		/*
> +		 * Any other device than the one being polled should get
> +		 * queued for a later flush.
> +		 */
> +		if (usb_poll_hcd_device != urb->dev) {
> +			INIT_LIST_HEAD(&urb->poll_list);

Why initialize the list_head here if the next line is going to 
overwrite it?

> +			list_add(&urb->poll_list, &delayed_usb_complete_queue);

You need to use list_add_tail, not list_add.  This is supposed to be a 
queue, not a LIFO stack.

> +			return;
> +		}
> +	}
>  	urb->complete (urb);
>  	atomic_dec (&urb->use_count);
>  	if (unlikely(atomic_read(&urb->reject)))
> @@ -2425,6 +2441,47 @@ usb_hcd_platform_shutdown(struct platform_device* dev)
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_platform_shutdown);
>  
> +void usb_poll_irq(struct usb_device *udev)
> +{

Please add a little kerneldoc explaining what this function does and 
why it is here.  Also mention that it must be called with interrupts 
disabled.

> +	struct usb_hcd *hcd;
> +
> +	hcd = bus_to_hcd(udev->bus);
> +	usb_poll_hcd_device = udev;
> +	usb_hcd_irq(0, hcd);
> +	usb_poll_hcd_device = NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_poll_irq);
> +
> +static void usb_poll_irq_flush_helper(struct work_struct *dummy)
> +{
> +	struct urb *urb;
> +	struct urb *scratch;
> +	unsigned long flags;
> +
> +	mutex_lock(&usb_poll_irq_flush_mutex);

What is this mutex used for?

> +	local_irq_save(flags);

Isn't this function meant to be called with interrupts disabled on all 
CPUs anyway?

> +	list_for_each_entry_safe(urb, scratch, &delayed_usb_complete_queue,
> +				 poll_list) {
> +		list_del(&urb->poll_list);
> +		urb->complete(urb);
> +		atomic_dec(&urb->use_count);
> +		if (unlikely(atomic_read(&urb->reject)))
> +			wake_up(&usb_kill_urb_queue);
> +		usb_put_urb(urb);
> +	}
> +	local_irq_restore(flags);
> +	mutex_unlock(&usb_poll_irq_flush_mutex);
> +}
> +
> +static DECLARE_WORK(usb_poll_irq_flush_work, usb_poll_irq_flush_helper);
> +
> +
> +void usb_poll_irq_schedule_flush(void)
> +{
> +	schedule_work(&usb_poll_irq_flush_work);
> +}
> +EXPORT_SYMBOL_GPL(usb_poll_irq_schedule_flush);

Why do you want to do this in a workqueue?  Just move all the code from 
usb_poll_irq_flush_helper over here directly.


> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h

> @@ -1188,6 +1192,7 @@ struct urb {
>  	struct list_head urb_list;	/* list head for use by the urb's
>  					 * current owner */
>  	struct list_head anchor_list;	/* the URB may be anchored */
> +	struct list_head poll_list;	/* Added to the poll queue */
>  	struct usb_anchor *anchor;
>  	struct usb_device *dev;		/* (in) pointer to associated device */
>  	struct usb_host_endpoint *ep;	/* (internal) pointer to endpoint */

You don't need to add an additional list_head to struct urb -- it
already contains too much stuff.  Simply use urb_list; that's what it's
for.

Alan Stern

Re: [RFC PATCH 2/5] usb-serial-console: try to poll the hcd vs dropping data

[Alan Stern]

On Fri, 5 Nov 2010, Jason Wessel wrote:

> This patch tries to solve the problem that printk data is dropped
> because there are too many outstanding transmit urb's while trying to
> execute printk's to a console.  You can observe this by booting a
> kernel with a serial console and cross checking the dmesg output with
> what you received on the console or doing something like
> "echo t > /proc/sysrq-trigger".
> 
> This patch takes the route of forcibly polling the hcd device to drain
> the urb queue in order to initiate the bulk write call backs.
> 
> A few millisecond penalty will get incurred to allow the hcd
> controller to complete a write urb, else the console data is thrown
> away.  Even with any kind of delay, the latency is still lower than
> using a traditional serial port uart due to the fact that there are
> bigger buffer for use with the USB serial console kfifo
> implementation.

> --- a/drivers/usb/serial/console.c
> +++ b/drivers/usb/serial/console.c
> @@ -197,13 +197,37 @@ static int usb_console_setup(struct console *co, char *options)
>  	return retval;
>  }
>  
> +static void usb_do_console_write(struct usb_serial *serial,
> +				 struct usb_serial_port *port,
> +				 const char *buf, unsigned count)
> +{
> +	int retval;
> +	int loops = 100;
> +try_again:
> +	/* pass on to the driver specific version of this function if
> +	   it is available */
> +	if (serial->type->write)
> +		retval = serial->type->write(NULL, port, buf, count);
> +	else
> +		retval = usb_serial_generic_write(NULL, port, buf, count);
> +	if (retval < count && retval >= 0 && loops--) {
> +		/* poll the hcd device because the queue is full */

In fact you can call the poll routine on every iteration.  Only the 
udelay needs to wait for the queue to be full.

> +		count -= retval;
> +		buf += retval;
> +		udelay(100);
> +		usb_poll_irq(serial->dev);
> +		usb_poll_irq_schedule_flush();

I don't understand this at all.  What's the point of adding the whole
delayed_usb_complete_queue mechanism if you flush the queue as soon as
some URBs are added to it?  Why not just let them complete normally
during the usb_poll_irq call?

I thought the whole idea was that you didn't want extraneous URBs to 
complete while the KDB debugger was running.  This means that you 
shouldn't call usb_poll_irq_schedule_flush until the debugger is ready 
to go back to running the main kernel.

Alan Stern

Re: [RFC PATCH 1/5] usb-hcd: implement polling a specific usb device

[Jason Wessel]

On 11/06/2010 01:32 PM, Alan Stern wrote:
> On Fri, 5 Nov 2010, Jason Wessel wrote:
> 
>> This patch adds a generic capability to poll a specific usb device and
>> run its completion handler.
>>
>> @@ -1562,6 +1567,17 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>>  
>>  	/* pass ownership to the completion handler */
>>  	urb->status = status;
>> +	if (unlikely(usb_poll_hcd_device)) {
>> +		/*
>> +		 * Any other device than the one being polled should get
>> +		 * queued for a later flush.
>> +		 */
>> +		if (usb_poll_hcd_device != urb->dev) {
>> +			INIT_LIST_HEAD(&urb->poll_list);
> 
> Why initialize the list_head here if the next line is going to 
> overwrite it?


Oops, I simply didn't know the list_add() obviated the need for
initialization.  Having now looked at the macro I see why.

> 
>> +			list_add(&urb->poll_list, &delayed_usb_complete_queue);
> 
> You need to use list_add_tail, not list_add.  This is supposed to be a 
> queue, not a LIFO stack.


Thanks for pointing that out.  All fixed.

>>  
>> +void usb_poll_irq(struct usb_device *udev)
>> +{
> 
> Please add a little kerneldoc explaining what this function does and 
> why it is here.  Also mention that it must be called with interrupts 
> disabled.


Sure.  I think it is also wise to put in: WARN_ON_ONCE(!irqs_disabled());


>> +static void usb_poll_irq_flush_helper(struct work_struct *dummy)
>> +{
>> +	struct urb *urb;
>> +	struct urb *scratch;
>> +	unsigned long flags;
>> +
>> +	mutex_lock(&usb_poll_irq_flush_mutex);
> 
> What is this mutex used for?


This mutex exists because I did not see a way to create a static
workqueue with the attribute WQ_NON_REENTRANT.  When the workqueue
does not have this attribute there is a corner case that between two
usb poll intervals (one being handled on cpu 0 and the next on cpu 1)
that you can get two units of work to run concurrently.

> 
>> +	local_irq_save(flags);
> 
> Isn't this function meant to be called with interrupts disabled on all 
> CPUs anyway?


The usb_poll_irq_flush_helper() is called from a work queue at some
point after the work had been originally been scheduled with the IRQ's
off.  We might be able to simply lock the HCD instead, but it looked
like the this code should really be called with the irq's off.  I came
to that conclusion based on looking at if the irqs were on or off in
the original context the HCD giveback was executed.

If you believe we don't need it, we'll simply remove the irq
save/restore.


>> +void usb_poll_irq_schedule_flush(void)
>> +{
>> +	schedule_work(&usb_poll_irq_flush_work);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_poll_irq_schedule_flush);
> 
> Why do you want to do this in a workqueue?  Just move all the code from 
> usb_poll_irq_flush_helper over here directly.
> 


I want this in a work queue because we need to schedule this to run at
a later point when the interrupts are restored.  If I don't put the
code here, it has to get duplicated in the kdb keyboard stack, as well
as the usb serial console code.  The alternative is that we need a
logical place to drain the queue at another point in the kernel or a
timer.

The "schedule later via a workqueue" seemed the path of least
resistance.

You could argue that we might have a user of this API that would want
to flush the delayed urb queue in a safe place.  For now though, the
two users of the new poll API (kdb, and usb serial console) do not
need it and this leads me to believe we would be best served to not
export the flush helper.


> 
>> --- a/include/linux/usb.h
>> @@ -1188,6 +1192,7 @@ struct urb {
>> +	struct list_head poll_list;	/* Added to the poll queue */
> 
> You don't need to add an additional list_head to struct urb -- it
> already contains too much stuff.  Simply use urb_list; that's what it's
> for.
> 


It looked to me like it might have already been used when I was
sifting through the other code in drivers/usb/*, but if you say it is
safe, I trust you.  :-)

A new copy of the patch is included below.

Jason.

---
 drivers/usb/core/hcd.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h    |    4 ++
 2 files changed, 79 insertions(+)

--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -96,6 +96,10 @@ struct usb_busmap {
 };
 static struct usb_busmap busmap;
 
+/* Support polling for a single device's urbs */
+static struct usb_device *usb_poll_hcd_device;
+static LIST_HEAD(delayed_usb_complete_queue);
+
 /* used when updating list of hcds */
 DEFINE_MUTEX(usb_bus_list_lock);	/* exported only for usbfs */
 EXPORT_SYMBOL_GPL (usb_bus_list_lock);
@@ -1528,6 +1532,7 @@ int usb_hcd_unlink_urb (struct urb *urb,
 }
 
 /*-------------------------------------------------------------------------*/
+static DEFINE_MUTEX(usb_poll_irq_flush_mutex);
 
 /**
  * usb_hcd_giveback_urb - return URB from HCD to device driver
@@ -1562,6 +1567,16 @@ void usb_hcd_giveback_urb(struct usb_hcd
 
 	/* pass ownership to the completion handler */
 	urb->status = status;
+	if (unlikely(usb_poll_hcd_device)) {
+		/*
+		 * Any other device than the one being polled should get
+		 * queued for a later flush.
+		 */
+		if (usb_poll_hcd_device != urb->dev) {
+			list_add_tail(&urb->urb_list, &delayed_usb_complete_queue);
+			return;
+		}
+	}
 	urb->complete (urb);
 	atomic_dec (&urb->use_count);
 	if (unlikely(atomic_read(&urb->reject)))
@@ -2425,6 +2440,66 @@ usb_hcd_platform_shutdown(struct platfor
 }
 EXPORT_SYMBOL_GPL(usb_hcd_platform_shutdown);
 
+/**
+ * usb_poll_irq - run HCD to call all urb->complete's for a usb device
+ * @udev: The usb device to poll
+ *
+ * The intent of this function is to run the completion handlers for
+ * any urbs for the specified usb device.  Any other devices will have
+ * the urb completions queued to be run at a later point.  Any user of
+ * this function needs call usb_poll_irq_schedule_flush() at a later
+ * point to schedule the processing of the queue.  This function MUST
+ * be called with the interrupts off.
+ */
+void usb_poll_irq(struct usb_device *udev)
+{
+	struct usb_hcd *hcd;
+
+	WARN_ON_ONCE(!irqs_disabled());
+	hcd = bus_to_hcd(udev->bus);
+	usb_poll_hcd_device = udev;
+	usb_hcd_irq(0, hcd);
+	usb_poll_hcd_device = NULL;
+}
+EXPORT_SYMBOL_GPL(usb_poll_irq);
+
+static void usb_poll_irq_flush_helper(struct work_struct *dummy)
+{
+	struct urb *urb;
+	struct urb *scratch;
+	unsigned long flags;
+
+	WARN_ON_ONCE(!irqs_disabled());
+	mutex_lock(&usb_poll_irq_flush_mutex);
+	local_irq_save(flags);
+	list_for_each_entry_safe(urb, scratch, &delayed_usb_complete_queue,
+				 urb_list) {
+		list_del(&urb->urb_list);
+		urb->complete(urb);
+		atomic_dec(&urb->use_count);
+		if (unlikely(atomic_read(&urb->reject)))
+			wake_up(&usb_kill_urb_queue);
+		usb_put_urb(urb);
+	}
+	local_irq_restore(flags);
+	mutex_unlock(&usb_poll_irq_flush_mutex);
+}
+
+static DECLARE_WORK(usb_poll_irq_flush_work, usb_poll_irq_flush_helper);
+
+/**
+ * usb_poll_irq_sechedule_flush()
+ *
+ * For any function that invoked usb_poll_irq() this function needs to
+ * be called to schedule the draining of the urb completion queue in
+ * order to restore normal processing of the usb devices.
+ */
+void usb_poll_irq_schedule_flush(void)
+{
+	schedule_work(&usb_poll_irq_flush_work);
+}
+EXPORT_SYMBOL_GPL(usb_poll_irq_schedule_flush);
+
 /*-------------------------------------------------------------------------*/
 
 #if defined(CONFIG_USB_MON) || defined(CONFIG_USB_MON_MODULE)
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -558,6 +558,10 @@ static inline void usb_mark_last_busy(st
 
 /*-------------------------------------------------------------------------*/
 
+/* for polling the hcd device */
+extern void usb_poll_irq(struct usb_device *udev);
+extern void usb_poll_irq_schedule_flush(void);
+
 /* for drivers using iso endpoints */
 extern int usb_get_current_frame_number(struct usb_device *usb_dev);
 

Re: [RFC PATCH 2/5] usb-serial-console: try to poll the hcd vs dropping data

[Jason Wessel]

On 11/06/2010 01:39 PM, Alan Stern wrote:
> On Fri, 5 Nov 2010, Jason Wessel wrote:
> 
>> This patch tries to solve the problem that printk data is dropped
>> because there are too many outstanding transmit urb's while trying to
>> execute printk's to a console.  You can observe this by booting a
>> kernel with a serial console and cross checking the dmesg output with
>> what you received on the console or doing something like
>> "echo t > /proc/sysrq-trigger".
>>
>> This patch takes the route of forcibly polling the hcd device to drain
>> the urb queue in order to initiate the bulk write call backs.
>>
>> A few millisecond penalty will get incurred to allow the hcd
>> controller to complete a write urb, else the console data is thrown
>> away.  Even with any kind of delay, the latency is still lower than
>> using a traditional serial port uart due to the fact that there are
>> bigger buffer for use with the USB serial console kfifo
>> implementation.
> 
>> --- a/drivers/usb/serial/console.c
>> +++ b/drivers/usb/serial/console.c
>> @@ -197,13 +197,37 @@ static int usb_console_setup(struct console *co, char *options)
>>  	return retval;
>>  }
>>  
>> +static void usb_do_console_write(struct usb_serial *serial,
>> +				 struct usb_serial_port *port,
>> +				 const char *buf, unsigned count)
>> +{
>> +	int retval;
>> +	int loops = 100;
>> +try_again:
>> +	/* pass on to the driver specific version of this function if
>> +	   it is available */
>> +	if (serial->type->write)
>> +		retval = serial->type->write(NULL, port, buf, count);
>> +	else
>> +		retval = usb_serial_generic_write(NULL, port, buf, count);
>> +	if (retval < count && retval >= 0 && loops--) {
>> +		/* poll the hcd device because the queue is full */
> 
> In fact you can call the poll routine on every iteration.  Only the 
> udelay needs to wait for the queue to be full.


I figured it would be best to sleep a bit but if you feel we should be
able to poll every time we could go with a udelay(1) and loops =
10000, or udelay(10) and loops(1000).


> 
>> +		count -= retval;
>> +		buf += retval;
>> +		udelay(100);
>> +		usb_poll_irq(serial->dev);
>> +		usb_poll_irq_schedule_flush();
> 
> I don't understand this at all.  What's the point of adding the whole
> delayed_usb_complete_queue mechanism if you flush the queue as soon as
> some URBs are added to it?  Why not just let them complete normally
> during the usb_poll_irq call?
> 

The schedule function schedules the work for outside this context and
just leaves one unit of work waiting for a later point.

> I thought the whole idea was that you didn't want extraneous URBs to 
> complete while the KDB debugger was running.  This means that you 
> shouldn't call usb_poll_irq_schedule_flush until the debugger is ready 
> to go back to running the main kernel.


This really has nothing to do with kdb/kgdb.  This is purely for the
usb serial console and printk().  In the case of the kdb keyboard
handler, yes it schedules the flush just before resuming the kernel.

I attempted to explain the need to schedule/defer the work of flushing
the urb completion queue in response to your comments about the usb
poll api calls.


There is another series of experimental patches in my backlog for
implementing kgdb/kdb over USB serial, but numerous issues still exist
and will keep that development off for some time to the future:

  * complete the USB hcd polling API (this series)
  * usb serial console can push buffers when KFIFO's are full (this series)
  * Extensions for CONSOLE_POLL for usb serial implemented
  * sysrq as a tasklet (don't run the sysrq while holding the hcd locks)
  * Some new code to attempt to run any hcd threads (to get out of locks)
    on entry to the debug core

Jason.

Re: [RFC PATCH 1/5] usb-hcd: implement polling a specific usb device

[Alan Stern]

On Mon, 8 Nov 2010, Jason Wessel wrote:

> >> +static void usb_poll_irq_flush_helper(struct work_struct *dummy)
> >> +{
> >> +	struct urb *urb;
> >> +	struct urb *scratch;
> >> +	unsigned long flags;
> >> +
> >> +	mutex_lock(&usb_poll_irq_flush_mutex);
> > 
> > What is this mutex used for?
> 
> 
> This mutex exists because I did not see a way to create a static
> workqueue with the attribute WQ_NON_REENTRANT.  When the workqueue
> does not have this attribute there is a corner case that between two
> usb poll intervals (one being handled on cpu 0 and the next on cpu 1)
> that you can get two units of work to run concurrently.

So this reduces to the more fundamental question of why you need to use 
a workqueue in the first place.

> >> +	local_irq_save(flags);
> > 
> > Isn't this function meant to be called with interrupts disabled on all 
> > CPUs anyway?
> 
> 
> The usb_poll_irq_flush_helper() is called from a work queue at some
> point after the work had been originally been scheduled with the IRQ's
> off.  We might be able to simply lock the HCD instead, but it looked
> like the this code should really be called with the irq's off.  I came
> to that conclusion based on looking at if the irqs were on or off in
> the original context the HCD giveback was executed.
> 
> If you believe we don't need it, we'll simply remove the irq
> save/restore.

Oh, there's no question that interrupts need to be disabled.  However 
if this routine isn't called from a workqueue then interrupts will 
already be disabled.

> >> +void usb_poll_irq_schedule_flush(void)
> >> +{
> >> +	schedule_work(&usb_poll_irq_flush_work);
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_poll_irq_schedule_flush);
> > 
> > Why do you want to do this in a workqueue?  Just move all the code from 
> > usb_poll_irq_flush_helper over here directly.
> > 
> 
> 
> I want this in a work queue because we need to schedule this to run at
> a later point when the interrupts are restored.

Why?  Why not run it before interrupts are restored?

>  If I don't put the
> code here, it has to get duplicated in the kdb keyboard stack, as well
> as the usb serial console code.  The alternative is that we need a
> logical place to drain the queue at another point in the kernel or a
> timer.

The queue should be drained just before you exit the debugger.

> The "schedule later via a workqueue" seemed the path of least
> resistance.
> 
> You could argue that we might have a user of this API that would want
> to flush the delayed urb queue in a safe place.  For now though, the
> two users of the new poll API (kdb, and usb serial console) do not
> need it and this leads me to believe we would be best served to not
> export the flush helper.

Why should usb serial console need to use the queue?

Let's put it this way: Your polling mechanism has conflated two 
separate issues.  The matter of polling the host controller driver is 
distinct from the question of which URBs to stick on the queue.

While kdb is running, it makes sense to give back only the URBs that 
are for the console device.  But if you are polling during the normal 
operation of printk and the usb serial console, then there's no reason 
not to give back all the URBs that complete during polling.

In other words, usb_poll_irq() doesn't need to set or clear
usb_poll_hcd_device.  (Incidentally, that's not a very good name -- I'd 
change to it usb_poll_device.)  Instead, set that pointer when you 
enter the debugger and clear it when you leave the debugger, at which 
time you can drain the queue.

> >> --- a/include/linux/usb.h
> >> @@ -1188,6 +1192,7 @@ struct urb {
> >> +	struct list_head poll_list;	/* Added to the poll queue */
> > 
> > You don't need to add an additional list_head to struct urb -- it
> > already contains too much stuff.  Simply use urb_list; that's what it's
> > for.
> > 
> 
> 
> It looked to me like it might have already been used when I was
> sifting through the other code in drivers/usb/*, but if you say it is
> safe, I trust you.  :-)

As the kerneldoc comment says, it is for use by whoever owns the URB.  
Just before the URB is given back to the driver, the USB core no longer 
owns it and the driver doesn't yet own it.  Hence kdb can take 
ownership of the URB.

> >> +static void usb_do_console_write(struct usb_serial *serial,
> >> +				 struct usb_serial_port *port,
> >> +				 const char *buf, unsigned count)
> >> +{
> >> +	int retval;
> >> +	int loops = 100;
> >> +try_again:
> >> +	/* pass on to the driver specific version of this function if
> >> +	   it is available */
> >> +	if (serial->type->write)
> >> +		retval = serial->type->write(NULL, port, buf, count);
> >> +	else
> >> +		retval = usb_serial_generic_write(NULL, port, buf, count);
> >> +	if (retval < count && retval >= 0 && loops--) {
> >> +		/* poll the hcd device because the queue is full */
> > 
> > In fact you can call the poll routine on every iteration.  Only the 
> > udelay needs to wait for the queue to be full.
> 
> 
> I figured it would be best to sleep a bit but if you feel we should be
> able to poll every time we could go with a udelay(1) and loops =
> 10000, or udelay(10) and loops(1000).

You don't need any delay when polling.  But polling does take some
time, so it will slow things down a little.  I don't know, perhaps
leaving it the way it is will be best for now.

Alan Stern

Re: [RFC PATCH 1/5] usb-hcd: implement polling a specific usb device

[Jason Wessel]

On 11/08/2010 01:19 PM, Alan Stern wrote:
> On Mon, 8 Nov 2010, Jason Wessel wrote:
> 
>>>> +static void usb_poll_irq_flush_helper(struct work_struct *dummy)
>>>> +{
>>>> +	struct urb *urb;
>>>> +	struct urb *scratch;
>>>> +	unsigned long flags;
>>>> +
>>>> +	mutex_lock(&usb_poll_irq_flush_mutex);
>>> What is this mutex used for?
>>
>> This mutex exists because I did not see a way to create a static
>> workqueue with the attribute WQ_NON_REENTRANT.  When the workqueue
>> does not have this attribute there is a corner case that between two
>> usb poll intervals (one being handled on cpu 0 and the next on cpu 1)
>> that you can get two units of work to run concurrently.
> 
> So this reduces to the more fundamental question of why you need to use 
> a workqueue in the first place.
>
[clip]
>
> The queue should be drained just before you exit the debugger.


I think this is the key issue here.  There is no debugger running.
The context of kdb has nothing to do with this patch.

In this case the input source was printk() constantly calling the usb
serial console write until there are no more kfifo buffers left and we
need to poll the usb hcd to push some of the data out.

The easiest example is to look at what happens when you set
console=ttyUSB0,115200 as a kernel boot argument.  

irq's off
  console_register(with usb device) {
    loop() {
       write_all_console_boot_log_so_far()
    }
  }
irq's on
... more printk's ...

It is not obvious where another good place exists to drain the queue.
You mentioned before interrupts are restored, but I don't see how we
can do that in this case without putting lots of other hook points or
potentially adding to the hot path of irq restore (which is obviously
a no-no).

What about another option?  Perhaps we drain the queue the next time
the HCD runs when it is not in the polling context?  If we do that,
then we can possibly eliminate the poll schedule all together, but I
believe there is a corner case where if you do not receive any irq
events for a while the queued urb completions hang in limbo.

> 
>> The "schedule later via a workqueue" seemed the path of least
>> resistance.
>>
>> You could argue that we might have a user of this API that would want
>> to flush the delayed urb queue in a safe place.  For now though, the
>> two users of the new poll API (kdb, and usb serial console) do not
>> need it and this leads me to believe we would be best served to not
>> export the flush helper.
> 
> Why should usb serial console need to use the queue?
> 
> Let's put it this way: Your polling mechanism has conflated two 
> separate issues.  The matter of polling the host controller driver is 
> distinct from the question of which URBs to stick on the queue.
> 
> While kdb is running, it makes sense to give back only the URBs that 
> are for the console device.  But if you are polling during the normal 
> operation of printk and the usb serial console, then there's no reason 
> not to give back all the URBs that complete during polling.
> 

That is an interesting way to look at things...  You metioned I
conflated to issues, but in my mind they were the same (kdb vs printk
requiremetns).  I just figured we should do the minimum possible when
trying to exec a printk() because we want it to be as reliable as
possible.  I had the following example in mind where a usb camera
driver executes a pile of printk()'s and the poll kicks in, we would
only like the completion handler to run for the usb serial device and
to defer the rest.

> In other words, usb_poll_irq() doesn't need to set or clear
> usb_poll_hcd_device.  (Incidentally, that's not a very good name -- I'd 
> change to it usb_poll_device.)

Seems resonable to me.  Its life has changed a bit since we originally
started talking about it.

> Instead, set that pointer when you enter the debugger and clear it
> when you leave the debugger, at which time you can drain the queue.

In the kdb keyboard patch we already have such a place we can do this,
and that is where the schedule logic occurs.  The other reason I opted
for the scheduling vs running it before kernel execution is completely
resumed is on the off chance something has other locks or mutexes
required by another cpu.

What about the printk() case though?  My argument for queuing and
scheduling was to minimize the ability that printk() will get deferred
by something else.


Thanks,
Jason.

Re: [RFC PATCH 1/5] usb-hcd: implement polling a specific usb device

[Alan Stern]

On Mon, 8 Nov 2010, Jason Wessel wrote:

> > Let's put it this way: Your polling mechanism has conflated two 
> > separate issues.  The matter of polling the host controller driver is 
> > distinct from the question of which URBs to stick on the queue.
> > 
> > While kdb is running, it makes sense to give back only the URBs that 
> > are for the console device.  But if you are polling during the normal 
> > operation of printk and the usb serial console, then there's no reason 
> > not to give back all the URBs that complete during polling.
> > 
> 
> That is an interesting way to look at things...  You metioned I
> conflated to issues, but in my mind they were the same (kdb vs printk
> requiremetns).  I just figured we should do the minimum possible when
> trying to exec a printk() because we want it to be as reliable as
> possible.  I had the following example in mind where a usb camera
> driver executes a pile of printk()'s and the poll kicks in, we would
> only like the completion handler to run for the usb serial device and
> to defer the rest.

It's true that drivers might not expect URB completions to occur in the
middle of a printk.  Particularly on UP systems with interrupts
disabled!  And there is always a possibility of deadlock: If an URB
completion handler takes a spinlock and then calls printk, draining the
queue could end up waiting on the same spinlock.  But of course the
same thing would happen regardless of whether or not URBs are deferred
via the queue.

It seems clear that you don't want to drain the queue while kdb is 
running, so the usb serial console code shouldn't do it.  This in turn 
implies that when polling during normal console operation, URBs should 
not be put on the queue.  Try doing things that way and see if you run 
into trouble.

> What about the printk() case though?  My argument for queuing and
> scheduling was to minimize the ability that printk() will get deferred
> by something else.

I don't think giving back URBs at the end of a printk is any better 
than giving them back in the middle of a printk.  For example, consider 
the case where there are two printk statements in a row.

The place you will really run into trouble is if the driver for the 
USB console serial device includes a printk in its completion handler!

Alan Stern