On Mon, Oct 11, 2010 at 07:21:36PM +0800, Xu, Andiry wrote: > > -----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. Yes, I think I only triggered it because my USB 3.0 hard drive started stalling SCSI commands after the resume from hibernate. The USB storage driver decided to reset it. > > 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. Ok, I'll test the device suspend and resume case today. Alan Stern added patches for USB storage autosuspend, so I should be able to test some USB 3.0 hard drives with it. 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
