On Wednesday 13 June 2007, Alan Stern wrote: > Dave and everyone else: > > The meaning of the return value from usb_unlink_urb() is not as clear > as it should be. The value is used in only a handful of drivers, > mostly for debugging output. David's code uses it for warnings and > error messages, but not for altering the control flow.
It's unclear what should be done, other than print diagnostics. > The real question is this: > > Under what conditions does it make sense to say that > usb_unlink_urb() has failed? If it fails. ;) > Since the call races against URB completion, I think it never really > makes sense. Here's a more detailed examination of the possibilities. > Consider what happens when an URB is submitted: > > 0. Initially the URB is unlinked and idle. > > 1. The URB gets submitted and linked. > > 2. The URB is passed to the HCD, which enqueues it. > > 3. The URB completes and is dequeued by the HCD. > > 4. The URB is unlinked and passed to the completion routine. > > 5. The URB is once again idle (or it may be resubmitted). That's the typical flow, yep. No unlink/kill/etc. The only difference between #4 and #5 is of course a refcount; the URB becomes idle at some point on its way into the completion routine, at which point it can be resubmitted. The refcount is kept (starting at #1) to ensure the URB is not prematurely freed. > Now consider what happens if usb_unlink_urb() is called just after each > step above: The parenthesis indicating the unlink() result... > 0. Nothing happens since the URB is idle. (Error) Since you say the after-#5 is the same as this, I'd not call it an error. It's a fault, sure ... but any "error" comes from a caller doing something that's defined to not succeed. When code does what it's defined to do, that's not an error. :) > 1. This is the tricky part. We want the submission to fail > but it hasn't finished yet. The URB is marked so that the > HCD will recognize it has already been unlinked, and the > enqueue call will fail. (Error?) Either of two outcomes could work, preserving the invariant that URBs are submitted exactly once and then completed either by normal I/O completion or else by unlink: - Submit succeeds, unlink does too (returning -ECONNRESET or whatever); - Neither call succeeds ... submit is aborted (though probably for puzzling reason), unlink does since this becomes case #0. The first of these makes a lot more sense to me, at this point, since nothing another thread does "should" make the submit fail unless the endpoint vanishes. You seem to be assuming the other path, which I'd normally file under "very surprising behavior from a programming interface". Now, ISTR that when implementing this the first time, there was something preventing the sanest behavior here. I forget the details, but I know the relevant interfaces have since been at least partially fixed up. > 2. The HCD's dequeue routine works in the usual way. (Success) Standard behavior. > 3. The HCD's dequeue routine fails since the URB has already > been dequeued. (Error?) The unlink shouldn't succeed, since the URB has completed in every sense except the trivial one of finishing giveback(). Note that you're combining "completes and dequeues" into one step, which doesn't really match my understanding. The two are can not be coupled atomic operations!! First completion happens (URB status becomes known, and is recorded). At some later point -- probably after recycling at least one TD, and other cleanup of HCD state including internal queues -- the URB starts its journey back to the driver. As soon as the status became known, the HCD has committed to returning the URB with that status, and the core "accelerate completion" of unlink() could no longer kick in. That "real I/O status" should never be replaced by a false "unlinked" status code. And before that hardware status is detected, we're still in the "after step #2" case. It's unfortunate that sometimes that means discarding useful hardware status reports... since the unlink() status would override hardware status which might not yet have been collected. (Because for example maybe the relevant IRQ was being held off by locking during the unlink.) > 4. The unlink call fails since the URB is no longer linked. > (Error?) As above for #3, except that the urb got further into its journey back to the driver. > 5. Same as 0. Sounds right. > 0 is clearly an error and 2 is clearly a success. The others aren't so > clear. I disagree. For #3 and #4 it's very clear: the URB has started to complete normally. Since "unlink" is nothing more than a way to kick in a software fault to accelerate the URB completion, the unlink can't succeed: there is nothing to accelerate, it's already going back and with some "real" status! And as you say, #5 is #0... leaving only #1 as potentially confusing. (And that one hardware race as an annoying #2 subcase, where hardware status must get discarded.) > In particular, in 1 can we say that the URB was unlinked when > strictly speaking it never got fully submitted to begin with? If the submit() succeeds, then there are only two ways for that URB to complete: (a) normally due to I/O completion, fault or normal; and (b) unlink, which is a fault triggered by the software. As I noted above, letting the unlink() prevent the submit() from succeeding seems like a pretty dubious model. But it might have become less surprising due to the way kill() can now change the URB lifecycle. > But if > the unlink call hadn't been made then the submission would have > succeeded! Part of the confusion results from the somewhat artificial > difference between being linked and being enqueued. When I think at the USB driver level, there's only submitted(). Those other transitions are internal, and should not be exposed to the drivers. > IMO the fine distinctions above simply don't matter. Drivers don't > care anyway, since they can't know what stage the URB is at when they > try to unlink it. Just what I said. ;) > The only possible place where this might matter is in usbtest, which > specifically intends to test the unlink code paths. Even there, > I'm not sure that usb_unlink_urb needs to return a meaningful value; > the URB's final status code ought to be sufficient. Yes, we need to test those paths to make sure they don't break or oops. They used to do both. ;) > So -- is there any objection if I change usb_unlink_urb to return void? The main reason to care is that drivers may need to know how the URB will be completing ... since how (and whether) they clean up will be affected. That's what usbtest does, for example ... when it "loses" either of the internal races (before HW gets the URB, or after I/O has finished but before it's been handed back to the driver) it acts differently ... the test can't terminate until a real unlink happens (and shouldn't). It never triggers cases #0/#5; the case #2 is how the test exits. > If there is, then consider that steps 0, 1, 3, and 4 ought to return > distinct error codes, which they currently don't do. (Or maybe 1 > should count as a success?) In practice this would just make more work > for drivers, since they would have to test more possible codes. Well, surely #0/#5 should return the same fault code. Those cases seem unlike the others, more of a "hey driver writer, you screwed up" indicator. And since #3 and #4 are essentially the same as each other, they should return the same code too. These are the cases where the driver didn't screw up; just another minor race (it's an async interface after all, races inherent), not any kind of screwup. If we assume that submit succeeds on #1, so that unlink() can succeed, then it behaves like #2. (I find the other option for #1 confusing, so I won't address it.) And #2 is a standard "unlink succeeded" outcome. That makes three outcomes needed by async unlink: error because of driver screwup; success, accelerated completion coming any moment now; fault due to race lost, normal completion coming any moment now. - Dave ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel