Am Mittwoch, 15. Januar 2003 21:54 schrieb Alan Stern:
> On Wed, 15 Jan 2003, Oliver Neukum wrote:
> > Am Mittwoch, 15. Januar 2003 17:04 schrieb Alan Stern:
> > > resubmitted urbs. Second (and more subtle), there are differences
> > > between resubmitting an urb from the handler as opposed to submitting
> > > it again after the handler is done. For example, any bandwidth
> > > reserved for the urb is kept when the handler resubmits, but it is
> > > deallocated otherwise.
> >
> > Eh, how so? usb_hcd_giveback_urb() does call unlink_urb()
> > unconditionally.
>
> This is where my lack of knowledge of the details underlying the usb core
> shows through. Okay, looking at hcd.c I see where urb_unlink() is called
> and how it releases the bandwidth. I'm surprised; is it really supposed
> to work this way?
Questionable. Probably it was alright during the days of automatic
resubmission. But it's another issue.
> I recall a long discussion a few months ago between you and David Brownell
> concerning the question of how to reserve bandwidth. The upshot was that
There was such a discussion, but I did hardly take part.
> there was no need, because bandwidth was allocated on demand and would be
> retained as long as an urb was resubmitted by its completion handler.
> Looking at the code, it appears that is not the case --
> usb_release_bandwidth() is called whether the urb is resubmitted or not.
>
> I don't know what ought to happen here. Somebody will have to fill me in.
>
> > The URB cannot finish regularly because that would be a hardware
> > IRQ reentering itself. The generic IRQ code should prevent that.
>
> Okay, there's no reason to worry about the possibility of a resubmitted
> urb being COMPLETING, and hence no reason to acquire the handler_lock
> before setting the state to COMPLETING. (But what if the resubmitted urb
> goes to a different usb device, on a different bus attached to a different
> IRQ line? Can't one hardware IRQ interrupt another? What if it is
> fielded by a different processor?)
Then we may have a problem. I've a plan for that. Could we defer this to
a seperate thread?
> > So the only cause of a status change at that point can be
> > usb_unlink_urb(). In that case going to CANCELED seem to be correct and
> > not causing a problem.
> >
> > IMHO changing the status to CANCELED is no problem and neither
> > is spinning on the handler_lock. Submitting and unlinking an URB
> > from a completion handler is arcane anyway.
>
> The situation I had in mind (rare, I admit -- but that's where the
> gremlins live) is that the handler always resubmits, but one time through
> the loop another processor decides to unlink the urb somewhere between
> when it was resubmitted and when the handler returned. When that
> happens the state will be CANCELED upon the return from the handler, and
> so usb_hcd_giveback_urb() will proceed to change the state to IDLE. (It
> has to assume this was a normally cancelled urb, not one that was
> resubmitted and then cancelled.) This would be wrong; the state should
> remain CANCELED since the handler for the cancelled urb has not yet been
> called. Now the state is corrupted, something I would like to avoid.
You are right, it's a bug. The decision must take the state upon entry
into consideration. I'll fix that.
> Although corrupting the state in this way seems innocent enough here, it
> has the potential to lead to problems later on. Since it only requires a
> little more programming effort to get things right, I think we should do
> so.
Ack.
> > Could you explain why CANCELED and TERMINATING should be differentiated?
> > It seems to me that both usb_unlink_urb() and usb_submit_urb() would fail
> > with an error in both cases.
>
> Well, the explanation above shows why something like TERMINATING needs to
> exist. Differentiating them occurs in two places. Immediately after the
> call to the completion handler, the state gets set to IDLE if it was
> TERMINATING but not if it was CANCELED. Although usb_submit_urb() will
> fail with an error in both cases, the errors ought to be different: If the
> state was CANCELED then the urb was resubmitted _before its handler was
> called_; definitely a programming bug. If the state was TERMINATING then
Yes, but detecting errors is an accidental benefit. If you do something obviously
stupid, you'll suffer deservedly. I don't that this is a consideration here.
Simplicity is paramount IMHO.
[..]
> If you feel that my approach still isn't right and want to discuss it some
> more, I'll be happy to oblige.
Do you like this version?
Regards
Oliver
# 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.1274
# 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.90
#
# 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
# --------------------------------------------
# 03/01/15 [EMAIL PROTECTED] 1.1273
# - remove incorrect spinlocking
# --------------------------------------------
# 03/01/15 [EMAIL PROTECTED] 1.1274
# - correct state handling in unlink case
# --------------------------------------------
#
diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c Wed Jan 15 22:28:59 2003
+++ b/drivers/usb/core/hcd.c Wed Jan 15 22:28:59 2003
@@ -1138,14 +1138,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 +1214,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;
}
/*-------------------------------------------------------------------------*/
@@ -1286,7 +1307,13 @@
*/
void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs)
{
+ int old_state;
+
urb_unlink (urb);
+ spin_lock(&urb->lock);
+ if ((old_state = 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 +1334,15 @@
}
/* 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 || old_state == URB_CANCELED)
+ 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 Wed Jan 15 22:28:59 2003
+++ b/drivers/usb/core/urb.c Wed Jan 15 22:28:59 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 Wed Jan 15 22:28:59 2003
+++ b/include/linux/usb.h Wed Jan 15 22:28:59 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: A Thawte Code Signing Certificate
is essential in establishing user confidence by providing assurance of
authenticity and code integrity. Download our Free Code Signing guide:
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0028en
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
