[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#670398: Deadlock in hid_reset when Dell iDRAC is reset



I'm handling a bug report to Debian by Sven Hoexter (cc'd) involving
lockups on Dell hardware, which seem to involve USB HID reset.  The bug
report is at <http://bugs.debian.org/670398>.

I found that Red Hat recently made a bug fix credited to you, described as:

BZ#797205
        Due to a bug in the hid_reset() function, a deadlock could occur
        when a Dell iDRAC controller was reset. Consequently, its USB
        keyboard or mouse device became unresponsive. A patch that fixes
        the underlying code has been provided to address this bug and
        the hangs no longer occur in the described scenario.

Do the symptoms that Sven found match your understanding of the bug?

Ben.

(I was also able to extract a patch by comparing the Red Hat packages:

--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -469,10 +469,8 @@
  * talking to TTs must queue control transfers (not just bulk and iso), so
  * both can talk to the same hub concurrently.
  */
-static void hub_tt_work(struct work_struct *work)
+void _hub_tt_work(struct usb_hub *hub)
 {
-	struct usb_hub		*hub =
-		container_of(work, struct usb_hub, tt.clear_work);
 	unsigned long		flags;
 	int			limit = 100;
 
@@ -507,6 +505,14 @@
 	spin_unlock_irqrestore (&hub->tt.lock, flags);
 }
 
+void hub_tt_work(struct work_struct *work)
+{
+	struct usb_hub		*hub =
+		container_of(work, struct usb_hub, tt.clear_work);
+
+	_hub_tt_work(hub);
+}
+
 /**
  * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
  * @urb: an URB associated with the failed or incomplete split transaction
@@ -554,7 +560,20 @@
 	/* tell keventd to clear state for this TT */
 	spin_lock_irqsave (&tt->lock, flags);
 	list_add_tail (&clear->clear_list, &tt->clear_list);
-	schedule_work(&tt->clear_work);
+	/* don't schedule on kevent if we're running on keventd (e.g.,
+	 * in hid_reset we can get here on kevent) unless on >=2.6.36
+	 */
+	if (!(current->flags & PF_KTHREAD) || !strstr(current->comm, "events"))
+		/* put it on keventd */
+		schedule_work(&tt->clear_work);
+	else {
+		/* let khubd do it */
+		struct usb_hub		*hub =
+			container_of(&tt->clear_work, struct usb_hub,
+					tt.clear_work);
+		kick_khubd(hub);
+	}
+
 	spin_unlock_irqrestore (&tt->lock, flags);
 	return 0;
 }
@@ -3421,6 +3440,10 @@
 		if (hub->quiescing)
 			goto loop_autopm;
 
+		/* _hub_tt_work usually run on keventd */
+		if (!list_empty(&hub->tt.clear_list))
+			_hub_tt_work(hub);
+
 		if (hub->error) {
 			dev_dbg (hub_dev, "resetting for error %d\n",
 				hub->error);
-- END ---

but if you can provide a version with your own description and sign-off,
I would be grateful for it.)

-- 
Ben Hutchings
Design a system any fool can use, and only a fool will want to use it.

Attachment: signature.asc
Description: This is a digitally signed message part


Reply to: