> -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Sarah Sharp > Sent: Tuesday, October 25, 2011 4:27 PM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [RFC v2 6/7] xHCI: wait for dequeue pointer move pass link TRB > > 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. > Oh, yes, you are right. Thanks for remind me this. > > 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? > Yes, I think this is feasible, I'll try this and see if I've more questions during the implementation. Thanks for the review and explanation, it's really appreciated. Thanks, Andiry -- 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
