Am Montag, 13. Januar 2003 16:16 schrieb Alan Stern:
> On Mon, 13 Jan 2003, Oliver Neukum wrote:
> > Let me explain.
> > The first problem we avoid is with unloading. We want to be sure that
> > the completion handler is not running after return from usb_unlink_urb().
> > If we wish to make the guarantee, usb_unlink_urb() must not simply
> > fail if the completion handler is running. A synchronous unlink must
> > wait.
> >
> > The second problem we fix is with resubmission from the completion
> > handler. If we have a state for unlinked during completion, we can fail
> > resubmission. Thus we kill races with disconnect in rx paths.
>
> This is highly analogous to the problem of cancelling a kernel timer. If
Absolutely.
> the timer's handler reschedules another timer interrupt, it can be hard to
> cancel reliably. For kernel timers, the problem was only partially solved
> by having del_timer() return a code indicating whether or not the timer
> was still active and by adding del_timer_sync(). There still has to be a
> requirement that the handler must avoid restarting the timer.
>
> Much the same is true for USB. No matter what the core does, if the
> user's completion handler resubmits, it will be hard to kill the URB
> entirely.
>
> Furthermore, there is a conceptual problem that has not been fully
> addressed. When a usb_unlink_urb() is called for an URB whose completion
> handler is running, which incarnation are we being asked to cancel? The
> one that just completed or the one that the completion handler is about to
> submit? If you can't answer this question first, then naturally you don't
> know what you're supposed to do.
My answer is not kill the URB about to be submitted, but to simply
fail the call to usb_submit_urb(). I believe that to be the simplest solution.
> As far as guaranteeing that when usb_unlink_urb() returns from a
> synchronous unlink, the completion handler will have already finished --
> so far as I can tell there is currently no such guarantee. At least, the
There's if the unlink doesn't return an error.
> kerneldoc preceding usb_unlink_urb() is sufficiently vague that it doesn't
> make this promise. Of course, we need to have something like this. But
> in principle there should be no problem -- or rather, the only problem is
Not trivial. The only way to do it is to use complete_and_exit() in the
completion handler.
> dealing with the case that the handler resubmits. What happens if the
> handler resubmits the URB, and another thread tries to unlink the
> newly-submitted incarnation before the handler has returned?
We must make sure that an interrupt handler is not reentered.
> If it weren't for the fact that a completion handler is allowed to
> resubmit an URB, I suspect the best solution would simply be to lock the
> URB while the handler is running.
Yes, if.
By the way. I am including the code I have so far. I am afraid it's still
buggy. But you might have a look at it.
Regards
Oliver
You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.
===================================================================
[EMAIL PROTECTED], 2003-01-13 16:01:26+01:00, [EMAIL PROTECTED]
- state machine
drivers/usb/core/hcd.c | 40 ++++++++++++++++++++++++++++++++++++----
drivers/usb/core/urb.c | 21 ++++++++++++++++++++-
include/linux/usb.h | 15 ++++++++++++---
3 files changed, 68 insertions(+), 8 deletions(-)
diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c Mon Jan 13 16:14:48 2003
+++ b/drivers/usb/core/hcd.c Mon Jan 13 16:14:48 2003
@@ -1101,6 +1101,7 @@
struct device *sys = 0;
unsigned long flags;
struct completion_splice splice;
+ struct completion cp;
int retval;
if (!urb)
@@ -1138,14 +1139,26 @@
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.
+ /*
+ * Decide what is to be done based on state
*
- * FIXME use better explicit urb state
*/
- if (urb->status != -EINPROGRESS) {
+ switch (urb->state) {
+ case URB_SUBMITTED:
+ break; /* common case hopefully */
+ case URB_COMPLETING:
+ urb->state = URB_CANCELED;
+ retval = -EBUSY;
+ if (urb->transfer_flags & URB_ASYNC_UNLINK)
+ goto done;
+ else
+ goto wait_for_handler;
+ case URB_IDLE:
+ case URB_CANCELED:
retval = -EBUSY;
goto done;
+ default:
+ BUG();
}
/* maybe set up to block until the urb's completion fires. the
@@ -1205,6 +1218,14 @@
if (retval && sys)
dev_dbg (sys, "hcd_unlink_urb %p fail %d\n", urb, retval);
return retval;
+
+wait_for_handler:
+ init_completion(&cp);
+ urb->unlink_wait = &cp;
+ spin_unlock (&hcd_data_lock);
+ spin_unlock_irqrestore (&urb->lock, flags);
+ wait_for_completion(&cp);
+ return retval;
}
/*-------------------------------------------------------------------------*/
@@ -1307,7 +1328,18 @@
}
/* pass ownership to the completion handler */
+ spin_lock(&urb->lock);
+ if (urb->state == URB_SUBMITTED) /* in case the unlinking fails */
+ urb->state = URB_COMPLETING;
+ spin_unlock(&urb->lock); /* handler may resubmit */
urb->complete (urb, regs);
+ spin_lock(&urb->lock);
+ if (urb->state != URB_SUBMITTED)
+ urb->state = URB_IDLE;
+ /* somebody might be waiting on us */
+ if (urb->unlink_wait)
+ complete(urb->unlink_wait);
+ 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 Mon Jan 13 16:14:48 2003
+++ b/drivers/usb/core/urb.c Mon Jan 13 16:14:48 2003
@@ -192,6 +192,8 @@
struct usb_device *dev;
struct usb_operations *op;
int is_out;
+ unsigned long flags;
+ int retval;
if (!urb || urb->hcpriv || !urb->complete)
return -EINVAL;
@@ -346,7 +348,24 @@
urb->interval = temp;
}
- return op->submit_urb (urb, mem_flags);
+ retval = 0;
+ spin_lock_irqsave(&urb->lock, flags);
+ switch (urb->state) {
+ case URB_IDLE:
+ case URB_COMPLETING:
+ urb->state = URB_SUBMITTED;
+ break;
+ case URB_CANCELED:
+ retval = -EBUSY;
+ break;
+ case URB_SUBMITTED:
+ default:
+ BUG();
+ break;
+ }
+ spin_unlock_irqrestore(&urb->lock, flags);
+
+ return retval ? retval : op->submit_urb (urb, mem_flags);
}
/*-------------------------------------------------------------------*/
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h Mon Jan 13 16:14:48 2003
+++ b/include/linux/usb.h Mon Jan 13 16:14:48 2003
@@ -583,6 +583,7 @@
* status of the particular request. ISO requests only use it
* to tell whether the URB was unlinked; detailed status for
* each frame is in the fields of the iso_frame-desc.
+ * @state: internal state used for unlinking
* @transfer_flags: A variety of flags may be used to affect how URB
* submission, unlinking, or operation are handled. Different
* kinds of URB can use different flags.
@@ -605,7 +606,7 @@
* transferred. It will normally be the same as requested, unless
* either an error was reported or a short read was performed.
* The URB_SHORT_NOT_OK transfer flag may be used to make such
- * short reads be reported as errors.
+ * short reads be reported as errors.
* @setup_packet: Only used for control transfers, this points to eight bytes
* of setup data. Control transfers always start by sending this data
* to the device. Then transfer_buffer is read or written, if needed.
@@ -626,7 +627,7 @@
* @complete: Completion handler. This URB is passed as the parameter to the
* completion function. The completion function may then do what
* it likes with the URB, including resubmitting or freeing it.
- * @iso_frame_desc: Used to provide arrays of ISO transfer buffers and to
+ * @iso_frame_desc: Used to provide arrays of ISO transfer buffers and to
* collect the transfer status for each buffer.
*
* This structure identifies USB transfer requests. URBs must be allocated by
@@ -660,7 +661,7 @@
* The URB_ASYNC_UNLINK transfer flag affects later invocations of
* the usb_unlink_urb() routine.
*
- * All URBs must also initialize
+ * All URBs must also initialize
* transfer_buffer and transfer_buffer_length. They may provide the
* URB_SHORT_NOT_OK transfer flag, indicating that short reads are
* to be treated as errors; that flag is invalid for write requests.
@@ -726,6 +727,8 @@
{
spinlock_t lock; /* lock for the URB */
atomic_t count; /* reference count of the URB */
+ int state; /* current state, lock must be held for access
+*/
+ struct completion *unlink_wait; /* to wait for a completion handler to finish
+*/
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 */
@@ -748,6 +751,12 @@
usb_complete_t complete; /* (in) completion routine */
struct usb_iso_packet_descriptor iso_frame_desc[0]; /* (in) ISO ONLY */
};
+
+/* internal states an URB may have */
+#define URB_IDLE 0
+#define URB_SUBMITTED 1
+#define URB_COMPLETING 2
+#define URB_CANCELED 3
/* -------------------------------------------------------------------------- */
===================================================================
This BitKeeper patch contains the following changesets:
1.1267
## Wrapped with gzip_uu ##
begin 664 bkpatch18195
M'XL(`.G7(CX``[57;6_;-A#^+/V*&PH$25K;I-YE(UU>D1E-TR"I/Q0H8-`2
M;0F6)8^4DF7S_ON.E-_CIFG6!D%(D7?'>X[WW#%OH">Y:!M%EMYS8;Z!/PI9
MXB?/BYPWDV+"LS2O_FH68H2;MT6!FRVUW*HU6H-QHQ2<RU:5H^38<DV4NV%E
ME`!NR[9!F_9RI7R<\K9Q>W'9NSJY-<VC(SA+6#[B=[R$HR-S,#Z.*YXUQZ)@
MB3IRMMR>68102BV7V*Y'W9D5V+8SHQX9Q#B-+3\<#KS0'`D^.J[5HV*RJ6ZC
M`9OX-+3"F>TXA)KG0)O4\GP@=HO0%K6!>FU"VY;W%O\2`C7&XUW1@+<V-(AY
M"O_?Z3,S@@;(DI4<)BQ*TIR;'\`./.J9-ZL(F8T?_#%-PHCY'K:"$@L%"B],
M#EI1(7BK$H-FI'RUB$=LZCH.]6:4A':(OCHQB3S+=X>AZP7\N8@\8UB'GCIN
M2(.9ZP2!A5X]C5N:1UD5\Y:VJ*PTDU4$'>(Z9$8\Q_%GL679<1"[[M!S;(\^
MZ]0WC*Y[9`74#]"CJ<K1;:?60251O`@4)6IP'4RET*<^)E3(!@YS&*..S0/G
MQP*U-+QRRY\1U_4\39(=&';3Y;41W.;-MX.&&>):U$4[U/?].8.L-0*Y;3=L
MT^`E!*(6-.Q?R*#Z7C]!0SSH7V3$S:Y8OH)873=P@9IP",?ZU#;:+;G(63;W
MHI(\AF$AH"Z+:3XRSST2H$ZW'N#0D$DA2A"<Q1(&'"=3_$8U)H$+40C91!TK
MU#IZ4,>ELN@/!9OP?LQEU%;E.X:R@*DH[M.8`Q."/4HHAM"]^P2E8+D<<@&#
M:HB#!)8K:;3KV=JN'M#N299![_94PJ22);!,%H@H+5.6I7]SL^M;`5BF@2!K
M?!W#,%J'$%5"\,7:.\B*:%P;0#@)S^H(L"CB4L)ARS1D*:JH!$RQ:<;+M,CA
ML(Y/_X&E94>91"AJ7FNN2V)*Q!DB08$ANB839;'KNP0\\ZN)FILWH*`J1)@/
MCZA[SY7XFYBC+E?K_>[YU85!-I;N>J<?NY\_7YP;=&/][-/'FZN+S]WK2\/:
MW#BY/KNX0GE;\W1W`514_8DE^`=L432%Q'$)UC@?^:/I:I'7L17U&O07LK7N
M"UMLW0WN-82EH:,RN,IE.LJ1,EF1CPQCF+&1[.C$QHP6O+QG6<<\QVJNV*&'
MP)ROPQ$0%)73-.^K3.^GXD^)B;6_ATXUWJNE=Z`-'BBQAU2]>/;UGL9Z`/^8
M1L3D*OW::]^K',-58Z6%IVZD)IHV!E@SQIUUY7D>*M6ELXV+T][=EYWR2VNH
[email protected],JJQ4JJ>]R_V#=85_YW"1I'/`@LL2[V$GYJ\Z4I7(8>[#[XM)&XHIXJD&
MD[3LHZ8.RSN8\$E_H;R3/[HO*O[\S-;\70)M=&-BX<2E,XIM,M`$"OPG!/)?
M]%[TH.'\2@*IY\+W^*.QO8H_6$F0$T]+N&%$4Z0,I0Y%@J&<8RFBM0Y-`_O*
M.8]45WI(6`FI5,4;6T.,T8$!4YT+"[O&H0TX>(`:/46^>K2^SZ3U9)YG+JC>
M5$PF:%V+)<64#ZLL>]1=Z*6<6["JLYM5Z7#NTZ+)UKD,>UKYY.[+]5F_=WW5
MO?YP@-+&J$#P"KG2Y9GDRT75[_K8[_KS%M=YKDPLF:X"I)OR4P)WJ45\")"0
MVZ;;JM3ATNKZ]O>BJ>*\1K+6C!'JGKK8]0(`^WN8/_V8E4Q7P(/.M^H#["Z*
M2W>>GK]1.10$FX3@K)7;-8M*?AG]^8UME<D#T"^"^OK+A*_>83!D:58_1W9<
M^3(C-J%M'*Y,+UXCZG6!F'5ETP\2?+X3\%_L]V_;?N_R2B6!RAH\5V)-&13Q
M(TS24:+?62JF"A;F>E7#6AZQ=IW*[CSH_.GF,V!7_\Y'"8_&LIH<N3P(!B$6
+F_\`&<RK;CT0````
`
end
-------------------------------------------------------
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
