> From: "Dunlap, Randy" <[EMAIL PROTECTED]>
> Date: Tue, 27 Feb 2001 12:21:00 -0800
> > If Randy or anyone else explain me why it happens, I am willing
> > to come up with a patch which would redo hub_disconnect()
> > to put hubs on reaping list and have usb_hub_events actually
> > destroy them. Then I will need some review and testing.
>
> Sounds like a decent solution.
>[...]
> Maybe use a semaphore instead of the spinlock, so that it
> can block?
OK. You were right. Delaying reaping without other arrangements
was a bad idea. When I tried that, rmmod thread would quickly
pass through hub_disconnect and then release the usb-uhci module,
while khubd continued to run, and oopsed the system at the next
get_status. Ergo, hub_disconnect must wait. And it is easier
to wait with a semafore than with add_wait_queue, schedule, etc.
BTW. I found a good way to reproduce races in hub.c: I run kudzu
on SMP. Kudzu loads whole stack, then pulls usb-uhci from it
immediately, causing disconnects right when khubd starts getting
excited about new devices.
I tried several prototypes and all were racy one way or another,
or caused recursive locks.
In the end I came up with a somewhat bizzare scheme which works.
I say bizzare because: 1. it calls down() from under spinlock
(in most cases it is no-no!) 2. has down();up(); with no code
in between, which looks strange.
Still, I dare everyone to come up with a scenario where the attached
patch fails to work. If you cannot, apply to your tree. :)
-- Pete
diff -ur -X ../dontdiff linux-2.4.2-ac12/drivers/usb/hub.c
linux-2.4.2-ac12-p3/drivers/usb/hub.c
--- linux-2.4.2-ac12/drivers/usb/hub.c Wed Mar 7 12:17:02 2001
+++ linux-2.4.2-ac12-p3/drivers/usb/hub.c Thu Mar 8 19:43:14 2001
@@ -357,6 +357,7 @@
INIT_LIST_HEAD(&hub->event_list);
hub->dev = dev;
+ init_MUTEX(&hub->khubd_sem);
/* Record the new hub's existence */
spin_lock_irqsave(&hub_event_lock, flags);
@@ -400,6 +399,9 @@
spin_unlock_irqrestore(&hub_event_lock, flags);
+ down(&hub->khubd_sem); /* Wait for khubd to leave this hub alone. */
+ up(&hub->khubd_sem);
+
if (hub->urb) {
usb_unlink_urb(hub->urb);
usb_free_urb(hub->urb);
@@ -709,7 +717,7 @@
spin_lock_irqsave(&hub_event_lock, flags);
if (list_empty(&hub_event_list))
- goto he_unlock;
+ break;
/* Grab the next entry from the beginning of the list */
tmp = hub_event_list.next;
@@ -720,6 +728,7 @@
list_del(tmp);
INIT_LIST_HEAD(tmp);
+ down(&hub->khubd_sem); /* never blocks, we were on list */
spin_unlock_irqrestore(&hub_event_lock, flags);
if (hub->error) {
@@ -728,6 +737,7 @@
if (usb_hub_reset(hub)) {
err("error resetting hub %d - disconnecting",
dev->devnum);
usb_hub_disconnect(dev);
+ up(&hub->khubd_sem);
continue;
}
@@ -742,6 +752,7 @@
ret = usb_get_port_status(dev, i + 1, &portsts);
if (ret < 0) {
err("get_port_status failed (err = %d)", ret);
+ up(&hub->khubd_sem);
continue;
}
@@ -803,9 +814,9 @@
usb_hub_power_on(hub);
}
}
+ up(&hub->khubd_sem);
} /* end while (1) */
-he_unlock:
spin_unlock_irqrestore(&hub_event_lock, flags);
}
diff -ur -X ../dontdiff linux-2.4.2-ac12/drivers/usb/hub.h
linux-2.4.2-ac12-p3/drivers/usb/hub.h
--- linux-2.4.2-ac12/drivers/usb/hub.h Wed Mar 7 12:17:02 2001
+++ linux-2.4.2-ac12-p3/drivers/usb/hub.h Thu Mar 8 19:17:38 2001
@@ -134,6 +134,8 @@
struct list_head event_list;
struct usb_hub_descriptor *descriptor;
+
+ struct semaphore khubd_sem;
};
#endif /* __LINUX_HUB_H */
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel
