> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Friday, October 08, 2010 6:19 AM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx; Alan Stern; > Su@xxxxxxxxxxxxxx; Su, Henry; He@xxxxxxxxxxxxxx; He, Alex > Subject: Re: [PATCH 2/3 v2 RESEND] xHCI: change xhci_reset_device() to > allocate new device > > Andiry, > > I think there is a bug in this patch, but I'm not sure exactly what > you're doing with virt_dev->configured, so I can't suggest how to fix > it. > My original intention to add virt_dev->configured is this: according to the Slot State Diagram in xHCI spec, a successful reset device command will move the device to Default state. So we may got unmatch situation here: udev->config is set, but actually it's not configured. I add virt_dev->configured to indicate whether the device is truly configured or not. I see the original condition is !udev->config, so I think maybe if the device is actually not configured yet (although udev->config exists), it should also call xhci_setup_addressable_virt_dev(). If xhci_setup_addressable_virt_dev() does not re-initialize ep0's ring or set the ep0 ring dequeue pointer properly, then this issue may occur. Strangely I haven't triggered this issue. > The problem happens when the USB core tries to reset and re-address a > device that had been active and had some control transfers complete. An > example would be when the usb-storage driver tries to reset the device > after it gets an answer to a SCSI command that it doesn't like. > > With this patch, after a Device Reset command finishes, > virt_dev->configured is set to false. When the USB core tries to > address the device, xhci_address_device() will call > xhci_setup_addressable_virt_dev(). This initializes the slot and > endpoint 0 contexts to their defaults, including setting the control > endpoint ring dequeue pointer to the top of the ring. > > However, the control endpoint ring was used before, so the internal xHCI > driver internal dequeue pointer still points somewhere in the middle of > the ring. Worse, the hardware may go re-execute the control transfers > between the top of the ring and the internal dequeue pointer, because > the cycle bit says it owns those TRBs. Then the driver will see a bunch > of spurious events from the control endpoint ring the next time the > doorbell is rung. See the attached log for an example. > Yes, I've seen this issue before, that's before the commit xhci: Set EP0 dequeue ptr after reset of configured device (ec49fbb9ed72b7d20bf967170651abebe5e21ccf) exists. That fix resolves this issue. My solution was re-initializing ep0's ring at that time. But you submit the commit just when I wanted to submit the ep0's ring re-initialization patch. > In the case of a device reset when control transfers have already > occurred, I think you really want to call > xhci_copy_ep0_dequeue_into_input_ctx() in xhci_address_device(). > However, the code in xhci_address_device() won't do that, since > virt_dev->configured is always set to false after the Device Reset > command completes. > > Were you trying to use virt_dev->configured to set up the slot context > for an address device command after a resume where the xHCI host lost > power and the virt_dev had to be reallocated? Doesn't the place where > you set it to false in xhci_alloc_dev() cover that case? > > You could remove the statement where virt_dev->configured gets set to > false in after the Device Reset command completes, and that would fix > the problem with active devices being reset. But why make > xhci_address_device() just test some field in the slot context to see if > it's set properly, and call xhci_setup_addressable_virt_dev() if not? > Then you wouldn't need virt_dev->configured at all. You could check > that, say, slot_ctx->dev_info isn't zero (since the speed field in that > has to be set to some non-zero number). I think it's a good idea and it looks OK to me. I'll test the solution. 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
