On Wed, Mar 03, 2010 at 10:54:33AM +0800, Andiry Xu wrote: > On Tue, 2010-03-02 at 14:06 -0800, Sarah Sharp wrote: > > On Tue, Mar 02, 2010 at 06:36:48PM +0800, Libin wrote: > > > >From 050df8768e10e58d01bbc24d8ee256d864cc6fff Mon Sep 17 00:00:00 2001 > > > From: Andiry Xu <andiry.xu@xxxxxxx> > > > Date: Fri, 26 Feb 2010 17:38:15 +0800 > > > Subject: [PATCH 2/7] xHCI: Isoc transfer URB enqueue > > > > > > This patch implements isoc transfer URB enqueue part. > > > > > > Split the isoc URB into isoc TDs and insert them to the isoc endpoint > > > transfer ring. A isoc TD can be made up of one isoc TRB and several normal > > > TRBs (if needed). > > > > Can you combine patches 2 and 3 into one patch? This is very confusing > > to review. > > > > Also, please add more description to your commit about what it means to > > have multiple TDs for one URB added to the endpoint list. Specifically, > > I want to know what your plan is for when an isochronous URB should be > > given back, and what happens to hcpriv, the isochronous TDs, and the > > endpoint's TD lists when an isochronous URB is canceled (dequeued) or > > the hardware dies. I can probably figure it out from the code, but I > > would rather have a statement of your intentions in case there is a bug > > in the code. :) > > > > Sarah Sharp > > > > OK, I'll merge patch 2 and 3. I split them because they have different functions. In general, it's probably better to group code by features, not by whether it touches different functions or files. So it's good that you split out patch 7, which is a bug fix, from your isochronous transfer support in the other patches. > I'll refine the patch description. The steps to insert isoc TDs into > isoc endpoint transfer ring are described in xHCI spec, I'll elaborate > them. > > I think the giveback and dequeue part are done in handle_tx_event or > after it, right? This patch only split isoc urb into isoc TDs and put > them on the transfer ring, then ring the ep doorbell. Do I need to take > care of the extreme situations in the enqueue patch? Well, patches 2, 3, 4, and 5 are really about adding one thing: isochronous support. Patch 1 really should be separate to show that adding the urb_priv structure doesn't effect the other types of transfers. Patch 6 should probably be a separate thing if it gets in at all. But splitting patches 2-5 up makes it a bit harder to review and spot bugs. Maybe you just want to combine patches 2-5? I do want to make sure some code in your patchset addresses the cancellation and hardware dying case. They're really not "extreme" situations. A driver could be unloaded immediately after submitting URBs and have to cancel them right away. As for the hardware dying code, it's used to handle PCI express card removal, which is pretty common if your card sits loose in the slot and you bump it occasionally. Here's my suggestion for the cancellation case: - Add all the TDs in an URB to the endpoint's cancellation list. - When processing a canceled TD when the endpoint ring is stopped, increment the new field in urb_priv for the number of TDs completed. - Only giveback a canceled URB if the count of TDs completed matches urb_priv->td_cnt. This should ensure that all the isochronous TDs are removed from the hardware ring, and the isochronous URB is only given back once. You can do a similar thing for the hardware dying case. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
