> -----Original Message----- > From: Dmitry Torokhov [mailto:dtor@xxxxxxxxxx] > Sent: Thursday, February 24, 2011 2:44 PM > To: Xu, Andiry > Cc: Sarah Sharp; linux-usb@xxxxxxxxxxxxxxx > Subject: Re: Potential issue in xhci-ring handle_tx_event() > > 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)]; > /* > It looks OK to me. 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
