[PATCH 0/4] usb: hub: convert khubd into workqueue

[PATCH 0/4] usb: hub: convert khubd into workqueue

[Petr Mladek]

The workqueue API is well defined, it allows to schedule the work
as needed, and usually allows to avoid an extra thread at all.

This patchset converts khubd into the workqueue. It saves one thread,
lock, and list. IMHO, the new code is a bit cleaner.

It  looks huge but the main change is in the first patch. The rest is
simple renaming of functions, comments and documentaion.

Note that this is my first change of this type. I hope that I selected
reasonable parameters for the workqueue and did it a good way. Also
I hope that I got correctly the logic around hub_event_lock. The result
seems to work but any feedback and testing are much appreciated.


The patches are tested against linux-next. The last commit is
c3f2d223eae58504aae ("Add linux-next specific files for 20140911").

It depends on the patch from the mail "[PATCH] usb: hub: wrong order
of putting interfaces when processing" that was sent few minutes before.

It can be applied also on Linus' tree but you need to apply
the commit c605f3cdff53a743f6d875 ("usb: hub: take hub->hdev reference
when processing from eventlist") first.


Petr Mladek (4):
  usb: hub: convert khubd into workqueue
  usb: hub: remove obsolete while cycle in hub_event()
  usb: hub: rename *kick_khubd to *kick_hub_wq
  usb: hub: rename khubd to hub_wq in documentation and comments

 Documentation/DocBook/usb.tmpl             |   2 +-
 Documentation/usb/WUSB-Design-overview.txt |   6 +-
 Documentation/usb/hotplug.txt              |   2 +-
 drivers/net/usb/usbnet.c                   |  14 +-
 drivers/usb/README                         |   2 +-
 drivers/usb/core/hcd.c                     |  14 +-
 drivers/usb/core/hub.c                     | 357 +++++++++++++----------------
 drivers/usb/core/hub.h                     |   2 +-
 drivers/usb/core/usb.h                     |   2 +-
 drivers/usb/host/ehci-fsl.c                |   2 +-
 drivers/usb/host/ehci-hcd.c                |   2 +-
 drivers/usb/host/ehci-hub.c                |   8 +-
 drivers/usb/host/fhci-hcd.c                |   6 +-
 drivers/usb/host/fotg210-hcd.c             |   8 +-
 drivers/usb/host/fusbh200-hcd.c            |   8 +-
 drivers/usb/host/isp1760-hcd.c             |   6 +-
 drivers/usb/host/ohci-hcd.c                |   6 +-
 drivers/usb/host/ohci-hub.c                |   4 +-
 drivers/usb/host/ohci-omap.c               |   2 +-
 drivers/usb/host/oxu210hp-hcd.c            |  10 +-
 drivers/usb/host/sl811-hcd.c               |   8 +-
 drivers/usb/host/xhci-hub.c                |   2 +-
 drivers/usb/host/xhci.c                    |   4 +-
 drivers/usb/misc/usbtest.c                 |   2 +-
 drivers/usb/musb/am35x.c                   |   1 +
 drivers/usb/musb/tusb6010.c                |   2 +-
 drivers/usb/phy/phy-fsl-usb.c              |   2 +-
 drivers/usb/phy/phy-isp1301-omap.c         |   2 +-
 drivers/usb/wusbcore/devconnect.c          |   6 +-
 drivers/usb/wusbcore/wa-hc.h               |   2 +-
 sound/usb/midi.c                           |   2 +-
 31 files changed, 228 insertions(+), 268 deletions(-)

-- 
1.8.4

[PATCH 1/4] usb: hub: convert khubd into workqueue

[Petr Mladek]

There is no need to have separate kthread for handling USB hub events.
It is more elegant to use the workqueue framework.

The workqueue is allocated as unbound, cpu intensive, and freezable.
There does not seem to be any big advantage to run it on the same CPU.
The handler is taking a lock and thus could block for a longer time.
And finally, the original thread was freezable as well.

struct usb_hub is passed via the work item. Therefore we do not need
hub_event_list.

hub_events() is modified to process the given work item. It is renamed to
hub_event(). The while cycle will be removed in a followup patch. It helps
to see the real change here.

One nice thing is that we do not need hub_event_lock any longer. It was needed
when doing operations with hub_event_list and for balancing the calls
usb_autopm_get_interface_no_resume() and usb_autopm_put_interface_no_suspend().
It still works because the workqueue operations have their own locking.
Also cancel_work_sync() tells us whether any work item was canceled.
It means that we could put the interface either in hub_event() handler or when
the work item was successfully canceled.

The rest of the changes should be pretty straightforward.

Note that the source file is still full of the obsolete "khubd" strings.
Let's remove them in a follow up patch to keep this one "simple".

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 drivers/usb/core/hub.c | 114 +++++++++++++++++--------------------------------
 drivers/usb/core/hub.h |   2 +-
 2 files changed, 41 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 6afd79ee3340..1825af63c686 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -22,9 +22,8 @@
 #include <linux/usb/hcd.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/quirks.h>
-#include <linux/kthread.h>
+#include <linux/workqueue.h>
 #include <linux/mutex.h>
-#include <linux/freezer.h>
 #include <linux/random.h>
 #include <linux/pm_qos.h>
 
@@ -41,14 +40,9 @@
  * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */
 static DEFINE_SPINLOCK(device_state_lock);
 
-/* khubd's worklist and its lock */
-static DEFINE_SPINLOCK(hub_event_lock);
-static LIST_HEAD(hub_event_list);	/* List of hubs needing servicing */
-
-/* Wakes up khubd */
-static DECLARE_WAIT_QUEUE_HEAD(khubd_wait);
-
-static struct task_struct *khubd_task;
+/* workqueue to process hub events */
+static struct workqueue_struct *hub_wq;
+static void hub_event(struct work_struct *work);
 
 /* synchronize hub-port add/remove and peering operations */
 DEFINE_MUTEX(usb_port_peer_mutex);
@@ -577,18 +571,20 @@ static int hub_port_status(struct usb_hub *hub, int port1,
 
 static void kick_khubd(struct usb_hub *hub)
 {
-	unsigned long	flags;
-
-	spin_lock_irqsave(&hub_event_lock, flags);
-	if (!hub->disconnected && list_empty(&hub->event_list)) {
-		list_add_tail(&hub->event_list, &hub_event_list);
-
-		/* Suppress autosuspend until khubd runs */
+	if (!hub->disconnected && !work_pending(&hub->events)) {
+		/*
+		 * Suppress autosuspend until the event is proceed.
+		 *
+		 * Be careful and make sure that the symmetric operation is
+		 * always called. We are here only when there is no pending
+		 * work for this hub. Therefore put the interface either when
+		 * the new work is called or when it is canceled.
+		 */
 		usb_autopm_get_interface_no_resume(
 				to_usb_interface(hub->intfdev));
-		wake_up(&khubd_wait);
+		INIT_WORK(&hub->events, hub_event);
+		queue_work(hub_wq, &hub->events);
 	}
-	spin_unlock_irqrestore(&hub_event_lock, flags);
 }
 
 void usb_kick_khubd(struct usb_device *hdev)
@@ -1647,13 +1643,9 @@ static void hub_disconnect(struct usb_interface *intf)
 	int port1;
 
 	/* Take the hub off the event list and don't let it be added again */
-	spin_lock_irq(&hub_event_lock);
-	if (!list_empty(&hub->event_list)) {
-		list_del_init(&hub->event_list);
+	if (cancel_work_sync(&hub->events))
 		usb_autopm_put_interface_no_suspend(intf);
-	}
 	hub->disconnected = 1;
-	spin_unlock_irq(&hub_event_lock);
 
 	/* Disconnect all children and quiesce the hub */
 	hub->error = 0;
@@ -1793,7 +1785,6 @@ descriptor_error:
 	}
 
 	kref_init(&hub->kref);
-	INIT_LIST_HEAD(&hub->event_list);
 	hub->intfdev = &intf->dev;
 	hub->hdev = hdev;
 	INIT_DELAYED_WORK(&hub->leds, led_work);
@@ -4993,9 +4984,8 @@ static void port_event(struct usb_hub *hub, int port1)
 }
 
 
-static void hub_events(void)
+static void hub_event(struct work_struct *work)
 {
-	struct list_head *tmp;
 	struct usb_device *hdev;
 	struct usb_interface *intf;
 	struct usb_hub *hub;
@@ -5004,29 +4994,13 @@ static void hub_events(void)
 	u16 hubchange;
 	int i, ret;
 
-	/*
-	 *  We restart the list every time to avoid a deadlock with
-	 * deleting hubs downstream from this one. This should be
-	 * safe since we delete the hub from the event list.
-	 * Not the most efficient, but avoids deadlocks.
-	 */
+	/* temporary keep the cycle to show real changes in this commit */
 	while (1) {
-
-		/* Grab the first entry at the beginning of the list */
-		spin_lock_irq(&hub_event_lock);
-		if (list_empty(&hub_event_list)) {
-			spin_unlock_irq(&hub_event_lock);
-			break;
-		}
-
-		tmp = hub_event_list.next;
-		list_del_init(tmp);
-
-		hub = list_entry(tmp, struct usb_hub, event_list);
+		hub = container_of(work, struct usb_hub, events);
 		kref_get(&hub->kref);
+
 		hdev = hub->hdev;
 		usb_get_dev(hdev);
-		spin_unlock_irq(&hub_event_lock);
 
 		hub_dev = hub->intfdev;
 		intf = to_usb_interface(hub_dev);
@@ -5134,36 +5108,21 @@ static void hub_events(void)
 		/* Balance the usb_autopm_get_interface() above */
 		usb_autopm_put_interface(intf);
  loop:
+ loop_disconnected:
 		/* Balance the usb_autopm_get_interface_no_resume() in
 		 * kick_khubd() and allow autosuspend.
+		 *
+		 * It has to be done even when the hub was disconnected
+		 * when waiting for hdev lock. This function is called
+		 * and it means that usb_disconnect() was late in removing
+		 * this work from hub_wq.
 		 */
 		usb_autopm_put_interface_no_suspend(intf);
- loop_disconnected:
 		usb_unlock_device(hdev);
 		usb_put_dev(hdev);
 		kref_put(&hub->kref, hub_release);
-
-	} /* end while (1) */
-}
-
-static int hub_thread(void *__unused)
-{
-	/* khubd needs to be freezable to avoid interfering with USB-PERSIST
-	 * port handover.  Otherwise it might see that a full-speed device
-	 * was gone before the EHCI controller had handed its port over to
-	 * the companion full-speed controller.
-	 */
-	set_freezable();
-
-	do {
-		hub_events();
-		wait_event_freezable(khubd_wait,
-				!list_empty(&hub_event_list) ||
-				kthread_should_stop());
-	} while (!kthread_should_stop() || !list_empty(&hub_event_list));
-
-	pr_debug("%s: khubd exiting\n", usbcore_name);
-	return 0;
+		break;
+	}
 }
 
 static const struct usb_device_id hub_id_table[] = {
@@ -5203,21 +5162,28 @@ int usb_hub_init(void)
 		return -1;
 	}
 
-	khubd_task = kthread_run(hub_thread, NULL, "khubd");
-	if (!IS_ERR(khubd_task))
+	/*
+	 * The workqueue needs to be freezable to avoid interfering with
+	 * USB-PERSIST port handover. Otherwise it might see that a full-speed
+	 * device was gone before the EHCI controller had handed its port
+	 * over to the companion full-speed controller.
+	 */
+	hub_wq = alloc_workqueue("hub",
+				 WQ_UNBOUND | WQ_CPU_INTENSIVE | WQ_FREEZABLE,
+				 0);
+	if (hub_wq)
 		return 0;
 
 	/* Fall through if kernel_thread failed */
 	usb_deregister(&hub_driver);
-	printk(KERN_ERR "%s: can't start khubd\n", usbcore_name);
+	pr_err("%s: can't allocate workqueue for hub\n", usbcore_name);
 
 	return -1;
 }
 
 void usb_hub_cleanup(void)
 {
-	kthread_stop(khubd_task);
-
+	destroy_workqueue(hub_wq);
 	/*
 	 * Hub resources are freed for us by usb_deregister. It calls
 	 * usb_driver_purge on every device which in turn calls that
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index c77d8778af4b..688817fb3246 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -41,7 +41,6 @@ struct usb_hub {
 	int			error;		/* last reported error */
 	int			nerrors;	/* track consecutive errors */
 
-	struct list_head	event_list;	/* hubs w/data or errs ready */
 	unsigned long		event_bits[1];	/* status change bitmask */
 	unsigned long		change_bits[1];	/* ports with logical connect
 							status change */
@@ -77,6 +76,7 @@ struct usb_hub {
 	u8			indicator[USB_MAXCHILDREN];
 	struct delayed_work	leds;
 	struct delayed_work	init_work;
+	struct work_struct      events;
 	struct usb_port		**ports;
 };
 
-- 
1.8.4

[PATCH 2/4] usb: hub: remove obsolete while cycle in hub_event()

[Petr Mladek]

The USB hub events are proceed by workqueue instead of kthread now.
The result is that hub_event() function processes only one event.
The while cycle was not removed earlier to show the real changes when
switching to the workqueue.

This patch also consolidates the goto targets and rename them from "loop*"
to "out*".

When touching the code, it fixes also formatting of dev_err() and dev_dbg()
calls to make checkpatch.pl happy :-)

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 drivers/usb/core/hub.c | 199 +++++++++++++++++++++++--------------------------
 1 file changed, 95 insertions(+), 104 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 1825af63c686..b31eec65caf7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4994,121 +4994,114 @@ static void hub_event(struct work_struct *work)
 	u16 hubchange;
 	int i, ret;
 
-	/* temporary keep the cycle to show real changes in this commit */
-	while (1) {
-		hub = container_of(work, struct usb_hub, events);
-		kref_get(&hub->kref);
-
-		hdev = hub->hdev;
-		usb_get_dev(hdev);
-
-		hub_dev = hub->intfdev;
-		intf = to_usb_interface(hub_dev);
-		dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
-				hdev->state, hdev->maxchild,
-				/* NOTE: expects max 15 ports... */
-				(u16) hub->change_bits[0],
-				(u16) hub->event_bits[0]);
-
-		/* Lock the device, then check to see if we were
-		 * disconnected while waiting for the lock to succeed. */
-		usb_lock_device(hdev);
-		if (unlikely(hub->disconnected))
-			goto loop_disconnected;
-
-		/* If the hub has died, clean up after it */
-		if (hdev->state == USB_STATE_NOTATTACHED) {
-			hub->error = -ENODEV;
-			hub_quiesce(hub, HUB_DISCONNECT);
-			goto loop;
-		}
+	hub = container_of(work, struct usb_hub, events);
+	kref_get(&hub->kref);
 
-		/* Autoresume */
-		ret = usb_autopm_get_interface(intf);
-		if (ret) {
-			dev_dbg(hub_dev, "Can't autoresume: %d\n", ret);
-			goto loop;
-		}
-
-		/* If this is an inactive hub, do nothing */
-		if (hub->quiescing)
-			goto loop_autopm;
+	hdev = hub->hdev;
+	usb_get_dev(hdev);
+
+	hub_dev = hub->intfdev;
+	intf = to_usb_interface(hub_dev);
+	dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
+			hdev->state, hdev->maxchild,
+			/* NOTE: expects max 15 ports... */
+			(u16) hub->change_bits[0],
+			(u16) hub->event_bits[0]);
+
+	/* Lock the device, then check to see if we were
+	 * disconnected while waiting for the lock to succeed. */
+	usb_lock_device(hdev);
+	if (unlikely(hub->disconnected))
+		goto out;
+
+	/* If the hub has died, clean up after it */
+	if (hdev->state == USB_STATE_NOTATTACHED) {
+		hub->error = -ENODEV;
+		hub_quiesce(hub, HUB_DISCONNECT);
+		goto out;
+	}
+
+	/* Autoresume */
+	ret = usb_autopm_get_interface(intf);
+	if (ret) {
+		dev_dbg(hub_dev, "Can't autoresume: %d\n", ret);
+		goto out;
+	}
 
-		if (hub->error) {
-			dev_dbg (hub_dev, "resetting for error %d\n",
-				hub->error);
+	/* If this is an inactive hub, do nothing */
+	if (hub->quiescing)
+		goto out_autopm;
 
-			ret = usb_reset_device(hdev);
-			if (ret) {
-				dev_dbg (hub_dev,
-					"error resetting hub: %d\n", ret);
-				goto loop_autopm;
-			}
+	if (hub->error) {
+		dev_dbg(hub_dev, "resetting for error %d\n", hub->error);
 
-			hub->nerrors = 0;
-			hub->error = 0;
+		ret = usb_reset_device(hdev);
+		if (ret) {
+			dev_dbg(hub_dev, "error resetting hub: %d\n", ret);
+			goto out_autopm;
 		}
 
-		/* deal with port status changes */
-		for (i = 1; i <= hdev->maxchild; i++) {
-			struct usb_port *port_dev = hub->ports[i - 1];
+		hub->nerrors = 0;
+		hub->error = 0;
+	}
 
-			if (test_bit(i, hub->event_bits)
-					|| test_bit(i, hub->change_bits)
-					|| test_bit(i, hub->wakeup_bits)) {
-				/*
-				 * The get_noresume and barrier ensure that if
-				 * the port was in the process of resuming, we
-				 * flush that work and keep the port active for
-				 * the duration of the port_event().  However,
-				 * if the port is runtime pm suspended
-				 * (powered-off), we leave it in that state, run
-				 * an abbreviated port_event(), and move on.
-				 */
-				pm_runtime_get_noresume(&port_dev->dev);
-				pm_runtime_barrier(&port_dev->dev);
-				usb_lock_port(port_dev);
-				port_event(hub, i);
-				usb_unlock_port(port_dev);
-				pm_runtime_put_sync(&port_dev->dev);
-			}
+	/* deal with port status changes */
+	for (i = 1; i <= hdev->maxchild; i++) {
+		struct usb_port *port_dev = hub->ports[i - 1];
+
+		if (test_bit(i, hub->event_bits)
+				|| test_bit(i, hub->change_bits)
+				|| test_bit(i, hub->wakeup_bits)) {
+			/*
+			 * The get_noresume and barrier ensure that if
+			 * the port was in the process of resuming, we
+			 * flush that work and keep the port active for
+			 * the duration of the port_event().  However,
+			 * if the port is runtime pm suspended
+			 * (powered-off), we leave it in that state, run
+			 * an abbreviated port_event(), and move on.
+			 */
+			pm_runtime_get_noresume(&port_dev->dev);
+			pm_runtime_barrier(&port_dev->dev);
+			usb_lock_port(port_dev);
+			port_event(hub, i);
+			usb_unlock_port(port_dev);
+			pm_runtime_put_sync(&port_dev->dev);
 		}
+	}
 
-		/* deal with hub status changes */
-		if (test_and_clear_bit(0, hub->event_bits) == 0)
-			;	/* do nothing */
-		else if (hub_hub_status(hub, &hubstatus, &hubchange) < 0)
-			dev_err (hub_dev, "get_hub_status failed\n");
-		else {
-			if (hubchange & HUB_CHANGE_LOCAL_POWER) {
-				dev_dbg (hub_dev, "power change\n");
-				clear_hub_feature(hdev, C_HUB_LOCAL_POWER);
-				if (hubstatus & HUB_STATUS_LOCAL_POWER)
-					/* FIXME: Is this always true? */
-					hub->limited_power = 1;
-				else
-					hub->limited_power = 0;
-			}
-			if (hubchange & HUB_CHANGE_OVERCURRENT) {
-				u16 status = 0;
-				u16 unused;
-
-				dev_dbg(hub_dev, "over-current change\n");
-				clear_hub_feature(hdev, C_HUB_OVER_CURRENT);
-				msleep(500);	/* Cool down */
-				hub_power_on(hub, true);
-				hub_hub_status(hub, &status, &unused);
-				if (status & HUB_STATUS_OVERCURRENT)
-					dev_err(hub_dev, "over-current "
-						"condition\n");
-			}
+	/* deal with hub status changes */
+	if (test_and_clear_bit(0, hub->event_bits) == 0)
+		;	/* do nothing */
+	else if (hub_hub_status(hub, &hubstatus, &hubchange) < 0)
+		dev_err(hub_dev, "get_hub_status failed\n");
+	else {
+		if (hubchange & HUB_CHANGE_LOCAL_POWER) {
+			dev_dbg(hub_dev, "power change\n");
+			clear_hub_feature(hdev, C_HUB_LOCAL_POWER);
+			if (hubstatus & HUB_STATUS_LOCAL_POWER)
+				/* FIXME: Is this always true? */
+				hub->limited_power = 1;
+			else
+				hub->limited_power = 0;
 		}
+		if (hubchange & HUB_CHANGE_OVERCURRENT) {
+			u16 status = 0;
+			u16 unused;
 
- loop_autopm:
+			dev_dbg(hub_dev, "over-current change\n");
+			clear_hub_feature(hdev, C_HUB_OVER_CURRENT);
+			msleep(500);	/* Cool down */
+			hub_power_on(hub, true);
+			hub_hub_status(hub, &status, &unused);
+			if (status & HUB_STATUS_OVERCURRENT)
+				dev_err(hub_dev, "over-current condition\n");
+		}
+	}
+ out_autopm:
 		/* Balance the usb_autopm_get_interface() above */
 		usb_autopm_put_interface(intf);
- loop:
- loop_disconnected:
+ out:
 		/* Balance the usb_autopm_get_interface_no_resume() in
 		 * kick_khubd() and allow autosuspend.
 		 *
@@ -5121,8 +5114,6 @@ static void hub_event(struct work_struct *work)
 		usb_unlock_device(hdev);
 		usb_put_dev(hdev);
 		kref_put(&hub->kref, hub_release);
-		break;
-	}
 }
 
 static const struct usb_device_id hub_id_table[] = {
-- 
1.8.4

[PATCH 3/4] usb: hub: rename *kick_khubd to *kick_hub_wq

[Petr Mladek]

USB hub started to use a workqueue instead of kthread. Let's make it clear from
the function names.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 drivers/usb/core/hcd.c |  4 ++--
 drivers/usb/core/hub.c | 16 ++++++++--------
 drivers/usb/core/usb.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 487abcfcccd8..4bec044a786c 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2386,7 +2386,7 @@ void usb_hc_died (struct usb_hcd *hcd)
 		/* make khubd clean up old urbs and devices */
 		usb_set_device_state (hcd->self.root_hub,
 				USB_STATE_NOTATTACHED);
-		usb_kick_khubd (hcd->self.root_hub);
+		usb_kick_hub_wq(hcd->self.root_hub);
 	}
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->shared_hcd) {
 		hcd = hcd->shared_hcd;
@@ -2396,7 +2396,7 @@ void usb_hc_died (struct usb_hcd *hcd)
 			/* make khubd clean up old urbs and devices */
 			usb_set_device_state(hcd->self.root_hub,
 					USB_STATE_NOTATTACHED);
-			usb_kick_khubd(hcd->self.root_hub);
+			usb_kick_hub_wq(hcd->self.root_hub);
 		}
 	}
 	spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b31eec65caf7..e5883c8e77c7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -569,7 +569,7 @@ static int hub_port_status(struct usb_hub *hub, int port1,
 	return ret;
 }
 
-static void kick_khubd(struct usb_hub *hub)
+static void kick_hub_wq(struct usb_hub *hub)
 {
 	if (!hub->disconnected && !work_pending(&hub->events)) {
 		/*
@@ -587,12 +587,12 @@ static void kick_khubd(struct usb_hub *hub)
 	}
 }
 
-void usb_kick_khubd(struct usb_device *hdev)
+void usb_kick_hub_wq(struct usb_device *hdev)
 {
 	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
 
 	if (hub)
-		kick_khubd(hub);
+		kick_hub_wq(hub);
 }
 
 /*
@@ -614,7 +614,7 @@ void usb_wakeup_notification(struct usb_device *hdev,
 	hub = usb_hub_to_struct_hub(hdev);
 	if (hub) {
 		set_bit(portnum, hub->wakeup_bits);
-		kick_khubd(hub);
+		kick_hub_wq(hub);
 	}
 }
 EXPORT_SYMBOL_GPL(usb_wakeup_notification);
@@ -654,7 +654,7 @@ static void hub_irq(struct urb *urb)
 	hub->nerrors = 0;
 
 	/* Something happened, let khubd figure it out */
-	kick_khubd(hub);
+	kick_hub_wq(hub);
 
 resubmit:
 	if (hub->quiescing)
@@ -968,7 +968,7 @@ static void hub_port_logical_disconnect(struct usb_hub *hub, int port1)
 	 */
 
 	set_bit(port1, hub->change_bits);
-	kick_khubd(hub);
+	kick_hub_wq(hub);
 }
 
 /**
@@ -1236,7 +1236,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 				&hub->leds, LED_CYCLE_PERIOD);
 
 	/* Scan all ports that need attention */
-	kick_khubd(hub);
+	kick_hub_wq(hub);
 
 	/* Allow autosuspend if it was suppressed */
 	if (type <= HUB_INIT3)
@@ -5103,7 +5103,7 @@ static void hub_event(struct work_struct *work)
 		usb_autopm_put_interface(intf);
  out:
 		/* Balance the usb_autopm_get_interface_no_resume() in
-		 * kick_khubd() and allow autosuspend.
+		 * kick_hub_wq() and allow autosuspend.
 		 *
 		 * It has to be done even when the hub was disconnected
 		 * when waiting for hdev lock. This function is called
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index d9d08720c386..b1b34d0557c9 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -48,7 +48,7 @@ static inline unsigned usb_get_max_power(struct usb_device *udev,
 	return c->desc.bMaxPower * mul;
 }
 
-extern void usb_kick_khubd(struct usb_device *dev);
+extern void usb_kick_hub_wq(struct usb_device *dev);
 extern int usb_match_one_id_intf(struct usb_device *dev,
 				 struct usb_host_interface *intf,
 				 const struct usb_device_id *id);
-- 
1.8.4

[PATCH 4/4] usb: hub: rename khubd to hub_wq in documentation and comments

[Petr Mladek]

USB hub has started to use a workqueue instead of kthread. Let's update
the documentation and comments here and there.

This patch mostly just replaces "khubd" with "hub_wq". There are only few
exceptions where the whole sentence was updated. These more complicated
changes can be found in the following files:

	   Documentation/usb/hotplug.txt
	   drivers/net/usb/usbnet.c
	   drivers/usb/core/hcd.c
	   drivers/usb/host/ohci-hcd.c
	   drivers/usb/host/xhci.c

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 Documentation/DocBook/usb.tmpl             |  2 +-
 Documentation/usb/WUSB-Design-overview.txt |  6 ++--
 Documentation/usb/hotplug.txt              |  2 +-
 drivers/net/usb/usbnet.c                   | 14 ++++++----
 drivers/usb/README                         |  2 +-
 drivers/usb/core/hcd.c                     | 10 +++----
 drivers/usb/core/hub.c                     | 44 +++++++++++++++---------------
 drivers/usb/host/ehci-fsl.c                |  2 +-
 drivers/usb/host/ehci-hcd.c                |  2 +-
 drivers/usb/host/ehci-hub.c                |  8 +++---
 drivers/usb/host/fhci-hcd.c                |  6 ++--
 drivers/usb/host/fotg210-hcd.c             |  8 +++---
 drivers/usb/host/fusbh200-hcd.c            |  8 +++---
 drivers/usb/host/isp1760-hcd.c             |  6 ++--
 drivers/usb/host/ohci-hcd.c                |  6 ++--
 drivers/usb/host/ohci-hub.c                |  4 +--
 drivers/usb/host/ohci-omap.c               |  2 +-
 drivers/usb/host/oxu210hp-hcd.c            | 10 +++----
 drivers/usb/host/sl811-hcd.c               |  8 +++---
 drivers/usb/host/xhci-hub.c                |  2 +-
 drivers/usb/host/xhci.c                    |  4 +--
 drivers/usb/misc/usbtest.c                 |  2 +-
 drivers/usb/musb/am35x.c                   |  1 +
 drivers/usb/musb/tusb6010.c                |  2 +-
 drivers/usb/phy/phy-fsl-usb.c              |  2 +-
 drivers/usb/phy/phy-isp1301-omap.c         |  2 +-
 drivers/usb/wusbcore/devconnect.c          |  6 ++--
 drivers/usb/wusbcore/wa-hc.h               |  2 +-
 sound/usb/midi.c                           |  2 +-
 29 files changed, 89 insertions(+), 86 deletions(-)

diff --git a/Documentation/DocBook/usb.tmpl b/Documentation/DocBook/usb.tmpl
index 85fc0e28576f..4cd5b2cd0f3d 100644
--- a/Documentation/DocBook/usb.tmpl
+++ b/Documentation/DocBook/usb.tmpl
@@ -593,7 +593,7 @@ for (;;) {
 	    Each device has one control endpoint (endpoint zero)
 	    which supports a limited RPC style RPC access.
 	    Devices are configured
-	    by khubd (in the kernel) setting a device-wide
+	    by hub_wq (in the kernel) setting a device-wide
 	    <emphasis>configuration</emphasis> that affects things
 	    like power consumption and basic functionality.
 	    The endpoints are part of USB <emphasis>interfaces</emphasis>,
diff --git a/Documentation/usb/WUSB-Design-overview.txt b/Documentation/usb/WUSB-Design-overview.txt
index 1cd07c017cf6..9d08f179a7ca 100644
--- a/Documentation/usb/WUSB-Design-overview.txt
+++ b/Documentation/usb/WUSB-Design-overview.txt
@@ -317,7 +317,7 @@ HC picks the /DN_Connect/ out (nep module sends to notif.c for delivery
 into /devconnect/). This process starts the authentication process for
 the device. First we allocate a /fake port/ and assign an
 unauthenticated address (128 to 255--what we really do is
-0x80 | fake_port_idx). We fiddle with the fake port status and /khubd/
+0x80 | fake_port_idx). We fiddle with the fake port status and /hub_wq/
 sees a new connection, so he moves on to enable the fake port with a reset.
 
 So now we are in the reset path -- we know we have a non-yet enumerated
@@ -326,7 +326,7 @@ device with an unauthorized address; we ask user space to authenticate
 exchange (FIXME: not yet done) and issue a /set address 0/ to bring the
 device to the default state. Device is authenticated.
 
-From here, the USB stack takes control through the usb_hcd ops. khubd
+From here, the USB stack takes control through the usb_hcd ops. hub_wq
 has seen the port status changes, as we have been toggling them. It will
 start enumerating and doing transfers through usb_hcd->urb_enqueue() to
 read descriptors and move our data.
@@ -340,7 +340,7 @@ Keep Alive IE; it responds with a /DN_Alive/ pong during the DNTS (this
 arrives to us as a notification through
 devconnect.c:wusb_handle_dn_alive(). If a device times out, we
 disconnect it from the system (cleaning up internal information and
-toggling the bits in the fake hub port, which kicks khubd into removing
+toggling the bits in the fake hub port, which kicks hub_wq into removing
 the rest of the stuff).
 
 This is done through devconnect:__wusb_check_devs(), which will scan the
diff --git a/Documentation/usb/hotplug.txt b/Documentation/usb/hotplug.txt
index a80b0e9a7a0b..5b243f315b2c 100644
--- a/Documentation/usb/hotplug.txt
+++ b/Documentation/usb/hotplug.txt
@@ -58,7 +58,7 @@ USB POLICY AGENT
 
 The USB subsystem currently invokes /sbin/hotplug when USB devices
 are added or removed from system.  The invocation is done by the kernel
-hub daemon thread [khubd], or else as part of root hub initialization
+hub workqueue [hub_wq], or else as part of root hub initialization
 (done by init, modprobe, kapmd, etc).  Its single command line parameter
 is the string "usb", and it passes these environment variables:
 
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 5173821a9575..20615bbd693b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -69,8 +69,9 @@
 // reawaken network queue this soon after stopping; else watchdog barks
 #define TX_TIMEOUT_JIFFIES	(5*HZ)
 
-// throttle rx/tx briefly after some faults, so khubd might disconnect()
-// us (it polls at HZ/4 usually) before we report too many false errors.
+/* throttle rx/tx briefly after some faults, so hub_wq might disconnect()
+ * us (it polls at HZ/4 usually) before we report too many false errors.
+ */
 #define THROTTLE_JIFFIES	(HZ/8)
 
 // between wakeups
@@ -595,9 +596,9 @@ static void rx_complete (struct urb *urb)
 			  "rx shutdown, code %d\n", urb_status);
 		goto block;
 
-	/* we get controller i/o faults during khubd disconnect() delays.
+	/* we get controller i/o faults during hub_wq disconnect() delays.
 	 * throttle down resubmits, to avoid log floods; just temporarily,
-	 * so we still recover when the fault isn't a khubd delay.
+	 * so we still recover when the fault isn't a hub_wq delay.
 	 */
 	case -EPROTO:
 	case -ETIME:
@@ -1185,8 +1186,9 @@ static void tx_complete (struct urb *urb)
 		case -ESHUTDOWN:		// hardware gone
 			break;
 
-		// like rx, tx gets controller i/o faults during khubd delays
-		// and so it uses the same throttling mechanism.
+		/* like rx, tx gets controller i/o faults during hub_wq
+		 * delays and so it uses the same throttling mechanism.
+		 */
 		case -EPROTO:
 		case -ETIME:
 		case -EILSEQ:
diff --git a/drivers/usb/README b/drivers/usb/README
index 284f46b3e1cc..2144e7dbfa41 100644
--- a/drivers/usb/README
+++ b/drivers/usb/README
@@ -24,7 +24,7 @@ Here is a list of what each subdirectory here is, and what is contained in
 them.
 
 core/		- This is for the core USB host code, including the
-		  usbfs files and the hub class driver ("khubd").
+		  usbfs files and the hub class driver ("hub_wq").
 
 host/		- This is for USB host controller drivers.  This
 		  includes UHCI, OHCI, EHCI, and others that might
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 4bec044a786c..d2607a973a5c 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2301,7 +2301,7 @@ EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
  * Context: in_interrupt()
  *
  * Starts enumeration, with an immediate reset followed later by
- * khubd identifying and possibly configuring the device.
+ * hub_wq identifying and possibly configuring the device.
  * This is needed by OTG controller drivers, where it helps meet
  * HNP protocol timing requirements for starting a port reset.
  *
@@ -2320,7 +2320,7 @@ int usb_bus_start_enum(struct usb_bus *bus, unsigned port_num)
 	if (port_num && hcd->driver->start_port_reset)
 		status = hcd->driver->start_port_reset(hcd, port_num);
 
-	/* run khubd shortly after (first) root port reset finishes;
+	/* allocate hub_wq shortly after (first) root port reset finishes;
 	 * it may issue others, until at least 50 msecs have passed.
 	 */
 	if (status == 0)
@@ -2383,7 +2383,7 @@ void usb_hc_died (struct usb_hcd *hcd)
 	if (hcd->rh_registered) {
 		clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
 
-		/* make khubd clean up old urbs and devices */
+		/* make hub_wq clean up old urbs and devices */
 		usb_set_device_state (hcd->self.root_hub,
 				USB_STATE_NOTATTACHED);
 		usb_kick_hub_wq(hcd->self.root_hub);
@@ -2393,7 +2393,7 @@ void usb_hc_died (struct usb_hcd *hcd)
 		if (hcd->rh_registered) {
 			clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
 
-			/* make khubd clean up old urbs and devices */
+			/* make hub_wq clean up old urbs and devices */
 			usb_set_device_state(hcd->self.root_hub,
 					USB_STATE_NOTATTACHED);
 			usb_kick_hub_wq(hcd->self.root_hub);
@@ -2655,7 +2655,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
 	/* HC is in reset state, but accessible.  Now do the one-time init,
-	 * bottom up so that hcds can customize the root hubs before khubd
+	 * bottom up so that hcds can customize the root hubs before hub_wq
 	 * starts talking to them.  (Note, bus id is assigned early too.)
 	 */
 	if ((retval = hcd_buffer_create(hcd)) != 0) {
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e5883c8e77c7..3457f471df21 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -641,7 +641,7 @@ static void hub_irq(struct urb *urb)
 		hub->error = status;
 		/* FALL THROUGH */
 
-	/* let khubd handle things */
+	/* let hub_wq handle things */
 	case 0:			/* we got data:  port status changed */
 		bits = 0;
 		for (i = 0; i < urb->actual_length; ++i)
@@ -653,7 +653,7 @@ static void hub_irq(struct urb *urb)
 
 	hub->nerrors = 0;
 
-	/* Something happened, let khubd figure it out */
+	/* Something happened, let hub_wq figure it out */
 	kick_hub_wq(hub);
 
 resubmit:
@@ -684,7 +684,7 @@ hub_clear_tt_buffer (struct usb_device *hdev, u16 devinfo, u16 tt)
 }
 
 /*
- * enumeration blocks khubd for a long time. we use keventd instead, since
+ * enumeration blocks hub_wq for a long time. we use keventd instead, since
  * long blocking there is the exception, not the rule.  accordingly, HCDs
  * talking to TTs must queue control transfers (not just bulk and iso), so
  * both can talk to the same hub concurrently.
@@ -950,7 +950,7 @@ static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
 
 /*
  * Disable a port and mark a logical connect-change event, so that some
- * time later khubd will disconnect() any existing usb_device on the port
+ * time later hub_wq will disconnect() any existing usb_device on the port
  * and will re-enumerate if there actually is a device attached.
  */
 static void hub_port_logical_disconnect(struct usb_hub *hub, int port1)
@@ -963,7 +963,7 @@ static void hub_port_logical_disconnect(struct usb_hub *hub, int port1)
 	 *  - SRP saves power that way
 	 *  - ... new call, TBD ...
 	 * That's easy if this hub can switch power per-port, and
-	 * khubd reactivates the port later (timer, SRP, etc).
+	 * hub_wq reactivates the port later (timer, SRP, etc).
 	 * Powerdown must be optional, because of reset/DFU.
 	 */
 
@@ -976,7 +976,7 @@ static void hub_port_logical_disconnect(struct usb_hub *hub, int port1)
  * @udev: device to be disabled and removed
  * Context: @udev locked, must be able to sleep.
  *
- * After @udev's port has been disabled, khubd is notified and it will
+ * After @udev's port has been disabled, hub_wq is notified and it will
  * see that the device has been disconnected.  When the device is
  * physically unplugged and something is plugged in, the events will
  * be received and processed normally.
@@ -1096,7 +1096,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
  init2:
 
 	/*
-	 * Check each port and set hub->change_bits to let khubd know
+	 * Check each port and set hub->change_bits to let hub_wq know
 	 * which ports need attention.
 	 */
 	for (port1 = 1; port1 <= hdev->maxchild; ++port1) {
@@ -1163,7 +1163,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 			clear_bit(port1, hub->removed_bits);
 
 		if (!udev || udev->state == USB_STATE_NOTATTACHED) {
-			/* Tell khubd to disconnect the device or
+			/* Tell hub_wq to disconnect the device or
 			 * check for a new connection
 			 */
 			if (udev || (portstatus & USB_PORT_STAT_CONNECTION) ||
@@ -1176,7 +1176,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 				USB_SS_PORT_LS_U0;
 			/* The power session apparently survived the resume.
 			 * If there was an overcurrent or suspend change
-			 * (i.e., remote wakeup request), have khubd
+			 * (i.e., remote wakeup request), have hub_wq
 			 * take care of it.  Look at the port link state
 			 * for USB 3.0 hubs, since they don't have a suspend
 			 * change bit, and they don't set the port link change
@@ -1197,7 +1197,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 				set_bit(port1, hub->change_bits);
 
 		} else {
-			/* The power session is gone; tell khubd */
+			/* The power session is gone; tell hub_wq */
 			usb_set_device_state(udev, USB_STATE_NOTATTACHED);
 			set_bit(port1, hub->change_bits);
 		}
@@ -1205,10 +1205,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 
 	/* If no port-status-change flags were set, we don't need any
 	 * debouncing.  If flags were set we can try to debounce the
-	 * ports all at once right now, instead of letting khubd do them
+	 * ports all at once right now, instead of letting hub_wq do them
 	 * one at a time later on.
 	 *
-	 * If any port-status changes do occur during this delay, khubd
+	 * If any port-status changes do occur during this delay, hub_wq
 	 * will see them later and handle them normally.
 	 */
 	if (need_debounce_delay) {
@@ -1269,7 +1269,7 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
 
 	cancel_delayed_work_sync(&hub->init_work);
 
-	/* khubd and related activity won't re-trigger */
+	/* hub_wq and related activity won't re-trigger */
 	hub->quiescing = 1;
 
 	if (type != HUB_SUSPEND) {
@@ -1280,7 +1280,7 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
 		}
 	}
 
-	/* Stop khubd and related activity */
+	/* Stop hub_wq and related activity */
 	usb_kill_urb(hub->urb);
 	if (hub->has_indicators)
 		cancel_delayed_work_sync(&hub->leds);
@@ -1602,7 +1602,7 @@ static int hub_configure(struct usb_hub *hub,
 	if (ret < 0)
 		goto fail;
 
-	/* Update the HCD's internal representation of this hub before khubd
+	/* Update the HCD's internal representation of this hub before hub_wq
 	 * starts getting port status changes for devices under the hub.
 	 */
 	if (hcd->driver->update_hub_device) {
@@ -2028,7 +2028,7 @@ static void choose_devnum(struct usb_device *udev)
 	int		devnum;
 	struct usb_bus	*bus = udev->bus;
 
-	/* If khubd ever becomes multithreaded, this will need a lock */
+	/* If hub_wq ever becomes multithreaded, this will need a lock */
 	if (udev->wusb) {
 		devnum = udev->portnum + 1;
 		BUG_ON(test_bit(devnum, bus->devmap.devicemap));
@@ -3061,7 +3061,7 @@ static unsigned wakeup_enabled_descendants(struct usb_device *udev)
  * Once VBUS drop breaks the circuit, the port it's using has to go through
  * normal re-enumeration procedures, starting with enabling VBUS power.
  * Other than re-initializing the hub (plug/unplug, except for root hubs),
- * Linux (2.6) currently has NO mechanisms to initiate that:  no khubd
+ * Linux (2.6) currently has NO mechanisms to initiate that:  no hub_wq
  * timer, no SRP, no requests through sysfs.
  *
  * If Runtime PM isn't enabled or used, non-SuperSpeed devices may not get
@@ -3203,7 +3203,7 @@ static int finish_port_resume(struct usb_device *udev)
 	/* usb ch9 identifies four variants of SUSPENDED, based on what
 	 * state the device resumes to.  Linux currently won't see the
 	 * first two on the host side; they'd be inside hub_port_init()
-	 * during many timeouts, but khubd can't suspend until later.
+	 * during many timeouts, but hub_wq can't suspend until later.
 	 */
 	usb_set_device_state(udev, udev->actconfig
 			? USB_STATE_CONFIGURED
@@ -3568,7 +3568,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
 
 	dev_dbg(&intf->dev, "%s\n", __func__);
 
-	/* stop khubd and related activity */
+	/* stop hub_wq and related activity */
 	hub_quiesce(hub, HUB_SUSPEND);
 	return 0;
 }
@@ -4961,10 +4961,10 @@ static void port_event(struct usb_hub *hub, int port1)
 	 * On disconnect USB3 protocol ports transit from U0 to
 	 * SS.Inactive to Rx.Detect. If this happens a warm-
 	 * reset is not needed, but a (re)connect may happen
-	 * before khubd runs and sees the disconnect, and the
+	 * before hub_wq runs and sees the disconnect, and the
 	 * device may be an unknown state.
 	 *
-	 * If the port went through SS.Inactive without khubd
+	 * If the port went through SS.Inactive without hub_wq
 	 * seeing it the C_LINK_STATE change flag will be set,
 	 * and we reset the dev to put it in a known state.
 	 */
@@ -5282,7 +5282,7 @@ static int descriptors_changed(struct usb_device *udev,
  * former operating configuration.  If the reset fails, or the device's
  * descriptors change from their values before the reset, or the original
  * configuration and altsettings cannot be restored, a flag will be set
- * telling khubd to pretend the device has been disconnected and then
+ * telling hub_wq to pretend the device has been disconnected and then
  * re-connected.  All drivers will be unbound, and the device will be
  * re-enumerated and probed all over again.
  *
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index cf2734b532a7..3d84b6a41dae 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -627,7 +627,7 @@ static int ehci_start_port_reset(struct usb_hcd *hcd, unsigned port)
 	if (!(status & PORT_CONNECT))
 		return -ENODEV;
 
-	/* khubd will finish the reset later */
+	/* hub_wq will finish the reset later */
 	if (ehci_is_TDI(ehci)) {
 		writel(PORT_RESET |
 		       (status & ~(PORT_CSC | PORT_PEC | PORT_OCC)),
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 81cda09b47e3..0ed9b1d5921f 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -788,7 +788,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
 				continue;
 
 			/* start 20 msec resume signaling from this port,
-			 * and make khubd collect PORT_STAT_C_SUSPEND to
+			 * and make hub_wq collect PORT_STAT_C_SUSPEND to
 			 * stop that signaling.  Use 5 ms extra for safety,
 			 * like usb_port_resume() does.
 			 */
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 6130b7574908..7ccb3ccf8e86 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -656,7 +656,7 @@ ehci_hub_status_data (struct usb_hcd *hcd, char *buf)
 
 		/*
 		 * Return status information even for ports with OWNER set.
-		 * Otherwise khubd wouldn't see the disconnect event when a
+		 * Otherwise hub_wq wouldn't see the disconnect event when a
 		 * high-speed device is switched over to the companion
 		 * controller by the user.
 		 */
@@ -902,7 +902,7 @@ int ehci_hub_control(
 
 		/*
 		 * Even if OWNER is set, so the port is owned by the
-		 * companion controller, khubd needs to be able to clear
+		 * companion controller, hub_wq needs to be able to clear
 		 * the port-change status bits (especially
 		 * USB_PORT_STAT_C_CONNECTION).
 		 */
@@ -1000,7 +1000,7 @@ int ehci_hub_control(
 			 * However, not all EHCI implementations do this
 			 * automatically, even if they _do_ support per-port
 			 * power switching; they're allowed to just limit the
-			 * current.  khubd will turn the power back on.
+			 * current.  hub_wq will turn the power back on.
 			 */
 			if (((temp & PORT_OC) || (ehci->need_oc_pp_cycle))
 					&& HCS_PPC(ehci->hcs_params)) {
@@ -1085,7 +1085,7 @@ int ehci_hub_control(
 		}
 
 		/*
-		 * Even if OWNER is set, there's no harm letting khubd
+		 * Even if OWNER is set, there's no harm letting hub_wq
 		 * see the wPortStatus values (they should all be 0 except
 		 * for PORT_POWER anyway).
 		 */
diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
index 1cf68eaf2ed8..a1a1ef521436 100644
--- a/drivers/usb/host/fhci-hcd.c
+++ b/drivers/usb/host/fhci-hcd.c
@@ -360,12 +360,12 @@ static int fhci_start(struct usb_hcd *hcd)
 	hcd->state = HC_STATE_RUNNING;
 
 	/*
-	 * From here on, khubd concurrently accesses the root
+	 * From here on, hub_wq concurrently accesses the root
 	 * hub; drivers will be talking to enumerated devices.
-	 * (On restart paths, khubd already knows about the root
+	 * (On restart paths, hub_wq already knows about the root
 	 * hub and could find work as soon as we wrote FLAG_CF.)
 	 *
-	 * Before this point the HC was idle/ready.  After, khubd
+	 * Before this point the HC was idle/ready.  After, hub_wq
 	 * and device drivers may start it running.
 	 */
 	fhci_usb_enable(fhci);
diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
index adcd2050dced..3de1278677d0 100644
--- a/drivers/usb/host/fotg210-hcd.c
+++ b/drivers/usb/host/fotg210-hcd.c
@@ -1483,7 +1483,7 @@ fotg210_hub_status_data(struct usb_hcd *hcd, char *buf)
 
 	/*
 	 * Return status information even for ports with OWNER set.
-	 * Otherwise khubd wouldn't see the disconnect event when a
+	 * Otherwise hub_wq wouldn't see the disconnect event when a
 	 * high-speed device is switched over to the companion
 	 * controller by the user.
 	 */
@@ -1572,7 +1572,7 @@ static int fotg210_hub_control(
 
 		/*
 		 * Even if OWNER is set, so the port is owned by the
-		 * companion controller, khubd needs to be able to clear
+		 * companion controller, hub_wq needs to be able to clear
 		 * the port-change status bits (especially
 		 * USB_PORT_STAT_C_CONNECTION).
 		 */
@@ -1723,7 +1723,7 @@ static int fotg210_hub_control(
 		}
 
 		/*
-		 * Even if OWNER is set, there's no harm letting khubd
+		 * Even if OWNER is set, there's no harm letting hub_wq
 		 * see the wPortStatus values (they should all be 0 except
 		 * for PORT_POWER anyway).
 		 */
@@ -5445,7 +5445,7 @@ static irqreturn_t fotg210_irq(struct usb_hcd *hcd)
 				fotg210->reset_done[0] == 0) {
 
 			/* start 20 msec resume signaling from this port,
-			 * and make khubd collect PORT_STAT_C_SUSPEND to
+			 * and make hub_wq collect PORT_STAT_C_SUSPEND to
 			 * stop that signaling.  Use 5 ms extra for safety,
 			 * like usb_port_resume() does.
 			 */
diff --git a/drivers/usb/host/fusbh200-hcd.c b/drivers/usb/host/fusbh200-hcd.c
index ba9499060f63..abe42f31559f 100644
--- a/drivers/usb/host/fusbh200-hcd.c
+++ b/drivers/usb/host/fusbh200-hcd.c
@@ -1441,7 +1441,7 @@ fusbh200_hub_status_data (struct usb_hcd *hcd, char *buf)
 
 	/*
 	 * Return status information even for ports with OWNER set.
-	 * Otherwise khubd wouldn't see the disconnect event when a
+	 * Otherwise hub_wq wouldn't see the disconnect event when a
 	 * high-speed device is switched over to the companion
 	 * controller by the user.
 	 */
@@ -1530,7 +1530,7 @@ static int fusbh200_hub_control (
 
 		/*
 		 * Even if OWNER is set, so the port is owned by the
-		 * companion controller, khubd needs to be able to clear
+		 * companion controller, hub_wq needs to be able to clear
 		 * the port-change status bits (especially
 		 * USB_PORT_STAT_C_CONNECTION).
 		 */
@@ -1678,7 +1678,7 @@ static int fusbh200_hub_control (
 		}
 
 		/*
-		 * Even if OWNER is set, there's no harm letting khubd
+		 * Even if OWNER is set, there's no harm letting hub_wq
 		 * see the wPortStatus values (they should all be 0 except
 		 * for PORT_POWER anyway).
 		 */
@@ -5355,7 +5355,7 @@ static irqreturn_t fusbh200_irq (struct usb_hcd *hcd)
 				fusbh200->reset_done[0] == 0) {
 
 			/* start 20 msec resume signaling from this port,
-			 * and make khubd collect PORT_STAT_C_SUSPEND to
+			 * and make hub_wq collect PORT_STAT_C_SUSPEND to
 			 * stop that signaling.  Use 5 ms extra for safety,
 			 * like usb_port_resume() does.
 			 */
diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index 51a0ae9cdd1d..e752c3098f38 100644
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -1760,7 +1760,7 @@ static int isp1760_hub_status_data(struct usb_hcd *hcd, char *buf)
 
 	/*
 	 * Return status information even for ports with OWNER set.
-	 * Otherwise khubd wouldn't see the disconnect event when a
+	 * Otherwise hub_wq wouldn't see the disconnect event when a
 	 * high-speed device is switched over to the companion
 	 * controller by the user.
 	 */
@@ -1871,7 +1871,7 @@ static int isp1760_hub_control(struct usb_hcd *hcd, u16 typeReq,
 
 		/*
 		 * Even if OWNER is set, so the port is owned by the
-		 * companion controller, khubd needs to be able to clear
+		 * companion controller, hub_wq needs to be able to clear
 		 * the port-change status bits (especially
 		 * USB_PORT_STAT_C_CONNECTION).
 		 */
@@ -2000,7 +2000,7 @@ static int isp1760_hub_control(struct usb_hcd *hcd, u16 typeReq,
 					reg_read32(hcd->regs, HC_PORTSC1));
 		}
 		/*
-		 * Even if OWNER is set, there's no harm letting khubd
+		 * Even if OWNER is set, there's no harm letting hub_wq
 		 * see the wPortStatus values (they should all be 0 except
 		 * for PORT_POWER anyway).
 		 */
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 46987735a2e3..d664edabf14e 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -632,7 +632,7 @@ retry:
 		return -EOVERFLOW;
 	}
 
-	/* use rhsc irqs after khubd is fully initialized */
+	/* use rhsc irqs after hub_wq is allocated */
 	set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
 	hcd->uses_new_polling = 1;
 
@@ -909,8 +909,8 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
 		 * choices for RHSC.  Many followed the spec; RHSC triggers
 		 * on an edge, like setting and maybe clearing a port status
 		 * change bit.  With others it's level-triggered, active
-		 * until khubd clears all the port status change bits.  We'll
-		 * always disable it here and rely on polling until khubd
+		 * until hub_wq clears all the port status change bits.  We'll
+		 * always disable it here and rely on polling until hub_wq
 		 * re-enables it.
 		 */
 		ohci_writel(ohci, OHCI_INTR_RHSC, &regs->intrdisable);
diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index 17d32b0ea565..0aa17c937115 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -585,7 +585,7 @@ static int ohci_start_port_reset (struct usb_hcd *hcd, unsigned port)
 	if (!(status & RH_PS_CCS))
 		return -ENODEV;
 
-	/* khubd will finish the reset later */
+	/* hub_wq will finish the reset later */
 	ohci_writel(ohci, RH_PS_PRS, &ohci->regs->roothub.portstatus [port]);
 	return 0;
 }
@@ -610,7 +610,7 @@ static int ohci_start_port_reset (struct usb_hcd *hcd, unsigned port)
 /* wrap-aware logic morphed from <linux/jiffies.h> */
 #define tick_before(t1,t2) ((s16)(((s16)(t1))-((s16)(t2))) < 0)
 
-/* called from some task, normally khubd */
+/* called from some task, normally hub_wq */
 static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 {
 	__hc32 __iomem *portstat = &ohci->regs->roothub.portstatus [port];
diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
index c923cafcaca7..de9428362503 100644
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -283,7 +283,7 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
 		ohci_to_hcd(ohci)->power_budget = 0;
 	}
 
-	/* FIXME khubd hub requests should manage power switching */
+	/* FIXME hub_wq hub requests should manage power switching */
 	omap_ohci_transceiver_power(1);
 
 	/* board init will have already handled HMC and mux setup.
diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
index da5fb0e3c363..4fe79a2d71a9 100644
--- a/drivers/usb/host/oxu210hp-hcd.c
+++ b/drivers/usb/host/oxu210hp-hcd.c
@@ -2046,7 +2046,7 @@ static void intr_deschedule(struct oxu_hcd *oxu, struct ehci_qh *qh)
 
 	/* simple/paranoid:  always delay, expecting the HC needs to read
 	 * qh->hw_next or finish a writeback after SPLIT/CSPLIT ... and
-	 * expect khubd to clean up after any CSPLITs we won't issue.
+	 * expect hub_wq to clean up after any CSPLITs we won't issue.
 	 * active high speed queues may need bigger delays...
 	 */
 	if (list_empty(&qh->qtd_list)
@@ -2501,7 +2501,7 @@ static irqreturn_t oxu210_hcd_irq(struct usb_hcd *hcd)
 				continue;
 
 			/* start 20 msec resume signaling from this port,
-			 * and make khubd collect PORT_STAT_C_SUSPEND to
+			 * and make hub_wq collect PORT_STAT_C_SUSPEND to
 			 * stop that signaling.
 			 */
 			oxu->reset_done[i] = jiffies + msecs_to_jiffies(20);
@@ -3119,7 +3119,7 @@ static int oxu_hub_status_data(struct usb_hcd *hcd, char *buf)
 
 		/*
 		 * Return status information even for ports with OWNER set.
-		 * Otherwise khubd wouldn't see the disconnect event when a
+		 * Otherwise hub_wq wouldn't see the disconnect event when a
 		 * high-speed device is switched over to the companion
 		 * controller by the user.
 		 */
@@ -3194,7 +3194,7 @@ static int oxu_hub_control(struct usb_hcd *hcd, u16 typeReq,
 
 		/*
 		 * Even if OWNER is set, so the port is owned by the
-		 * companion controller, khubd needs to be able to clear
+		 * companion controller, hub_wq needs to be able to clear
 		 * the port-change status bits (especially
 		 * USB_PORT_STAT_C_CONNECTION).
 		 */
@@ -3336,7 +3336,7 @@ static int oxu_hub_control(struct usb_hcd *hcd, u16 typeReq,
 		}
 
 		/*
-		 * Even if OWNER is set, there's no harm letting khubd
+		 * Even if OWNER is set, there's no harm letting hub_wq
 		 * see the wPortStatus values (they should all be 0 except
 		 * for PORT_POWER anyway).
 		 */
diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index a517151867af..ad0c348e68e9 100644
--- a/drivers/usb/host/sl811-hcd.c
+++ b/drivers/usb/host/sl811-hcd.c
@@ -674,7 +674,7 @@ retry:
 			sl811->next_periodic = sl811->periodic[index];
 	}
 
-	/* khubd manages debouncing and wakeup */
+	/* hub_wq manages debouncing and wakeup */
 	if (irqstat & SL11H_INTMASK_INSRMV) {
 		sl811->stat_insrmv++;
 
@@ -714,7 +714,7 @@ retry:
 #endif
 
 		/* port status seems weird until after reset, so
-		 * force the reset and make khubd clean up later.
+		 * force the reset and make hub_wq clean up later.
 		 */
 		if (irqstat & SL11H_INTMASK_RD)
 			sl811->port1 &= ~USB_PORT_STAT_CONNECTION;
@@ -1079,7 +1079,7 @@ sl811h_hub_status_data(struct usb_hcd *hcd, char *buf)
 	if (!(sl811->port1 & (0xffff << 16)))
 		return 0;
 
-	/* tell khubd port 1 changed */
+	/* tell hub_wq port 1 changed */
 	*buf = (1 << 1);
 	return 1;
 }
@@ -1196,7 +1196,7 @@ sl811h_timer(unsigned long _sl811)
 		sl811_write(sl811, SL811_EP_A(SL11H_HOSTCTLREG),
 				SL11H_HCTLMASK_ARM);
 
-		/* khubd provides debounce delay */
+		/* hub_wq provides debounce delay */
 	} else {
 		sl811->ctrl1 = 0;
 	}
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 69aece31143a..0cf49ffa902e 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -892,7 +892,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			/*
 			 * Turn on ports, even if there isn't per-port switching.
 			 * HC will report connect events even before this is set.
-			 * However, khubd will ignore the roothub events until
+			 * However, hub_wq will ignore the roothub events until
 			 * the roothub is registered.
 			 */
 			writel(temp | PORT_POWER, port_array[wIndex]);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c020b094fe7d..4b0dbafad28f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3761,8 +3761,8 @@ disable_slot:
 /*
  * Issue an Address Device command and optionally send a corresponding
  * SetAddress request to the device.
- * We should be protected by the usb_address0_mutex in khubd's hub_port_init, so
- * we should only issue and wait on one address command at the same time.
+ * We should be protected by the usb_address0_mutex in hub_wq's hub_port_init,
+ * so we should only issue and wait on one address command at the same time.
  */
 static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 			     enum xhci_setup_dev setup)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 90e6644dc886..0bbafe795a72 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -2031,7 +2031,7 @@ static int test_unaligned_bulk(
  *
  * WARNING:  Because usbfs grabs udev->dev.sem before calling this ioctl(),
  * it locks out usbcore in certain code paths.  Notably, if you disconnect
- * the device-under-test, khubd will wait block forever waiting for the
+ * the device-under-test, hub_wq will wait block forever waiting for the
  * ioctl to complete ... so that usb_disconnect() can abort the pending
  * urbs and then call usbtest_disconnect().  To abort a test, you're best
  * off just killing the userspace task and waiting for it to exit.
diff --git a/drivers/usb/musb/am35x.c b/drivers/usb/musb/am35x.c
index 0a34dd859555..a2735df24cc6 100644
--- a/drivers/usb/musb/am35x.c
+++ b/drivers/usb/musb/am35x.c
@@ -1,3 +1,4 @@
+
 /*
  * Texas Instruments AM35x "glue layer"
  *
diff --git a/drivers/usb/musb/tusb6010.c b/drivers/usb/musb/tusb6010.c
index 7dfc6cb732c9..2daa779f1382 100644
--- a/drivers/usb/musb/tusb6010.c
+++ b/drivers/usb/musb/tusb6010.c
@@ -433,7 +433,7 @@ static void musb_do_idle(unsigned long _musb)
 	if (!musb->is_active) {
 		u32	wakeups;
 
-		/* wait until khubd handles port change status */
+		/* wait until hub_wq handles port change status */
 		if (is_host_active(musb) && (musb->port1_status >> 16))
 			goto done;
 
diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 2b0f968d9325..f1ea5990a50a 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -609,7 +609,7 @@ static int fsl_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
 		otg->host->otg_port = fsl_otg_initdata.otg_port;
 		otg->host->is_b_host = otg_dev->fsm.id;
 		/*
-		 * must leave time for khubd to finish its thing
+		 * must leave time for hub_wq to finish its thing
 		 * before yanking the host driver out from under it,
 		 * so suspend the host after a short delay.
 		 */
diff --git a/drivers/usb/phy/phy-isp1301-omap.c b/drivers/usb/phy/phy-isp1301-omap.c
index 69e49be8866b..8eea56d3ded6 100644
--- a/drivers/usb/phy/phy-isp1301-omap.c
+++ b/drivers/usb/phy/phy-isp1301-omap.c
@@ -1011,7 +1011,7 @@ static void isp_update_otg(struct isp1301 *isp, u8 stat)
 				break;
 			case OTG_STATE_A_WAIT_VFALL:
 				state = OTG_STATE_A_IDLE;
-				/* khubd may take a while to notice and
+				/* hub_wq may take a while to notice and
 				 * handle this disconnect, so don't go
 				 * to B_IDLE quite yet.
 				 */
diff --git a/drivers/usb/wusbcore/devconnect.c b/drivers/usb/wusbcore/devconnect.c
index 0677139c6065..3f4f5fbded55 100644
--- a/drivers/usb/wusbcore/devconnect.c
+++ b/drivers/usb/wusbcore/devconnect.c
@@ -329,7 +329,7 @@ void wusbhc_devconnect_ack(struct wusbhc *wusbhc, struct wusb_dn_connect *dnc,
 	port->wusb_dev = wusb_dev;
 	port->status |= USB_PORT_STAT_CONNECTION;
 	port->change |= USB_PORT_STAT_C_CONNECTION;
-	/* Now the port status changed to connected; khubd will
+	/* Now the port status changed to connected; hub_wq will
 	 * pick the change up and try to reset the port to bring it to
 	 * the enabled state--so this process returns up to the stack
 	 * and it calls back into wusbhc_rh_port_reset().
@@ -343,7 +343,7 @@ error_unlock:
 /*
  * Disconnect a Wireless USB device from its fake port
  *
- * Marks the port as disconnected so that khubd can pick up the change
+ * Marks the port as disconnected so that hub_wq can pick up the change
  * and drops our knowledge about the device.
  *
  * Assumes there is a device connected
@@ -379,7 +379,7 @@ static void __wusbhc_dev_disconnect(struct wusbhc *wusbhc,
 		wusbhc_gtk_rekey(wusbhc);
 
 	/* The Wireless USB part has forgotten about the device already; now
-	 * khubd's timer will pick up the disconnection and remove the USB
+	 * hub_wq's timer will pick up the disconnection and remove the USB
 	 * device from the system
 	 */
 }
diff --git a/drivers/usb/wusbcore/wa-hc.h b/drivers/usb/wusbcore/wa-hc.h
index f2a8d29e17b9..edc7267157f3 100644
--- a/drivers/usb/wusbcore/wa-hc.h
+++ b/drivers/usb/wusbcore/wa-hc.h
@@ -64,7 +64,7 @@
  *
  * Note much of the activity is difficult to follow. For example a
  * device connect goes to devconnect, which will cause the "fake" root
- * hub port to show a connect and stop there. Then khubd will notice
+ * hub port to show a connect and stop there. Then hub_wq will notice
  * and call into the rh.c:hwahc_rc_port_reset() code to authenticate
  * the device (and this might require user intervention) and enable
  * the port.
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index 69e93a9d486a..d3d49525a16b 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -64,7 +64,7 @@
 /* #define DUMP_PACKETS */
 
 /*
- * how long to wait after some USB errors, so that khubd can disconnect() us
+ * how long to wait after some USB errors, so that hub_wq can disconnect() us
  * without too many spurious errors
  */
 #define ERROR_DELAY_JIFFIES (HZ / 10)
-- 
1.8.4

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Alan Stern]

On Fri, 12 Sep 2014, Petr Mladek wrote:

> There is no need to have separate kthread for handling USB hub events.
> It is more elegant to use the workqueue framework.
> 
> The workqueue is allocated as unbound, cpu intensive, and freezable.
> There does not seem to be any big advantage to run it on the same CPU.
> The handler is taking a lock and thus could block for a longer time.
> And finally, the original thread was freezable as well.
> 
> struct usb_hub is passed via the work item. Therefore we do not need
> hub_event_list.
> 
> hub_events() is modified to process the given work item. It is renamed to
> hub_event(). The while cycle will be removed in a followup patch. It helps
> to see the real change here.
> 
> One nice thing is that we do not need hub_event_lock any longer. It was needed
> when doing operations with hub_event_list and for balancing the calls
> usb_autopm_get_interface_no_resume() and usb_autopm_put_interface_no_suspend().
> It still works because the workqueue operations have their own locking.
> Also cancel_work_sync() tells us whether any work item was canceled.
> It means that we could put the interface either in hub_event() handler or when
> the work item was successfully canceled.

I don't think you can eliminate the lock quite so easily.  This patch 
introduces some nasty races.

> @@ -577,18 +571,20 @@ static int hub_port_status(struct usb_hub *hub, int port1,
>  
>  static void kick_khubd(struct usb_hub *hub)
>  {
> -	unsigned long	flags;
> -
> -	spin_lock_irqsave(&hub_event_lock, flags);
> -	if (!hub->disconnected && list_empty(&hub->event_list)) {
> -		list_add_tail(&hub->event_list, &hub_event_list);
> -
> -		/* Suppress autosuspend until khubd runs */
> +	if (!hub->disconnected && !work_pending(&hub->events)) {

Here you test hub->disconnected, with no lock for protection.

(Also, note that work_pending is not synchronized with anything.  What 
happens if two threads call this routine at the same time?)

> @@ -1647,13 +1643,9 @@ static void hub_disconnect(struct usb_interface *intf)
>  	int port1;
>  
>  	/* Take the hub off the event list and don't let it be added again */
> -	spin_lock_irq(&hub_event_lock);
> -	if (!list_empty(&hub->event_list)) {
> -		list_del_init(&hub->event_list);
> +	if (cancel_work_sync(&hub->events))
>  		usb_autopm_put_interface_no_suspend(intf);
> -	}
>  	hub->disconnected = 1;

And here you set hub->disconnected with no lock for protection.  So 
what happens if one thread calls kick_khubd at the same time as another 
thread calls hub_disconnect?

Alan Stern

Re: [PATCH 2/4] usb: hub: remove obsolete while cycle in hub_event()

[Alan Stern]

On Fri, 12 Sep 2014, Petr Mladek wrote:

> The USB hub events are proceed by workqueue instead of kthread now.
> The result is that hub_event() function processes only one event.
> The while cycle was not removed earlier to show the real changes when
> switching to the workqueue.
> 
> This patch also consolidates the goto targets and rename them from "loop*"
> to "out*".
> 
> When touching the code, it fixes also formatting of dev_err() and dev_dbg()
> calls to make checkpatch.pl happy :-)

Although the reason given in the description above is not really
accurate, removing the loop in hub_events is a reasonable thing to do.  
That loop doesn't serve any purpose as far as I can see.

Alan Stern

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Petr Mladek]

On Fri 2014-09-12 10:16:21, Alan Stern wrote:
> On Fri, 12 Sep 2014, Petr Mladek wrote:
> 
> > There is no need to have separate kthread for handling USB hub events.
> > It is more elegant to use the workqueue framework.
> > 
> > The workqueue is allocated as unbound, cpu intensive, and freezable.
> > There does not seem to be any big advantage to run it on the same CPU.
> > The handler is taking a lock and thus could block for a longer time.
> > And finally, the original thread was freezable as well.
> > 
> > struct usb_hub is passed via the work item. Therefore we do not need
> > hub_event_list.
> > 
> > hub_events() is modified to process the given work item. It is renamed to
> > hub_event(). The while cycle will be removed in a followup patch. It helps
> > to see the real change here.
> > 
> > One nice thing is that we do not need hub_event_lock any longer. It was needed
> > when doing operations with hub_event_list and for balancing the calls
> > usb_autopm_get_interface_no_resume() and usb_autopm_put_interface_no_suspend().
> > It still works because the workqueue operations have their own locking.
> > Also cancel_work_sync() tells us whether any work item was canceled.
> > It means that we could put the interface either in hub_event() handler or when
> > the work item was successfully canceled.
> 
> I don't think you can eliminate the lock quite so easily.  This patch 
> introduces some nasty races.
> 
> > @@ -577,18 +571,20 @@ static int hub_port_status(struct usb_hub *hub, int port1,
> >  
> >  static void kick_khubd(struct usb_hub *hub)
> >  {
> > -	unsigned long	flags;
> > -
> > -	spin_lock_irqsave(&hub_event_lock, flags);
> > -	if (!hub->disconnected && list_empty(&hub->event_list)) {
> > -		list_add_tail(&hub->event_list, &hub_event_list);
> > -
> > -		/* Suppress autosuspend until khubd runs */
> > +	if (!hub->disconnected && !work_pending(&hub->events)) {
> 
> Here you test hub->disconnected, with no lock for protection.

This should not be that big problem. It will schedule hub_event() but
it will do basically nothing. This is why I thought that the lock was
not needed.


> (Also, note that work_pending is not synchronized with anything.  What 
> happens if two threads call this routine at the same time?)

You are right! This is a real problem because it might call
usb_autopm_put_interface_no_suspend() twice but it might schedule
hub_event() and call usb_autopm_put_interface() only once.

Well, it might be possible to check the return value of
queue_work and do something like:

	if (!hub->disconnected && !work_pending(&hub->events)) {
		usb_autopm_get_interface_no_resume(
			to_usb_interface(hub->intfdev));
		if (!queue_work(hub_wq, &hub->events))
			usb_autopm_put_interface_no_suspend(intf);
	}

But there is still problem that we need to call
"INIT_WORK(&hub->events, hub_event)" somewhere and do it only once
before calling kick_hub_wq(). I wonder if it might be safe to do
so in hub_activate().

Hmm, I am not longer that optimistic about it. After all, it might
be better to put the lock back. Would you prefer it, please?


> > @@ -1647,13 +1643,9 @@ static void hub_disconnect(struct usb_interface *intf)
> >  	int port1;
> >  
> >  	/* Take the hub off the event list and don't let it be added again */
> > -	spin_lock_irq(&hub_event_lock);
> > -	if (!list_empty(&hub->event_list)) {
> > -		list_del_init(&hub->event_list);
> > +	if (cancel_work_sync(&hub->events))
> >  		usb_autopm_put_interface_no_suspend(intf);
> > -	}
> >  	hub->disconnected = 1;
> 
> And here you set hub->disconnected with no lock for protection.  So 
> what happens if one thread calls kick_khubd at the same time as another 
> thread calls hub_disconnect?

This should not be that big problem as explained above. Note that
hub->disconnected was tested in hub_events() without the lock
even before this patch. Hence I thought that the new code was as racy
as before.


Best Regards,
Petr

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Alan Stern]

On Fri, 12 Sep 2014, Petr Mladek wrote:

> > I don't think you can eliminate the lock quite so easily.  This patch 
> > introduces some nasty races.
> > 
> > > @@ -577,18 +571,20 @@ static int hub_port_status(struct usb_hub *hub, int port1,
> > >  
> > >  static void kick_khubd(struct usb_hub *hub)
> > >  {
> > > -	unsigned long	flags;
> > > -
> > > -	spin_lock_irqsave(&hub_event_lock, flags);
> > > -	if (!hub->disconnected && list_empty(&hub->event_list)) {
> > > -		list_add_tail(&hub->event_list, &hub_event_list);
> > > -
> > > -		/* Suppress autosuspend until khubd runs */
> > > +	if (!hub->disconnected && !work_pending(&hub->events)) {
> > 
> > Here you test hub->disconnected, with no lock for protection.
> 
> This should not be that big problem. It will schedule hub_event() but
> it will do basically nothing. This is why I thought that the lock was
> not needed.

What do you mean "basically nothing"?  hub_event will be scheduled, via 
a work_struct that is embedded in the usb_hub structure.  But that 
structure will be deallocated by hub_disconnect, so you will create a 
"use after free" bug.

> > (Also, note that work_pending is not synchronized with anything.  What 
> > happens if two threads call this routine at the same time?)
> 
> You are right! This is a real problem because it might call
> usb_autopm_put_interface_no_suspend() twice but it might schedule
> hub_event() and call usb_autopm_put_interface() only once.
> 
> Well, it might be possible to check the return value of
> queue_work and do something like:
> 
> 	if (!hub->disconnected && !work_pending(&hub->events)) {
> 		usb_autopm_get_interface_no_resume(
> 			to_usb_interface(hub->intfdev));
> 		if (!queue_work(hub_wq, &hub->events))
> 			usb_autopm_put_interface_no_suspend(intf);
> 	}
> 
> But there is still problem that we need to call
> "INIT_WORK(&hub->events, hub_event)" somewhere and do it only once
> before calling kick_hub_wq(). I wonder if it might be safe to do
> so in hub_activate().

If I thought this was the right way to go, I would suggest initializing 
hub->events in hub_probe, where the structure is created.

> Hmm, I am not longer that optimistic about it. After all, it might
> be better to put the lock back. Would you prefer it, please?

Here's what I think.  If you want to make khubd into a work queue 
thread, you can.  But it should be invoked only once, and the routine 
it runs should be hub_thread, not hub_events.  Overall I don't see any 
advantage in making this change.

> > And here you set hub->disconnected with no lock for protection.  So 
> > what happens if one thread calls kick_khubd at the same time as another 
> > thread calls hub_disconnect?
> 
> This should not be that big problem as explained above. Note that
> hub->disconnected was tested in hub_events() without the lock
> even before this patch. Hence I thought that the new code was as racy
> as before.

But you ignored what the comment says about "don't let it be added 
again".

Alan Stern

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Tejun Heo]

On Fri, Sep 12, 2014 at 02:21:05PM +0200, Petr Mladek wrote:
> There is no need to have separate kthread for handling USB hub events.
> It is more elegant to use the workqueue framework.
> 
> The workqueue is allocated as unbound, cpu intensive, and freezable.

I'd just go with WQ_FREEZABLE.  As a general rule of thumb, please
only use attributes which are strictly necessary.  If the default
behavior turns out to be not so good for typical wq usages (and AFAICS
khubd seems fairly typical), we should be changing the default
behavior.  Each usage deviating complicates the long-term prospect.

Thanks.

-- 
tejun

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Petr Mládek]

On Fri 12-09-14 11:44:14, Alan Stern wrote:
> On Fri, 12 Sep 2014, Petr Mladek wrote:
> 
> > > I don't think you can eliminate the lock quite so easily.  This patch 
> > > introduces some nasty races.
> > > 
> > > > @@ -577,18 +571,20 @@ static int hub_port_status(struct usb_hub *hub, int port1,
> > > >  
> > > >  static void kick_khubd(struct usb_hub *hub)
> > > >  {
> > > > -	unsigned long	flags;
> > > > -
> > > > -	spin_lock_irqsave(&hub_event_lock, flags);
> > > > -	if (!hub->disconnected && list_empty(&hub->event_list)) {
> > > > -		list_add_tail(&hub->event_list, &hub_event_list);
> > > > -
> > > > -		/* Suppress autosuspend until khubd runs */
> > > > +	if (!hub->disconnected && !work_pending(&hub->events)) {
> > > 
> > > Here you test hub->disconnected, with no lock for protection.
> > 
> > This should not be that big problem. It will schedule hub_event() but
> > it will do basically nothing. This is why I thought that the lock was
> > not needed.
> 
> What do you mean "basically nothing"?  hub_event will be scheduled, via 
> a work_struct that is embedded in the usb_hub structure.  But that 
> structure will be deallocated by hub_disconnect, so you will create a 
> "use after free" bug.

Sigh, I feel like an idiot. The code is not easy but still. I hope
that it was just a temporary parental dementia that I got when taking
care of our new born twins.

Feel free to ask me to send another version of the patch set if you
are getting lost in this Xth reply.


Anyway, the solution for the race between kick_hub_wq() and
hub_event() might be to get the reference already in kick_hub_wq().
I mean something like:

static void kick_hub_wq(struct usb_hub *hub)
{
	if (hub->disconnected || work_pending(&hub->events))
		return;
	/*
	 * Suppress autosuspend until the event is proceed.
	 *
	 * Be careful and make sure that the symmetric operation is
	 * always called. We are here only when there is no pending
	 * work for this hub. Therefore put the interface either when
	 * the new work is called or when it is canceled.
	 */
	usb_autopm_get_interface_no_resume(to_usb_interface(hub->intfdev));
	kref_get(&hub->kref);

	if (queue_work(hub_wq, &hub->events))
		return;

	/* the work could not be scheduled twice */
	kref_put(&hub->kref, hub_release);
	usb_autopm_put_interface_no_suspend(intf);
}

And test for hub->disconnected at the very beginning of hub_event().
Also we would need to put the reference when the work is canceled in
hub_disconnect().

Hmm, I am not 100% sure if we could call
usb_autopm_get_interface_no_resume() without any lock here.
I guess so because otherwise the original code would not work.
The check for "hub->disconnected" would fail if the struct was freed
before the lock was taken.

It looks promissing. hub->intfdev is released together with "hub" in
hub_release(). Also it seems that kick_* and *disconnect functions
are called with some top level lock. For example, there is used dev
lock in drivers/usb/core/device.c.

> > > (Also, note that work_pending is not synchronized with anything.  What 
> > > happens if two threads call this routine at the same time?)
> > 
> > You are right! This is a real problem because it might call
> > usb_autopm_put_interface_no_suspend() twice but it might schedule
> > hub_event() and call usb_autopm_put_interface() only once.
> > 
> > Well, it might be possible to check the return value of
> > queue_work and do something like:
> > 
> > 	if (!hub->disconnected && !work_pending(&hub->events)) {
> > 		usb_autopm_get_interface_no_resume(
> > 			to_usb_interface(hub->intfdev));
> > 		if (!queue_work(hub_wq, &hub->events))
> > 			usb_autopm_put_interface_no_suspend(intf);
> > 	}
> > 
> > But there is still problem that we need to call
> > "INIT_WORK(&hub->events, hub_event)" somewhere and do it only once
> > before calling kick_hub_wq(). I wonder if it might be safe to do
> > so in hub_activate().
>
> If I thought this was the right way to go, I would suggest initializing 
> hub->events in hub_probe, where the structure is created.

Thanks for hint.

> > Hmm, I am not longer that optimistic about it. After all, it might
> > be better to put the lock back. Would you prefer it, please?
> 
> Here's what I think.  If you want to make khubd into a work queue 
> thread, you can.  But it should be invoked only once,

sure

> and the routine it runs should be hub_thread, not hub_events.

Yes, but most of the hub_thread() content is handled by the
workqueue() framework. In fact, after I removed all the obsolete stuff
then only "hub_events() call was left. So, there was no reason to keep it.


> Overall I don't see any advantage in making this change.

There are several motivations:

First, workqueues have clean and rich API for all basic operations.
The code is usually easier and better readable. For example, see how we
reduced hub_thread() and avoided operations with hub_event_list.

Also it should be more safe because the workqueue code has matured
and all the internal locking is well tested.

You might think that there is less control over the job but workqueues
are specialized on these scheduled tasks. There are many options, so
you could easily switch to a better mode if needed. All this is
standardized and thus should be better tested than any local "hacks".

In many cases, it allows to avoid extra kernel thread. It helps to stop the
growing number of them.  Also there will be less thread-specific
hacks all over the kernel code.

It forces making the task selfcontained. There is no longer an unclear
infinite loop. This helps to avoid problems with suspend. Also it will
be very helpful for kGraft (kernel live patching).

I guess that Tejun could come with more advantages.
 
> > > And here you set hub->disconnected with no lock for protection.  So 
> > > what happens if one thread calls kick_khubd at the same time as another 
> > > thread calls hub_disconnect?
> > 
> > This should not be that big problem as explained above. Note that
> > hub->disconnected was tested in hub_events() without the lock
> > even before this patch. Hence I thought that the new code was as racy
> > as before.
> 
> But you ignored what the comment says about "don't let it be added 
> again".

If we increase the reference in kick_hub_wq() and test
hub->disconnected at the very beginning of hub_event() it
should not cause real problem. In the worst case, it will release
struct usb_hub there instead of in hub_disconnect(). It already happens
with the original code in some circumstances.

If you think that this is too tricky, I will put the lock back.


Thanks for patience.

Best Regards,
Petr

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Petr Mládek]

On Tue 16-09-14 11:10:31, Petr Mládek wrote:
> Anyway, the solution for the race between kick_hub_wq() and
> hub_event() might be to get the reference already in kick_hub_wq().
> I mean something like:
> 
> static void kick_hub_wq(struct usb_hub *hub)
> {
> 	if (hub->disconnected || work_pending(&hub->events))
> 		return;
> 	/*
> 	 * Suppress autosuspend until the event is proceed.
> 	 *
> 	 * Be careful and make sure that the symmetric operation is
> 	 * always called. We are here only when there is no pending
> 	 * work for this hub. Therefore put the interface either when
> 	 * the new work is called or when it is canceled.
> 	 */
> 	usb_autopm_get_interface_no_resume(to_usb_interface(hub->intfdev));
> 	kref_get(&hub->kref);
> 
> 	if (queue_work(hub_wq, &hub->events))
> 		return;
> 
> 	/* the work could not be scheduled twice */
> 	kref_put(&hub->kref, hub_release);
> 	usb_autopm_put_interface_no_suspend(intf);
> }

I have just realized that I was working in the Linus' three where the
commit c605f3cdff53a743f6d87 ("usb: hub: take hub->hdev reference when
processing from eventlist") was missing :-(

It means that we would need to call also usb_get_dev(hdev) in
kick_hub_wq() to make sure that everything is valid when the work
hub_event() is proceed.


Hmm, it stopped being nice. So, I was looking for another solution and
found an interesting behavior of cancel_work_sync() that is used in
hub_disconnect(). It either removes the work item from the queue or it
waits until the work is done. Anyway, no work is pending when the
function returns. It means that struct usb_hub never will be released
when hub_event() is in progress and we do not need to get the
references in advance at all.


I am really sorry for sending so many mails. I am still getting familiar with
kernel hacking. I am often surprised how complex it is. I do not want
to spam and lose your interest, definitely. So, I am going to calm
down and be more patient when doing the homework.


Best Regards,
Petr

Re: [PATCH 1/4] usb: hub: convert khubd into workqueue

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 3678 bytes --]

On Tue, 16 Sep 2014, Petr [iso-8859-1] Mládek wrote:

> Anyway, the solution for the race between kick_hub_wq() and
> hub_event() might be to get the reference already in kick_hub_wq().

Yes, this probably would work.

> I mean something like:
> 
> static void kick_hub_wq(struct usb_hub *hub)
> {
> 	if (hub->disconnected || work_pending(&hub->events))
> 		return;
> 	/*
> 	 * Suppress autosuspend until the event is proceed.
> 	 *
> 	 * Be careful and make sure that the symmetric operation is
> 	 * always called. We are here only when there is no pending
> 	 * work for this hub. Therefore put the interface either when
> 	 * the new work is called or when it is canceled.
> 	 */
> 	usb_autopm_get_interface_no_resume(to_usb_interface(hub->intfdev));
> 	kref_get(&hub->kref);
> 
> 	if (queue_work(hub_wq, &hub->events))
> 		return;
> 
> 	/* the work could not be scheduled twice */
> 	kref_put(&hub->kref, hub_release);
> 	usb_autopm_put_interface_no_suspend(intf);

This should be usb_autopm_put_interface_async, not *_no_suspend,
because you don't know at this point whether the runtime PM usage
counter is > 1.  (You do know it was > 1 at the time queue_work was 
called, but it may have changed since then.)

> }
> 
> And test for hub->disconnected at the very beginning of hub_event().

No, that's no good.  The value isn't fully meaningful unless you are 
holding the device lock.  That's why the test for hub->disconnected is 
placed where it is in hub_events.

> Also we would need to put the reference when the work is canceled in
> hub_disconnect().

You can't cancel the work in hub_disconnect.  That's because cancelling
a work item is always synchronous -- there's no cancel_work, only
cancel_work_sync.

This is okay; removing items from the hub_event_list was only a minor 
optimization anyway.  There's no harm in letting the work item go ahead 
and run.

> Hmm, I am not 100% sure if we could call
> usb_autopm_get_interface_no_resume() without any lock here.
> I guess so because otherwise the original code would not work.
> The check for "hub->disconnected" would fail if the struct was freed
> before the lock was taken.

The caller of kick_khubd has to guarantee that the hub structure hasn't 
been deallocated.

> It looks promissing. hub->intfdev is released together with "hub" in
> hub_release(). Also it seems that kick_* and *disconnect functions
> are called with some top level lock. For example, there is used dev
> lock in drivers/usb/core/device.c.

There's one more improvement you could make.  It's not necessary to
call usb_get_dev and usb_put_dev every time hub_events runs.  
Instead, we can call usb_get_dev once when the hub structure is created 
and usb_put_dev in hub_release.

> > But you ignored what the comment says about "don't let it be added 
> > again".
> 
> If we increase the reference in kick_hub_wq() and test
> hub->disconnected at the very beginning of hub_event() it
> should not cause real problem. In the worst case, it will release
> struct usb_hub there instead of in hub_disconnect(). It already happens
> with the original code in some circumstances.
> 
> If you think that this is too tricky, I will put the lock back.

It's probably okay without the lock.  Please post an updated patch 
(just the first one in the series; the later ones will remain the same) 
with the changes discussed here so I can review it.

Or better yet, rearrange the patch series.  Make the first patch be the 
one that removes the useless loop in hub_events.  That patch will be 
acceptable in any case, regardless of what happens with the workqueue 
change.  Then the second patch can be the conversion to a workqueue.

Alan Stern

Re: [PATCH 2/4] usb: hub: remove obsolete while cycle in hub_event()

[Petr Mládek]

On Fri 12-09-14 10:23:26, Alan Stern wrote:
> On Fri, 12 Sep 2014, Petr Mladek wrote:
> 
> > The USB hub events are proceed by workqueue instead of kthread now.
> > The result is that hub_event() function processes only one event.
> > The while cycle was not removed earlier to show the real changes when
> > switching to the workqueue.
> > 
> > This patch also consolidates the goto targets and rename them from "loop*"
> > to "out*".
> > 
> > When touching the code, it fixes also formatting of dev_err() and dev_dbg()
> > calls to make checkpatch.pl happy :-)
> 
> Although the reason given in the description above is not really
> accurate, removing the loop in hub_events is a reasonable thing to do.  
> That loop doesn't serve any purpose as far as I can see.

IMHO, the while cycle was needed when hub_events() was used in
khubd thread. More events could had been added to the list before
the thread was scheduled.

Therefore I think that the above description makes sense.

And this is why I did not change the order of the patches in v2 of the patch
set. I sent all patches again because this one and the 3rd one needed
refresh.

Best Regards,
Petr

Re: [PATCH 2/4] usb: hub: remove obsolete while cycle in hub_event()

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1490 bytes --]

On Wed, 17 Sep 2014, Petr [iso-8859-1] Mládek wrote:

> On Fri 12-09-14 10:23:26, Alan Stern wrote:
> > On Fri, 12 Sep 2014, Petr Mladek wrote:
> > 
> > > The USB hub events are proceed by workqueue instead of kthread now.
> > > The result is that hub_event() function processes only one event.
> > > The while cycle was not removed earlier to show the real changes when
> > > switching to the workqueue.
> > > 
> > > This patch also consolidates the goto targets and rename them from "loop*"
> > > to "out*".
> > > 
> > > When touching the code, it fixes also formatting of dev_err() and dev_dbg()
> > > calls to make checkpatch.pl happy :-)
> > 
> > Although the reason given in the description above is not really
> > accurate, removing the loop in hub_events is a reasonable thing to do.  
> > That loop doesn't serve any purpose as far as I can see.
> 
> IMHO, the while cycle was needed when hub_events() was used in
> khubd thread. More events could had been added to the list before
> the thread was scheduled.

There's nothing wrong with that.  The thread would get around to 
executing the extra events in due course.

> Therefore I think that the above description makes sense.
> 
> And this is why I did not change the order of the patches in v2 of the patch
> set. I sent all patches again because this one and the 3rd one needed
> refresh.

The patch is getting close.  There are still a couple of problems 
remaining.  I'll go into more detail in another email.

Alan Stern