More detailed reply below. On Wed, Jul 14, 2010 at 06:41:49PM +0800, Xu, Andiry wrote: > > -----Original Message----- > > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > > Sent: Wednesday, July 14, 2010 2:44 AM > > To: Xu, Andiry > > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx > > Subject: Re: [PATCH 0/9 v7] xHCI: Isoc transfer implementation > > > > On Fri, Jun 25, 2010 at 04:48:37PM +0800, Andiry Xu wrote: > > > Hi Sarah, > > > > > > This is xHCI isoc transfer implementation patchset v7. > > > > > > Changelog from v6: > > > 1. Break the xHCI: handle_tx_event() refine patch into six smaller > > > patches. > > > > > > Again, this patchset is based on revert-link-trb branch, > > > commit 5162708fc3c0. > > > > I assume you based your work off the revert-link-trb branch because > you > > ran into issues with overwriting the link TRBs when queueing > isochronous > > transfers. However, the patches on that branch are not going to be > > merged. They were just a quick test to see if the link TRB change was > > an issue for someone. > > > > The patches to fix the known bugs in the link TRB handoff change from > > John have been sent to Greg, and one has already been sent out by > Linus. > > I don't want to revert John's patch, as it's needed for streams > > communications to work properly. So you're going to have to add a > patch > > to work around the isochronous issues with John's patch. > > > > I think the patch will be similar to the control transfer fix (commit > > 6cc30d85a5bf61248ff0e1f0e0f15fe718bae378 in Linus' tree). Basically, > > you need to indicate to queue_trb() whether more TDs are coming if > > you're going to queue multiple isochronous TDs without calling > > prepare_transfer() in between each TD. > > > > Thanks for the information, I'll rebase the patchset on your master > branch. I've checked the control transfer fix, but I wonder what you > mean by saying that I do not to need to call prepare_transfer() for each > TD? As I can see, the prepare_transfer() do initialization for each TD, > add it to the ep's ring, which is necessary for each TD. Does the > control transfer fix affect prepare_transfer() calling? I don't quite > understand. If you wanted to, you could have written the code such that you only called prepare_transfer() once for each isochronous URB, with the total number of TRBs for all the packets in the isochronous URB. That would mean slightly different code in prepare_transfer() than what you have. In that case, you would be enqueueing multiple TDs without calling prepare_transfer() in between. John changed the code in inc_enq() to only move the enqueue pointer past the link TRB if the chain bit was set in the previous TRB. He wanted to not give a link TRB to the hardware unless another TD was at the top of the next ring segment. The code to move the enqueue pointer past the link TRB was in prepare_ring(), which is called from prepare_transfer(). The trouble is, if you try to enqueue multiple TDs without calling prepare_transfer() in between, then the link TRB would get overwritten. John's code falls apart for control transfers, because each control transfer is comprised of multiple TDs, and prepare_transfer() is only called once for the control URB. So if the setup or data TD of the control transfer ended just before the ring, and inc_enq() was called, it would notice the chain bit was cleared and not move the pointer past the link TRB. Then the link TRB would get overwritten. The fix I did for control transfers was to add a flag to inc_enq() that would tell it that the driver was going to enqueue another TD, and thus that function could move the enqueue pointer past the link TRB. If you were queueing multiple isoch TDs without calling prepare_transfer() in between, then you would need to do something similar. But you aren't, so you just need to pass false to inc_enq() for the more_tds_coming variable. I hope that clears things up, but please ask more questions if it doesn't. :) > > I've updated my master branch to be against 2.6.35-rc4, with some > > additional bug fixes that Greg has queued. Can you please base your > > next patch set against that? I'll send more comments on the isoc > > patchset later today so you can do the changes all at once. > > > > Thanks. I'll refine the patches and do another 24-hour stress test > before resubmit. > > I got another question. When I'm debugging the PM patches, I found that > when hub driver disable a USB3 protocol port and then reset the port, it > will transition to a USB2 protocol port. This will affect USB3 device > re-initialization during S3 resume. Is this normal behavior? If I do not > disable the USB3 protocol port, the USB3.0 device will be recognized > normally later. As I said, I have a patch in the master branch that might fix your issue, so please test that first. However, I also remember that Dong submitted a patch to turn off disabling of USB 3.0 ports. Greg asked him to resend it, but I never saw Dong resend it, and it doesn't seem to be in Greg's queue. Is that patch still needed? 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
