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
