On Fri, 30 Apr 2004, Oliver Neukum wrote:
> Am Freitag, 30. April 2004 20:01 schrieb Alan Stern:
> > Well, in your patch you've got it changing urb->phase. And that's at a
> > time when usbcore doesn't own the URB.
>
> It is a private field and we still own a reference count.
I accept the "private field" argument. That doesn't affect the fact that
you _are_ changing something in the URB.
> > Furthermore, the issue isn't how giveback_urb knows when the handler has
> > run -- it's how usb_kill_urb() knows.
>
> By being woken up.
Okay, yes, I admit, if you can tell reliably that the URB isn't being used
then you can use a waitqueue for waking up. It's not clear how much
better this is than polling. Especially when you consider that a large
proportion of synchronous unlinks are done at disconnect() time, when the
URBs have already been dequeued and the completion handlers have run, so
the test succeeds the very first time.
> > I looked at it quickly. In effect, your urb->phase _is_ a refcount,
> > although it only takes on 2-1/2 values (DOING and DONE are approximately
> > equivalent). You have ignored the possibility that giveback_urb might be
> > called reentrantly for the same URB. You have ignored the possibility
>
> How? giveback_urb is an irq handler. You would have to resubmit
> to a device on another bus.
Exactly. That's perfectly legal, just as calling usb_get_urb() is. If
you don't object to one then you shouldn't object to the other.
> > that more than one thread might call usb_kill_urb() for the same URB
> > simultaneously. Rather than preventing resubmission, you are allowing it
> > and then awkwardly trying to unlink the URB again.
>
> You cannot reliably prevent resubmission. There's a race between
> setting the flag and checking it. It would need further locking.
> Your patch falls short in that regard, too. It's a hard problem.
Not at all. The core already contains the necessary locking, and I
mentioned in passing that the check has to be done at the right place.
This simple patch will do the job:
--- 1.137/drivers/usb/core/hcd.c Wed Apr 21 06:46:29 2004
+++ edited/drivers/usb/core/hcd.c Fri Apr 30 14:51:51 2004
@@ -1074,7 +1074,10 @@
// FIXME: verify that quiescing hc works right (RH cleans up)
spin_lock_irqsave (&hcd_data_lock, flags);
- if (HCD_IS_RUNNING (hcd->state) && hcd->state != USB_STATE_QUIESCING) {
+ if (unlikely (urb->transfer_flags & URB_REJECT)) {
+ INIT_LIST_HEAD (&urb->urb_list); //(*)
+ status = -EPERM;
+ } else if (HCD_IS_RUNNING (hcd->state) && hcd->state != USB_STATE_QUIESCING) {
usb_get_dev (urb->dev);
list_add_tail (&urb->urb_list, &dev->urb_list);
status = 0;
(I'm not sure about the line marked (*); I just copied it from below. I
have a vague feeling that it might be necessary, but at the moment I
don't recall why.)
This test is in a region protected by hcd_data_lock. If the URB_REJECT
flag is set before entering the region then the submission will fail. If
not, then the subsequent call to usb_unlink_urb() -- which also uses
hcd_data_lock -- will find the URB fully linked and will be able to unlink
it.
> > Also, you have added a bunch of extra stuff to struct urb, which is
> > already too big, and a lot of extra code to usbcore. If you object to
> > using the existing refcount, wouldn't it be a lot simpler to adopt my
> > scheme but using an additional submission-count field? That would
> > increase the size of struct urb by a smaller amount.
>
> Why does the size of struct urb matter? It is not a numerous struct
> like an inode or a dentry. Making URBs more efficient for 2.7 requires
> us to grow struct urb, not shrink it.
> As for the code, little of it is in hot code path and the part which is,
> allows removal of code from properly written handlers like hub_irq()
My scheme allows the same code removal from hub_irq(). In fact, working
on that handler was one of the things that got me thinking about this
stuff again.
On the whole, the only advantage I can see in your way of doing things is
that usb_kill_urb() gets woken up more quickly. The difference is just a
few milliseconds, and since this is a routine that expects to sleep that
doesn't seem important. However, if you really wanted to, you could
change my scheme by putting in the waitqueue head and another refcount.
That would still be no larger than your change to struct urb, and the
coding would be much simpler.
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
