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.

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.

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.

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.

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.)

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.

Alan Stern




-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to