On Sun, Jun 30, 2013 at 2:35 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Sat, 29 Jun 2013, Ming Lei wrote: > >> > The ehci_endpoint_disable() routine can be improved. In the >> > QH_STATE_LINNKED or QH_STATE_COMPLETING case, we now need to handle >> > interrupt QHs -- the comment about "periodic qh self-unlinks on empty" >> > isn't entirely correct any more, because now the unlink doesn't occur >> > until the QH has been empty for 5 ms. >> >> Actually, almost all interrupt URBs are resubmitted in its completion handler, >> that means they are basically stopped by unlinking before disabling endpoint, >> so I am wondering if we need to consider improving ehci_endpoint_disable() >> on interrupt endpoint. > > Come to think of it, we never should see an endpoint being disabled > while there are active URBs. It might be a better idea to put a Right. > WARN_ON there and simply return, without unlinking anything. If this > ever triggers, it means there's a bug in usbcore. But that isn't always true, see below case: - the last URB in the endpoint completes without resubmitting in its completion handler - then the corresponding qh is kept in linked state, but wait for being unlinked - usb_disable_endpoint() is called before the qh is put into unlinked state, and no URBs are unlinked in usb_hcd_flush_endpoint() - ehci_endpoint_disable() still see qh in QH_STATE_LINKED state. So I think we should unlink here to speed up the procedure as suggested in your previous email. > >> But I suggest to do the above on in another patch because most of the >> change is not much related with this patch. > > Okay. > >> >> @@ -921,7 +966,7 @@ static void scan_intr(struct ehci_hcd *ehci) >> >> temp = qh_completions(ehci, qh); >> >> if (unlikely(temp || (list_empty(&qh->qtd_list) && >> >> qh->qh_state == QH_STATE_LINKED))) >> >> - start_unlink_intr(ehci, qh); >> >> + start_unlink_intr_wait(ehci, qh); >> > >> > This is not quite right. If temp is nonzero then we want to unlink the >> > QH right away (because a fault occurred), so we should call >> >> It might depend on the fault type, looks we need to unlink qh >> immediately on SHUTDOWN and qh->dequeue_during_giveback, and > > Yes. > >> for other non-unlink faults, drivers may not treat it as fault and continue >> to resubmit URB, such as hub_irq(). > > No. All faults have to cause the QH to be unlinked. That's another > invariant of the driver. This is partly related to the silicon quirk > present in some controllers (they perform the overlay even when the > Halt bit is set in the qTD). > > In any case, whenever the QH is halted, the only safe way to recover is > to unlink it and then relink. OK, got it, so we have to unlink the QH here as before to handle the fault. Thanks, -- Ming Lei -- 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
