Am Dienstag, 14. Januar 2003 18:29 schrieb Alan Stern: > On Tue, 14 Jan 2003, Oliver Neukum wrote: > > Hi, > > > > thanks for your suggestion about a spinlock. > > Here it is again, after a redesign based on your suggestion. > > > > I'll start testing this, when I'm back home. > > > > Regards > > Oliver > > Your patch looks like a pretty good start to me. It guarantees that an > unlink during a completion handler will prevent a resubmission, and it > insures that usb_unlink_urb() won't return before the handler is done. > > You need to add the spinlocking and internal_state stuff to > unlink_complete() in hcd.c.
Right. Done. > There's a little problem with usb_hcd_giveback_urb(). It boils down to > this: who owns the urb while the completion handler is running? Since the > driver needs to able to resubmit from within the handler, the driver > should be the owner. But, the core needs to be able to set the > internal_state back to IDLE after the handler has finished (assuming no > resubmission), so the core needs to retain ownership of internal_state. internal_state is always owned by the core. > This problem shows up where you set internal_state back to IDLE. You > should only set it to IDLE if the state currently is COMPLETING. After > all, at that point if the urb has been resubmitted, you don't want to > change the state from whatever it is -- probably SUBMITTED but maybe > CANCELED. Right. Done. > A related problem is this. If an urb is cancelled before it finishes, > while the handler is running should internal_state be CANCELED or > COMPLETING? Since the handler needs to be able to resubmit, I think it > should be set to COMPLETING. That has to be added to your patch. No, by your own argument below it must be CANCELED. If there's really a need to resubmit in this case from the handler, I'll provide a helper function to force the state to COMPLETING. > This leads to a third problem. Suppose the handler resubmits, and the urb > either finishes normally or gets cancelled, all before the handler > returns. The handler_lock will prevent the handler from being re-entered, > but the state will still get set to COMPLETING, which will confuse things > upon return from the initial call of the handler. To prevent that from > happening, internal_state should be set to COMPLETING only while the > handler_lock is held. (This may lead to more problems, since > internal_state must be under the control of both lock and handler_lock.) Right. But the point is moot if usb_unlink_urb() sets the state to CANCELED, isn't it? > So the lifecycle now looks like this: > > IDLE --> SUBMITTED [--> CANCELED] -->* COMPLETING { -->* IDLE > { --> SUBMITTED > > where the transitions marked with * can only take place under control of > handler_lock. > > I think this is pretty much how it should work. I'm still bothered by one > thought. There's an inevitable race between completion notification and > unlinking. What is the deciding factor: how should we distinguish between > an unlink affecting the current urb and an unlink preventing a > resubmission? That line has to be crossed before invoking the completion > handler; I'm just not sure that the scheme outlined above does it > correctly. Hm, I have to think about that. Meanwhile, what about the current version? Regards Oliver PS: 2.5.57 simply causes a blank screen after boot. Any ideas? # This is a BitKeeper generated patch for the following project: # Project Name: greg k-h's linux 2.5 USB kernel tree # This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas: # ChangeSet 1.1270 -> 1.1272 # include/linux/usb.h 1.122 -> 1.123 # drivers/usb/core/urb.c 1.19 -> 1.20 # drivers/usb/core/hcd.c 1.86 -> 1.88 # # The following is the BitKeeper ChangeSet Log # -------------------------------------------- # 03/01/14 [EMAIL PROTECTED] 1.1271 # - explicit URB state # - improvement in unlinking semantics # -------------------------------------------- # 03/01/14 [EMAIL PROTECTED] 1.1272 # - spinlocks in unlink notification path # - fix state in case of resubmission # -------------------------------------------- # diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c --- a/drivers/usb/core/hcd.c Tue Jan 14 19:48:27 2003 +++ b/drivers/usb/core/hcd.c Tue Jan 14 19:48:27 2003 @@ -1082,8 +1082,13 @@ /* issue original completion call */ urb->complete = splice->complete; urb->context = splice->context; + spin_lock(&urb->handler_lock); urb->complete (urb, regs); + spin_unlock(&urb->handler_lock); + spin_lock(&urb->lock); + urb->internal_state = URB_IDLE; + spin_unlock(&urb->lock); /* then let the synchronous unlink call complete */ complete (&splice->done); } @@ -1138,14 +1143,23 @@ goto done; } - /* Any status except -EINPROGRESS means something already started to - * unlink this URB from the hardware. So there's no more work to do. - * - * FIXME use better explicit urb state + /* + * We decide based upon the internal_state what's to be done */ - if (urb->status != -EINPROGRESS) { + switch (urb->internal_state) { + case URB_SUBMITTED: /* normal case */ + urb->internal_state = URB_CANCELED; + break; + default: + BUG(); + case URB_IDLE: + case URB_CANCELED: retval = -EBUSY; goto done; + case URB_COMPLETING: + retval = -EBUSY; + urb->internal_state = URB_CANCELED; + goto err_out_waiting; } /* maybe set up to block until the urb's completion fires. the @@ -1205,6 +1219,18 @@ if (retval && sys) dev_dbg (sys, "hcd_unlink_urb %p fail %d\n", urb, retval); return retval; + +err_out_waiting: + /* we are asked to unlink while the completing routine is already + running. We drop the locks and spin in case of synchronous unlink */ + spin_unlock (&hcd_data_lock); + spin_unlock_irqrestore (&urb->lock, flags); + if (!(urb->transfer_flags & URB_ASYNC_UNLINK)) { + spin_lock_irq(&urb->handler_lock); + spin_unlock_irq(&urb->handler_lock); + } + + return retval; } /*-------------------------------------------------------------------------*/ @@ -1287,6 +1313,10 @@ void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs) { urb_unlink (urb); + spin_lock(&urb->lock); + if (urb->internal_state == URB_SUBMITTED) + urb->internal_state = URB_COMPLETING; + spin_unlock(&urb->lock); // NOTE: a generic device/urb monitoring hook would go here. // hcd_monitor_hook(MONITOR_URB_FINISH, urb, dev) @@ -1307,7 +1337,14 @@ } /* pass ownership to the completion handler */ + spin_lock(&urb->handler_lock); urb->complete (urb, regs); + spin_unlock(&urb->handler_lock); + /* we are now idle unless the urb was resubmitted */ + spin_lock(&urb->lock); + if (urb->internal_state == URB_COMPLETING) + urb->internal_state = URB_IDLE; + spin_unlock(&urb->lock); usb_put_urb (urb); } EXPORT_SYMBOL (usb_hcd_giveback_urb); diff -Nru a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c --- a/drivers/usb/core/urb.c Tue Jan 14 19:48:27 2003 +++ b/drivers/usb/core/urb.c Tue Jan 14 19:48:27 2003 @@ -44,6 +44,7 @@ memset(urb, 0, sizeof(*urb)); urb->count = (atomic_t)ATOMIC_INIT(1); spin_lock_init(&urb->lock); + spin_lock_init(&urb->handler_lock); return urb; } @@ -192,6 +193,8 @@ struct usb_device *dev; struct usb_operations *op; int is_out; + int r; + unsigned long flags; if (!urb || urb->hcpriv || !urb->complete) return -EINVAL; @@ -346,7 +349,28 @@ urb->interval = temp; } + spin_lock_irqsave(urb->lock, flags); + switch (urb->internal_state) { + case URB_IDLE: + case URB_COMPLETING: + urb->internal_state = URB_SUBMITTED; + break; + case URB_CANCELED: + r = -EINTR; + goto err_out; + case URB_SUBMITTED: + default: + BUG(); + r = -EBUSY; + goto err_out; + } + spin_unlock_irqrestore(urb->lock, flags); + return op->submit_urb (urb, mem_flags); + +err_out: + spin_unlock_irqrestore(urb->lock, flags); + return r; } /*-------------------------------------------------------------------*/ diff -Nru a/include/linux/usb.h b/include/linux/usb.h --- a/include/linux/usb.h Tue Jan 14 19:48:27 2003 +++ b/include/linux/usb.h Tue Jan 14 19:48:27 2003 @@ -728,6 +728,8 @@ { spinlock_t lock; /* lock for the URB */ atomic_t count; /* reference count of the URB */ + int internal_state; /* state in the URB's lifecycle */ + spinlock_t handler_lock; /* lock during completion routine */ void *hcpriv; /* private data for host controller */ struct list_head urb_list; /* list pointer to all active urbs */ struct usb_device *dev; /* (in) pointer to associated device */ @@ -750,6 +752,11 @@ usb_complete_t complete; /* (in) completion routine */ struct usb_iso_packet_descriptor iso_frame_desc[0]; /* (in) ISO ONLY */ }; + +#define URB_IDLE 0 +#define URB_SUBMITTED 1 +#define URB_COMPLETING 2 +#define URB_CANCELED 3 /* -------------------------------------------------------------------------- */ ------------------------------------------------------- This SF.NET email is sponsored by: Take your first step towards giving your online business a competitive advantage. Test-drive a Thawte SSL certificate - our easy online guide will show you how. Click here to get started: http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0027en _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel