On Thu, Feb 24, 2011 at 02:07:27PM +0800, Xu, Andiry wrote: > > -----Original Message----- > > From: Dmitry Torokhov [mailto:dtor@xxxxxxxxxx] > > Sent: Thursday, February 24, 2011 12:48 PM > > To: Xu, Andiry; Sarah Sharp > > Cc: linux-usb@xxxxxxxxxxxxxxx > > Subject: Potential issue in xhci-ring handle_tx_event() > > > > Hi Andiry, Sarah, > > > > I was compiling xhci and got a warning that event_trb might be used > > uninitialized in handle_tx_event(). Looking at the implementation the > > logic is quite convoluted but appears that if the following condition > is > > true: > > > > !event_seg && ep->skip && usb_endpoint_xfer_isoc(...) > > > > we indeed will end up calling process_*_td(..., event_trb, ...). > > > > Well, if the condition is met, it must be an isoc endpoint and > process_isoc_td() will be called with ep->skip set. You can follow the > call trace and see that event_trb will never be used in that case. > That's a normal case for isoc transfer. Sometimes the xHC is unable to > process all the isoc tds in time and it will miss some tds. The driver > will never find the corresponding event_trb for these tds in this case. Ah, I completely missed the fact that event_trb is not always used by process_isoc_td(). In this case do you think the following patch clears up logic a bit: diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 3e8211c..8336de6 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1565,71 +1565,66 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td, union xhci_trb *event_trb, struct xhci_transfer_event *event, struct xhci_virt_ep *ep, int *status) { - struct xhci_ring *ep_ring; - struct urb_priv *urb_priv; - int idx; + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); + struct urb_priv *urb_priv = td->urb->hcpriv; + int idx = urb_priv->td_cnt; int len = 0; - int skip_td = 0; union xhci_trb *cur_trb; struct xhci_segment *cur_seg; - u32 trb_comp_code; - - ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); - trb_comp_code = GET_COMP_CODE(event->transfer_len); - urb_priv = td->urb->hcpriv; - idx = urb_priv->td_cnt; + u32 trb_comp_code = GET_COMP_CODE(event->transfer_len); + bool skip_td = false; if (ep->skip) { /* The transfer is partly done */ *status = -EXDEV; td->urb->iso_frame_desc[idx].status = -EXDEV; - } else { - /* handle completion code */ - switch (trb_comp_code) { - case COMP_SUCCESS: - td->urb->iso_frame_desc[idx].status = 0; - xhci_dbg(xhci, "Successful isoc transfer!\n"); - break; - case COMP_SHORT_TX: - if (td->urb->transfer_flags & URB_SHORT_NOT_OK) - td->urb->iso_frame_desc[idx].status = - -EREMOTEIO; - else - td->urb->iso_frame_desc[idx].status = 0; - break; - case COMP_BW_OVER: - td->urb->iso_frame_desc[idx].status = -ECOMM; - skip_td = 1; - break; - case COMP_BUFF_OVER: - case COMP_BABBLE: - td->urb->iso_frame_desc[idx].status = -EOVERFLOW; - skip_td = 1; - break; - case COMP_STALL: - td->urb->iso_frame_desc[idx].status = -EPROTO; - skip_td = 1; - break; - case COMP_STOP: - case COMP_STOP_INVAL: - break; - default: - td->urb->iso_frame_desc[idx].status = -1; - break; - } - } - /* calc actual length */ - if (ep->skip) { + /* calc actual length */ td->urb->iso_frame_desc[idx].actual_length = 0; + /* Update ring dequeue pointer */ while (ep_ring->dequeue != td->last_trb) inc_deq(xhci, ep_ring, false); inc_deq(xhci, ep_ring, false); + return finish_td(xhci, td, event_trb, event, ep, status, true); } - if (trb_comp_code == COMP_SUCCESS || skip_td == 1) { + /* handle completion code */ + switch (trb_comp_code) { + case COMP_SUCCESS: + td->urb->iso_frame_desc[idx].status = 0; + xhci_dbg(xhci, "Successful isoc transfer!\n"); + break; + case COMP_SHORT_TX: + if (td->urb->transfer_flags & URB_SHORT_NOT_OK) + td->urb->iso_frame_desc[idx].status = + -EREMOTEIO; + else + td->urb->iso_frame_desc[idx].status = 0; + break; + case COMP_BW_OVER: + td->urb->iso_frame_desc[idx].status = -ECOMM; + skip_td = true; + break; + case COMP_BUFF_OVER: + case COMP_BABBLE: + td->urb->iso_frame_desc[idx].status = -EOVERFLOW; + skip_td = true; + break; + case COMP_STALL: + td->urb->iso_frame_desc[idx].status = -EPROTO; + skip_td = true; + break; + case COMP_STOP: + case COMP_STOP_INVAL: + break; + default: + td->urb->iso_frame_desc[idx].status = -1; + break; + } + + if (trb_comp_code == COMP_SUCCESS || skip_td) { td->urb->iso_frame_desc[idx].actual_length = td->urb->iso_frame_desc[idx].length; td->urb->actual_length += @@ -1667,13 +1662,10 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_td *td, union xhci_trb *event_trb, struct xhci_transfer_event *event, struct xhci_virt_ep *ep, int *status) { - struct xhci_ring *ep_ring; + struct xhci_ring *ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); union xhci_trb *cur_trb; struct xhci_segment *cur_seg; - u32 trb_comp_code; - - ep_ring = xhci_dma_to_transfer_ring(ep, event->buffer); - trb_comp_code = GET_COMP_CODE(event->transfer_len); + u32 trb_comp_code = GET_COMP_CODE(event->transfer_len); switch (trb_comp_code) { case COMP_SUCCESS: @@ -1918,22 +1910,27 @@ static int handle_tx_event(struct xhci_hcd *xhci, } td = list_entry(ep_ring->td_list.next, struct xhci_td, td_list); + /* Is this a TRB in the currently executing TD? */ event_seg = trb_in_td(ep_ring->deq_seg, ep_ring->dequeue, td->last_trb, event_dma); - if (event_seg && ep->skip) { - xhci_dbg(xhci, "Found td. Clear skip flag.\n"); - ep->skip = false; - } - if (!event_seg && - (!ep->skip || !usb_endpoint_xfer_isoc(&td->urb->ep->desc))) { - /* HC is busted, give up! */ - xhci_err(xhci, "ERROR Transfer event TRB DMA ptr not " + if (!event_seg) { + if (!ep->skip || + !usb_endpoint_xfer_isoc(&td->urb->ep->desc)) { + /* HC is busted, give up! */ + xhci_err(xhci, + "ERROR Transfer event TRB DMA ptr not " "part of current TD\n"); - return -ESHUTDOWN; - } + return -ESHUTDOWN; + } + /* When skipping event_trb is not used */ + event_trb = NULL; + } else { + if (ep->skip) { + xhci_dbg(xhci, "Found td. Clear skip flag.\n"); + ep->skip = false; + } - if (event_seg) { event_trb = &event_seg->trbs[(event_dma - event_seg->dma) / sizeof(*event_trb)]; /* Thanks. -- Dmitry -- 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
