On Thu, 6 Sep 2012, Pavan Kondeti wrote:
> Hi
>
> I am debugging "EHCI host system error" (4.15.2.4) issue. The issue
> happens during unlinking of URB from an interface driver. In our system
> the device is always connected to the host. some interfaces are always
> active (I/O can happen). Other interface's I/O depends on the user
> space. if user opens the device node, we submit IN URB. This issue
> happens when unlinking URB upon file close. This issue happens very rarely.
>
> I have a doubt in unlink path of EHCI driver. Say if an endpoint has two
> pending IN URB requests at the time of calling unlinking API.
> (usb_kill_anchored_urbs API).
>
> The QH's queue of this endpoint looks like this.
>
> Qtd1 --> Qtd2 --> Dummy
>
> EHCI driver kicks in the Async Advance Doorbell handshake after
> manipulating the QH horizontal pointer in start_unlink_async() function.
> EHCI driver wait for IAAD interrupt from the controller.
>
> Say controller is working on Qtd1. Which means transfer overlay of this
> QH has reference to Qtd2. controller generates IAAD interrupt (Qtd1 is
> not completely finished. QH active pointer points to Qtd1). EHCI driver
> finishes Qtd2 unlinking and points qtd1's next pointer to qtd'2 next
> pointer and puts this QH back to asynchronous schedule.
>
> My doubt is that software updated only QTD1 memory to point it to Qtd'2
> next pointer i.e dummy. What about the QH's transfer overlay memory?
> Would not the controller try to access the Qtd2 when QH is put back into
> schedule.
>
> To further clarify my question, I am copy pasting the qh_refresh() code
> here. In the below code, if we enter "first qtd may already be partially
> processed" case, should not we update qh->hw->hw_qtd_next and
> qh->hw->hw_alt_next to reflect the current Qtd memory.
>
> static void
> qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
> {
> struct ehci_qtd *qtd;
>
> if (list_empty (&qh->qtd_list))
> qtd = qh->dummy;
> else {
> qtd = list_entry (qh->qtd_list.next,
> struct ehci_qtd, qtd_list);
> /* first qtd may already be partially processed */
> if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
> qtd = NULL;
> }
>
> if (qtd)
> qh_update (ehci, qh, qtd);
> }
You're absolutely right; this is a bug in the driver. Would you like
to submit a patch to fix it?
(It shouldn't be necessary to update the hw_alt_next field. The
hw_alt_next part of the qTD doesn't get changed when the following qTD
is removed.)
Alan Stern
--
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
