Ok, there are some fundamental issues with the design of this patchset.
You are basically wanting to block usb_submit_urb() if the ring is full
and the link TRB is given over to the hardware, until some more URBs
complete on the ring. You use one completion to wait for that to
happen:
> + xhci_dbg(xhci, "Waiting for dequeue pointer pass "
> + "link TRB\n");
> + ep_ring->notify_link = 1;
> + init_completion(&ep_ring->pass_link);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> +
> + /* Wait for the dequeue pointer pass link TRB */
> + timeleft = wait_for_completion_interruptible_timeout(
> + &ep_ring->pass_link,
> + USB_CTRL_SET_TIMEOUT);
> + if (timeleft <= 0) {
> + xhci_warn(xhci, "%s while waiting for passing "
> + "link TRB\n", timeleft == 0 ?
> + "Timeout" : "Signal");
> + spin_lock_irqsave(&xhci->lock, flags);
> + return -ETIME;
> + }
> + spin_lock_irqsave(&xhci->lock, flags);
Then, later, when the link TRB is walked past in inc_deq, the completion
is signaled and ring->notify_link is cleared:
> @@ -169,6 +169,14 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer
> ring->deq_seg = ring->deq_seg->next;
> ring->dequeue = ring->deq_seg->trbs;
> next = ring->dequeue;
> + /*
> + * Complete pass_link if ring expansion is waiting for dequeue
> + * pointer pass link TRB.
> + */
> + if (ring->notify_link) {
> + complete(&ring->pass_link);
> + ring->notify_link = 0;
> + }
But what happens if the URB completion function resubmits the URB that
just completed before you pass the link TRB? You have one URB waiting
on the completion, and then xhci_submit_urb() is called again for a
different URB. Say neither URB can fit on the ring, and then you have
two URBs waiting on the completion.
Then the link TRB is finally passed, and ring->notify_link is cleared.
Only one thread will get woken up when complete() is called. (I can't
remember if the first thread will get woken up, or if it's random which
thread is woken up.) The woken thread will expand the ring, queue its
TRBs, and continue on its merry way. But the second sleeping thread
will never be woken up, because ring->notify_link was cleared by
inc_deq, and the completion will never be signaled for the sleeping
thread.
I think what you need instead is an xHCI software queue of URBs waiting
to be submitted when there's room on the ring. It should be a FIFO, to
preserve the rule that URBs submitted to the same endpoint are queued in
the order that usb_submit_urb() is called. Because of that rule, if
xhci_submit_urb() is called, and there are URBs in the queue, we should
just add the new URB to the back of the queue and immediately return
success. If there's nothing in the queue, but there's not enough room
on the ring, we should add the URB to the software queue and return
success from xhci_submit_urb().
Then if inc_deq() walks past a link TRB, it can use the queue to decide
whether it needs to expand the ring. If the queue is empty, it doesn't
need to expand the ring. If there are URBs in the queue, it can walk
the queue, count the number of TRBs each URB will need, and know how
many ring segments it needs to add. Then the URBs in the software
queue can be enqueued onto the ring and the endpoint ring will be rung.
Since the xHCI driver will return success in usb_submit_urb() when the
URB is added to the software queue, it will also need to handle URBs
being canceled while they are in that software queue. There will have
to be some work done in the cancellation code to check whether the URB
to be canceled is in the software queue, before stopping the endpoint.
It can just be removed from the software queue in that case. There will
also need to be some work done in the watchdog timer to complete URBs in
the software queue when the host controller has died.
This alternative solution can also make the rest of the patch a little
cleaner. If you're only expanding the ring in inc_deq(), you know that
is always called in interrupt context, and you don't have to add new
memory flags to prepare_transfer(), prepare_ring(),
xhci_queue_intr_tx(), queue_bulk_sg_tx(), xhci_queue_bulk_tx(),
xhci_queue_ctrl_tx(), xhci_queue_isoc_tx(), and
xhci_queue_isoc_tx_prepare().
What do you think?
Sarah Sharp
--
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
