On Wed, Aug 22, 2001, Pete Zaitcev <zaitcev@redhat.com> wrote:
> The usb-uhci and ohci worked fine, so I poked uhci a little bit
> and made it all work, at least with my setup. Apparently,
> sometimes uhci returns a success (0) from submit_urb, but
> forgets to deliver a completion callback.
>
> The fix is a Johannes' territory and I not so sure of my way there,
> so perhaps a better one may be forthcoming.
Forgets may or may not be true. Success (0) from submit_urb only means
the URB was scheduled.
You may not receive a completion callback if the device NAK's the
transfer forever, or if there is a bug in uhci.c where the URB didn't
really get scheduled.
What was you test scenario where you didn't see a completion callback?
> diff -ur -X dontdiff linux-2.4.8-ac9/drivers/usb/uhci.c linux-2.4.8-ac9-niph/drivers/usb/uhci.c
> --- linux-2.4.8-ac9/drivers/usb/uhci.c Wed Aug 22 11:01:57 2001
> +++ linux-2.4.8-ac9-niph/drivers/usb/uhci.c Wed Aug 22 13:25:00 2001
> @@ -1471,6 +1471,7 @@
>
> static int uhci_submit_urb(struct urb *urb)
> {
> + struct usb_device *dev;
> int ret = -EINVAL;
> struct uhci *uhci;
> unsigned long flags;
> @@ -1480,15 +1481,16 @@
> if (!urb)
> return -EINVAL;
>
> - if (!urb->dev || !urb->dev->bus || !urb->dev->bus->hcpriv) {
> + dev = urb->dev;
> + if (!dev || !dev->bus || !dev->bus->hcpriv) {
> warn("uhci_submit_urb: urb %p belongs to disconnected device or bus?", urb);
> return -ENODEV;
> }
>
> - uhci = (struct uhci *)urb->dev->bus->hcpriv;
> + uhci = (struct uhci *)dev->bus->hcpriv;
>
> INIT_LIST_HEAD(&urb->urb_list);
> - usb_inc_dev_use(urb->dev);
> + usb_inc_dev_use(dev);
>
> spin_lock_irqsave(&urb->lock, flags);
>
> @@ -1497,7 +1499,7 @@
> dbg("uhci_submit_urb: urb not available to submit (status = %d)", urb->status);
> /* Since we can have problems on the out path */
> spin_unlock_irqrestore(&urb->lock, flags);
> - usb_dec_dev_use(urb->dev);
> + usb_dec_dev_use(dev);
>
> return ret;
> }
> @@ -1516,7 +1518,7 @@
> }
>
> /* Short circuit the virtual root hub */
> - if (urb->dev == uhci->rh.dev) {
> + if (dev == uhci->rh.dev) {
> ret = rh_submit_urb(urb);
>
> goto out;
> @@ -1528,13 +1530,13 @@
> break;
> case PIPE_INTERRUPT:
> if (urb->bandwidth == 0) { /* not yet checked/allocated */
> - bustime = usb_check_bandwidth(urb->dev, urb);
> + bustime = usb_check_bandwidth(dev, urb);
> if (bustime < 0)
> ret = bustime;
> else {
> ret = uhci_submit_interrupt(urb);
> if (ret == -EINPROGRESS)
> - usb_claim_bandwidth(urb->dev, urb, bustime, 0);
> + usb_claim_bandwidth(dev, urb, bustime, 0);
> }
> } else /* bandwidth is already set */
> ret = uhci_submit_interrupt(urb);
> @@ -1548,7 +1550,7 @@
> ret = -EINVAL;
> break;
> }
> - bustime = usb_check_bandwidth(urb->dev, urb);
> + bustime = usb_check_bandwidth(dev, urb);
> if (bustime < 0) {
> ret = bustime;
> break;
> @@ -1556,7 +1558,7 @@
>
> ret = uhci_submit_isochronous(urb);
> if (ret == -EINPROGRESS)
> - usb_claim_bandwidth(urb->dev, urb, bustime, 1);
> + usb_claim_bandwidth(dev, urb, bustime, 1);
> } else /* bandwidth is already set */
> ret = uhci_submit_isochronous(urb);
> break;
> @@ -1578,8 +1580,14 @@
>
> uhci_unlink_generic(uhci, urb);
> uhci_destroy_urb_priv(urb);
> + if (ret == 0) { /* N.B. Done, must notify */
> + /* uhci_call_completion(urb); */ /* ->> uhci_destroy_urb_priv */
> + urb->dev = NULL;
> + if (urb->complete)
> + urb->complete(urb);
> + }
>
> - usb_dec_dev_use(urb->dev);
> + usb_dec_dev_use(dev);
>
> return ret;
> }
What's all of this for? Protecting against an urb->dev race condition?
JE
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri Aug 31 2001 - 21:00:11 EST