Re: A question on EHCI unlink code

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


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


B and H Foto and Electronics Corp.

[Linux Media]     [Video for Linux]     [Linux Input]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]     [More Archives]

Add to Google Powered by Linux