> -----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. > I think something like the following is needed... > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 3e8211c..85ae5ef 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1918,22 +1918,26 @@ 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))) > { > + if (!event_seg) { > + if (ep->skip && > + usb_endpoint_xfer_isoc(&td->urb->ep->desc)) { > + ret = 0; > + goto cleanup; > + } By doing this you will skip the process_isoc_td calling. Actually it's really needed. > /* HC is busted, give up! */ > - xhci_err(xhci, "ERROR Transfer event TRB DMA ptr not " > - "part of current TD\n"); > + xhci_err(xhci, > + "ERROR Transfer event TRB DMA ptr not part of > current TD\n"); > return -ESHUTDOWN; > - } > + } 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)]; > /* > If you are just worry about a compile warning for an uninitialized variable, I think simply initialize event_trb with NULL will do. 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
