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

Reply via email to