> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, July 15, 2010 11:45 PM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx > Subject: Re: [PATCH 0/9 v7] xHCI: Isoc transfer implementation > > 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. :) > Thanks for the explanation! Now it's pretty clear. I think I will just call prepare_transfer() for each td and pass false to inc_enq() for the more_tds_coming variable. > > > 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? > I've checked the patch in the master branch. The patch fixed a software issue after the port reset. Actually the port is still a USB3 port, but the driver falsely detects it. But my issue is to disable a USB3 port, then reset it. In this case the H/W will transition the port to a USB2 port, which is indicated in the PORTSC register. The only way is to skip the disabling. I've not checked the patch you mentioned but I think it may be needed. Thanks, Andiry -- 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
