On Mon, Apr 16, 2012 at 04:54:32PM +0000, Arvid Brodin wrote:
> On 2012-04-12 19:51, Michael Grzeschik wrote:
> > Hi Arvid,
> >
> > On Tue, Apr 10, 2012 at 11:41:24PM +0000, Arvid Brodin wrote:
> >> On 2012-04-05 14:56, Michael Grzeschik wrote:
> >>> Its possible that there are RETIRED qtds in the
> >>> the qtd_list, which have been dequeued. Make sure
> >>> to iterate through the whole list.
> >>>
> >>> This for instance fixes hanging serial devices, which wait
> >>> for dequeued urbs to properly close their device node.
> >>>
> >>> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/usb/host/isp1760-hcd.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
> >>> index 6599616..6e0bd48 100644
> >>> --- a/drivers/usb/host/isp1760-hcd.c
> >>> +++ b/drivers/usb/host/isp1760-hcd.c
> >>> @@ -800,7 +800,7 @@ static void collect_qtds(struct usb_hcd *hcd, struct isp1760_qh *qh,
> >>>
> >>> list_for_each_entry_safe(qtd, qtd_next, &qh->qtd_list, qtd_list) {
> >>> if (qtd->status < QTD_XFER_COMPLETE)
> >>> - break;
> >>> + continue;
> >>>
> >>> last_qtd = last_qtd_of_urb(qtd, qh);
> >>>
> >>
> >> Ok, so this happens when
> >>
> >> 1) several urbs are queued to an endpoint
> >> 2) an urb (not the first in the list) is dequeued
> >> and
> >> 3) the device for some reason won't handle/finish the earlier urbs?
> >
> > Yes, to all three points.
> >
> > Especially point three is tricky. The caller [1] of usb_kill_urb
> > triggers a wait_event, which will hang until somebody calls
> > isp1760_urb_done.
> >
> > This will only happen when we collect all of the enqueud QTDs
> > and look for QTDs which have the status QTD_RETIRE.
> >
> > What is the condition to finish the earlier urb in the queue of an
> > endpoint? Has the collect_qtds to be in a serial order?
> >
> > This situation happens reproducible when i try to establish a pppd
> > connection with my sierra serial usb modem which then does not get a
> > link. The pppd afterwards hangs while trying to close the ttyUSB
> > devicenode.
> >
> > [1] In my special case its: sierra_stop_rx_urbs called by sierra_close.
> >
> > Cheers,
> > Michael
> >
>
> I've been thinking about this some more. The reason I don't really like this patch is that
> this loop is nested and is run in interrupt context holding a spinlock. The idea here is
> to check only the head of the queue and handle only the qtds that are ready to be handled,
> instead of interating though the whole queue on every interrupt.
>
> What about handling this on urb dequeueing instead, by moving the qtds belonging to the
> dequeued urb to the front of the queue?
That sounds worth trying. I am currently trying to implement a solution
based on that thought. With this i wonder why we don't just call the
isp1760_urb_done from inside urb_dequeue. Isn't it then also possible
to do the freem_mem, list_del and qtd_free in the same call.
By the way; could the break here not also be a continue instead. Especially
with the comment above in mind.
diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index 37f017b..2e117c4 100644
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -1629,7 +1629,7 @@ static void dequeue_urb_from_qtd(struct usb_hcd *hcd, struct isp1760_qh *qh,
urb_was_running = 0;
list_for_each_entry_from(qtd, &qh->qtd_list, qtd_list) {
if (qtd->urb != urb)
- break;
+ continue;
if (qtd->status >= QTD_XFER_STARTED)
urb_was_running = 1;
Regards,
Michael
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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
