On Thu, Jun 16, 2011 at 04:45:12PM -0400, Alan Stern wrote: > On Thu, 16 Jun 2011, Sarah Sharp wrote: > > > Change the xHCI driver to be consistent with the EHCI and UHCI driver, > > and always set urb->status to 0 for isochronous URBs. > > > @@ -1798,8 +1795,13 @@ static int skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td, > > idx = urb_priv->td_cnt; > > frame = &td->urb->iso_frame_desc[idx]; > > > > - /* The transfer is partly done */ > > - *status = -EXDEV; > > + /* The transfer is partly done. > > + * > > + * Do not set status to -EXDEV. If we are on the last isoc TD of an > > + * URB, urb->status will be set to -EXDEV. Both UHCI and EHCI return > > + * zero for urb->status, even if they set -EXDEV in the status of some > > + * iso_frame_desc. > > + */ > > This comment says more than is necessary, and it makes the driver sound > as though it has an inferiority complex. :-) Sorry, I must have had my fake snooty nose and monocle on. :) > Just say something simple, like: Isochronous URBs report status in the > individual iso_frame_desc elements, not in urb->status. Or I could just say nothing at all. > > frame->status = -EXDEV; > > > > /* calc actual length */ > > @@ -2191,7 +2193,13 @@ cleanup: > > urb->transfer_buffer_length, > > status); > > spin_unlock(&xhci->lock); > > - usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, status); > > + /* EHCI, UHCI, and OHCI always unconditionally set the > > + * urb->status of an isochronous endpoint to 0. > > + */ > > + if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) > > + usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, 0); > > + else > > + usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, status); > > Is this part needed at all? If it is, you can do it like this: > > + if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) > + status = 0; > usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, status); Yes, that bit is needed. There's a switch statement in the beginning of xhci_handle_tx_event that sets status based on TRB completion code, regardless of what type of endpoint it is. For example, if there's a transfer error, status gets set to -EPROTO. The overrun and underrun completion codes for isochronous endpoints also get checked there, so I can't simply skip it without refactoring a bunch of code. I'd rather do the refactoring later, so that this patch can be easily backported to the stable trees. > But it would be even better to set status to 0 somewhere in the code > that's specifically responsible for handling isochronous URBs, instead > of adding an extra test here. Yeah, I really really hate this code and want to rip it out and refactor it into something better, but not today. 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
