On 2012-04-17 15:58, Michael Grzeschik wrote:
> 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.
I wanted to avoid code duplication, so I tried to have only one code path where this was
done. Of course, when I wrote this I thought it would be possible to "abort" a transfer
that was already underway inside the isp1760, so that dequeued urbs could be cleanly
collected by the interrupt routine.
But you still need to handle (free etc) completed and retired qtds in the interrupt
routine anyway, so the point about avoiding code duplication still stands.
> 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;
>
All qtds belonging to a certain urb lie next to each other in the list, so there would not
be any point in continuing the search here. (I you find one qtd that doesn't belong to the
urb, you know that there cannot be any more qtds in the queue that belongs to the urb.)
--
Arvid Brodin
Enea Services Stockholm AB - since February 16 a part of Xdin in the Alten Group. Soon we
will be working under the common brand name Xdin. Read more at www.xdin.com.--
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
