Oh...Thanks for the comment. I'll resend this patch. Thanks & Best regards, Andiry > -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Wednesday, July 21, 2010 2:42 AM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx > Subject: Re: [PATCH 0/10 v8] xHCI: Isoc transfer implementation > > On Tue, Jul 20, 2010 at 04:49:08PM +0800, Andiry Xu wrote: > > Hi Sarah, > > > > This is xHCI isoc patchset v8. > > > > Changelog from v7: > > > > 1. Leave urb->status intact during host controller process. > > 2. Add the allocate bigger ring for isoc endpoint patch. > > 3. Set isoc urb status to -EXDEV when missed service event occurs. > > 4. Do not set isoc urb packet status when COMP_STOP or COMP_STOP_INVAL > > happens. Group COMP_BUFF_OVER and COMP_BABBLE case to remove redundancy. > > Your change log says that the urb packet status isn't set when COMP_STOP > or COMP_STOP_INVAL is encounted, but your code doesn't match that > statement: > > On Tue, Jul 20, 2010 at 04:49:30PM +0800, Andiry Xu wrote: > > +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) > > +{ > > ... > > > + /* 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; > > + default: > > + td->urb->iso_frame_desc[idx].status = -1; > > + break; > > + } > > + } > > If the TRB completion code is set to COMP_STOP or COMP_STOP_INVAL, the > packet's status will get set to -1, because of the default case in the > switch statement. You just need to add cases for those two with a break > statement: > > 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; > } > > Other than that, the whole patchset looks fine. Perhaps you want to > resend just this patch? > > 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
