> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, June 16, 2011 11:30 AM > To: linux-usb@xxxxxxxxxxxxxxx > Cc: Xu, Andiry; Alan Stern > Subject: [RFC] xhci: Always set urb->status to zero for isoc endpoints. > > When the xHCI driver encounters a Missed Service Interval event for an > isochronous endpoint ring, it means the host controller skipped over > one or more isochronous TDs. For TD that is skipped, skip_isoc_td() is > called. This sets the frame descriptor status to -EXDEV, and also sets > the value stored in the int pointed to by status to -EXDEV. > > If the isochronous TD happens to be the last TD in an URB, > handle_tx_event() will use the status variable to give back the URB to > the USB core. That means drivers will see urb->status as -EXDEV. > > It turns out that both EHCI and UHCI always set urb->status to zero for > an > isochronous urb, regardless of what the frame status is. See > itd_complete() in > ehci-sched.c: > > } else { > /* URB was too late */ > desc->status = -EXDEV; > } > } > > /* handle completion now? */ > if (likely ((urb_index + 1) != urb->number_of_packets)) > goto done; > > /* ASSERT: it's really the last itd for this urb > list_for_each_entry (itd, &stream->td_list, itd_list) > BUG_ON (itd->urb == urb); > */ > > /* give urb back to the driver; completion often (re)submits */ > dev = urb->dev; > ehci_urb_done(ehci, urb, 0); > > ehci_urb_done() completes the URB with the status of the third argument, > which > is always zero in this case. > > It turns out that many USB webcam drivers, such as uvcvideo, cannot > handle urb->status set to a non-zero value. They will not resubmit > their isochronous URBs in that case, and userspace will see a frozen > video. > > Change the xHCI driver to be consistent with the EHCI and UHCI driver, > and always set urb->status to 0 for isochronous URBs. > > Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> > --- > drivers/usb/host/xhci-ring.c | 17 ++++++++++++++--- > 1 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci- > ring.c > index 99668ba..17695a6 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1798,8 +1798,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. > + */ > frame->status = -EXDEV; > > /* calc actual length */ > @@ -2191,7 +2196,13 @@ cleanup: > urb->transfer_buffer_length, > status); > spin_unlock(&xhci->lock); > - usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, > status); > + /* EHCI and UHCI 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); > spin_lock(&xhci->lock); > } > You can remove the following lines in process_isoc_td(): if ((idx == urb_priv->length - 1) && *status == -EINPROGRESS) *status = 0; Since they're redundant if this patch is merged. 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
