Am 13.10.2012 05:01, schrieb Ming Lei: > On Sat, Oct 13, 2012 at 6:11 AM, Tilman Schmidt <tilman@xxxxxxx> wrote: >> >> There are two cases I have to worry about: >> (1) a reset triggered by int_in_work() >> (2) a reset triggered by some other source external to the driver >> >> In case (1) the current design will deadlock because pre_reset() is >> indirectly called from int_in_work() but calls cancel_work_sync() >> which will wait for this very work item to complete. This would be >> fixed by pre_reset() not calling cancel_work_sync() at all. > > Maybe you need to set a flag in int_in_work() to indicate that > current reset is from itself, so you may fix both two cases. > >> int_in_work() will then run to completion without resubmitting >> urb_int_in because it's already past that point once it calls >> usb_reset_device(), so there's no conflict with post_reset() doing >> that. >> >> In case (2) the current design will work fine, but if I remove the >> call to cancel_work_sync() from pre_reset() there's a slight chance >> that an uncompleted int_in_work() work item remains uncancelled. >> So any part of int_in_work() might run asynchronously after >> pre_reset(). The question is whether this might cause a problem. >> >> Which of the two cases does your argument address? > > I am saying the case 2, and the current design should be fine > if the 1sec timeout can be tolerated in int_in_work(). > > You are right on case 1. Ok, what do you think about this patch? It uses the fact that pre_reset() calls the suspend method with message=PMSG_ON, which the USB framework never does, to skip the cancel_work_sync() in that case. It also avoids the extraneous reset if usb_submit_urb() fails because post_reset() has already submitted the interrupt URB. -- Tilman Schmidt E-Mail: tilman@xxxxxxx Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
diff --git a/drivers/isdn/gigaset/bas-gigaset.c b/drivers/isdn/gigaset/bas-gigaset.c
index 5275887..3c69dbf 100644
--- a/drivers/isdn/gigaset/bas-gigaset.c
+++ b/drivers/isdn/gigaset/bas-gigaset.c
@@ -617,7 +617,11 @@ static void int_in_work(struct work_struct *work)
if (rc == 0)
/* success, resubmit interrupt read URB */
rc = usb_submit_urb(urb, GFP_ATOMIC);
- if (rc != 0 && rc != -ENODEV) {
+
+ /* try to recover from failures by resetting the device, except
+ * for -ENODEV (device gone) and -EINVAL (URB already resubmitted)
+ */
+ if (rc != 0 && rc != -EINVAL && rc != -ENODEV) {
dev_err(cs->dev, "clear halt failed: %s\n", get_usb_rcmsg(rc));
rc = usb_lock_device_for_reset(ucs->udev, ucs->interface);
if (rc == 0) {
@@ -2442,7 +2446,9 @@ static void gigaset_disconnect(struct usb_interface *interface)
}
/* gigaset_suspend
- * This function is called before the USB connection is suspended.
+ * This function is called before the USB connection is suspended
+ * or before the USB device is reset.
+ * In the latter case, message == PMSG_ON.
*/
static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
{
@@ -2498,7 +2504,12 @@ static int gigaset_suspend(struct usb_interface *intf, pm_message_t message)
del_timer_sync(&ucs->timer_atrdy);
del_timer_sync(&ucs->timer_cmd_in);
del_timer_sync(&ucs->timer_int_in);
- cancel_work_sync(&ucs->int_in_wq);
+
+ /* don't try to cancel int_in_wq from within reset as it
+ * might be the one requesting the reset
+ */
+ if (message.event != PM_EVENT_ON)
+ cancel_work_sync(&ucs->int_in_wq);
gig_dbg(DEBUG_SUSPEND, "suspend complete");
return 0;
Attachment:
signature.asc
Description: OpenPGP digital signature
