RE: [PATCH 2/3 v2 RESEND] xHCI: change xhci_reset_device() to allocate new device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux