Hi Johannes and list,
I was still seeing very slow scanning from 2.4.18-pre9 and and Epson.
I was wondering whether all the fixes to the 2.5 uhci.c settle this
issue? (Still can't quite get 2.5.5 vanilla to compile yet... :-((()
Cheers!
Richard Chan
On Wed, 2002-02-20 at 14:23, Johannes Erdfelt wrote:
> uhci.c didn't work well with USB storage. It would tend to stall
> relatively quickly and sometimes locked up the system. It usually only
> took me a couple of tries ripping a CD to reproduce the problem.
>
> I took a long hard look at the locking in uhci.c and decided to clean
> it up, fixing a couple of bugs along the way as well as documenting the
> locking strategy.
>
> With this patch applies, where I could only rip a CD a couple of times
> before causing problems, I was able to rip a CD 12,000 times in a row
> successfully, before I stopped it. Not a single error :)
>
> This is the last patch for today.
>
> JE
>
> diff -ur linux-2.4.18-pre9/drivers/usb/uhci.c linux-2.4.18-pre9.je/drivers/usb/uhci.c
> --- linux-2.4.18-pre9/drivers/usb/uhci.c Tue Feb 19 22:10:21 2002
> +++ linux-2.4.18-pre9.je/drivers/usb/uhci.c Tue Feb 19 21:59:14 2002
> @@ -4,7 +4,7 @@
> * Maintainer: Johannes Erdfelt <[EMAIL PROTECTED]>
> *
> * (C) Copyright 1999 Linus Torvalds
> - * (C) Copyright 1999-2001 Johannes Erdfelt, [EMAIL PROTECTED]
> + * (C) Copyright 1999-2002 Johannes Erdfelt, [EMAIL PROTECTED]
> * (C) Copyright 1999 Randy Dunlap
> * (C) Copyright 1999 Georg Acher, [EMAIL PROTECTED]
> * (C) Copyright 1999 Deti Fliegl, [EMAIL PROTECTED]
> @@ -240,10 +240,10 @@
> unsigned long flags;
>
> /* If it's not inserted, don't remove it */
> + spin_lock_irqsave(&uhci->frame_list_lock, flags);
> if (td->frame == -1 && list_empty(&td->fl_list))
> - return;
> + goto out;
>
> - spin_lock_irqsave(&uhci->frame_list_lock, flags);
> if (td->frame != -1 && uhci->fl->frame_cpu[td->frame] == td) {
> if (list_empty(&td->fl_list)) {
> uhci->fl->frame[td->frame] = td->link;
> @@ -268,6 +268,7 @@
> list_del_init(&td->fl_list);
> td->frame = -1;
>
> +out:
> spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
> }
>
> @@ -358,6 +359,9 @@
> pci_pool_free(uhci->qh_pool, qh, qh->dma_handle);
> }
>
> +/*
> + * MUST be called with uhci->frame_list_lock acquired
> + */
> static void _uhci_insert_qh(struct uhci *uhci, struct uhci_qh *skelqh, struct urb
>*urb)
> {
> struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
> @@ -417,11 +421,10 @@
> return;
>
> /* Only go through the hoops if it's actually linked in */
> + spin_lock_irqsave(&uhci->frame_list_lock, flags);
> if (!list_empty(&qh->list)) {
> qh->urbp = NULL;
>
> - spin_lock_irqsave(&uhci->frame_list_lock, flags);
> -
> pqh = list_entry(qh->list.prev, struct uhci_qh, list);
>
> if (pqh->urbp) {
> @@ -444,9 +447,8 @@
> qh->element = qh->link = UHCI_PTR_TERM;
>
> list_del_init(&qh->list);
> -
> - spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
> }
> + spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
>
> spin_lock_irqsave(&uhci->qh_remove_list_lock, flags);
>
> @@ -658,6 +660,9 @@
> return urbp;
> }
>
> +/*
> + * MUST be called with urb->lock acquired
> + */
> static void uhci_add_td_to_urb(struct urb *urb, struct uhci_td *td)
> {
> struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
> @@ -667,6 +672,9 @@
> list_add_tail(&td->list, &urbp->td_list);
> }
>
> +/*
> + * MUST be called with urb->lock acquired
> + */
> static void uhci_remove_td_from_urb(struct uhci_td *td)
> {
> if (list_empty(&td->list))
> @@ -677,22 +685,22 @@
> td->urb = NULL;
> }
>
> +/*
> + * MUST be called with urb->lock acquired
> + */
> static void uhci_destroy_urb_priv(struct urb *urb)
> {
> struct list_head *head, *tmp;
> struct urb_priv *urbp;
> struct uhci *uhci;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&urb->lock, flags);
>
> urbp = (struct urb_priv *)urb->hcpriv;
> if (!urbp)
> - goto out;
> + return;
>
> if (!urbp->dev || !urbp->dev->bus || !urbp->dev->bus->hcpriv) {
> warn("uhci_destroy_urb_priv: urb %p belongs to disconnected device or
>bus?", urb);
> - goto out;
> + return;
> }
>
> if (!list_empty(&urb->urb_list))
> @@ -730,9 +738,6 @@
>
> urb->hcpriv = NULL;
> kmem_cache_free(uhci_up_cachep, urbp);
> -
> -out:
> - spin_unlock_irqrestore(&urb->lock, flags);
> }
>
> static void uhci_inc_fsbr(struct uhci *uhci, struct urb *urb)
> @@ -874,7 +879,7 @@
> * It's IN if the pipe is an output pipe or we're not expecting
> * data back.
> */
> - destination &= ~TD_PID;
> + destination &= ~TD_TOKEN_PID_MASK;
> if (usb_pipeout(urb->pipe) || !urb->transfer_buffer_length)
> destination |= USB_PID_IN;
> else
> @@ -1312,9 +1317,7 @@
> struct uhci *uhci = (struct uhci *)urb->dev->bus->hcpriv;
> struct list_head *tmp, *head;
> int ret = 0;
> - unsigned long flags;
>
> - spin_lock_irqsave(&uhci->urb_list_lock, flags);
> head = &uhci->urb_list;
> tmp = head->next;
> while (tmp != head) {
> @@ -1337,8 +1340,6 @@
> } else
> ret = -1; /* no previous urb found */
>
> - spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> -
> return ret;
> }
>
> @@ -1446,34 +1447,30 @@
> return ret;
> }
>
> +/*
> + * MUST be called with uhci->urb_list_lock acquired
> + */
> static struct urb *uhci_find_urb_ep(struct uhci *uhci, struct urb *urb)
> {
> struct list_head *tmp, *head;
> - unsigned long flags;
> - struct urb *u = NULL;
>
> /* We don't match Isoc transfers since they are special */
> if (usb_pipeisoc(urb->pipe))
> return NULL;
>
> - spin_lock_irqsave(&uhci->urb_list_lock, flags);
> head = &uhci->urb_list;
> tmp = head->next;
> while (tmp != head) {
> - u = list_entry(tmp, struct urb, urb_list);
> + struct urb *u = list_entry(tmp, struct urb, urb_list);
>
> tmp = tmp->next;
>
> if (u->dev == urb->dev && u->pipe == urb->pipe &&
> u->status == -EINPROGRESS)
> - goto out;
> + return u;
> }
> - u = NULL;
> -
> -out:
> - spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
>
> - return u;
> + return NULL;
> }
>
> static int uhci_submit_urb(struct urb *urb)
> @@ -1497,13 +1494,15 @@
> INIT_LIST_HEAD(&urb->urb_list);
> usb_inc_dev_use(urb->dev);
>
> - spin_lock_irqsave(&urb->lock, flags);
> + spin_lock_irqsave(&uhci->urb_list_lock, flags);
> + spin_lock(&urb->lock);
>
> if (urb->status == -EINPROGRESS || urb->status == -ECONNRESET ||
> urb->status == -ECONNABORTED) {
> 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);
> + spin_unlock(&urb->lock);
> + spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> usb_dec_dev_use(urb->dev);
>
> return ret;
> @@ -1572,18 +1571,21 @@
> out:
> urb->status = ret;
>
> - spin_unlock_irqrestore(&urb->lock, flags);
> -
> if (ret == -EINPROGRESS) {
> - spin_lock_irqsave(&uhci->urb_list_lock, flags);
> /* We use _tail to make find_urb_ep more efficient */
> list_add_tail(&urb->urb_list, &uhci->urb_list);
> +
> + spin_unlock(&urb->lock);
> spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
>
> return 0;
> }
>
> uhci_unlink_generic(uhci, urb);
> +
> + spin_unlock(&urb->lock);
> + spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> +
> uhci_call_completion(urb);
>
> return ret;
> @@ -1592,7 +1594,7 @@
> /*
> * Return the result of a transfer
> *
> - * Must be called with urb_list_lock acquired
> + * MUST be called with urb_list_lock acquired
> */
> static void uhci_transfer_result(struct uhci *uhci, struct urb *urb)
> {
> @@ -1631,10 +1633,10 @@
>
> urbp->status = ret;
>
> - spin_unlock_irqrestore(&urb->lock, flags);
> -
> - if (ret == -EINPROGRESS)
> + if (ret == -EINPROGRESS) {
> + spin_unlock_irqrestore(&urb->lock, flags);
> return;
> + }
>
> switch (usb_pipetype(urb->pipe)) {
> case PIPE_CONTROL:
> @@ -1664,11 +1666,17 @@
> usb_pipetype(urb->pipe), urb);
> }
>
> + /* Remove it from uhci->urb_list */
> list_del_init(&urb->urb_list);
>
> uhci_add_complete(urb);
> +
> + spin_unlock_irqrestore(&urb->lock, flags);
> }
>
> +/*
> + * MUST be called with urb->lock acquired
> + */
> static void uhci_unlink_generic(struct uhci *uhci, struct urb *urb)
> {
> struct list_head *head, *tmp;
> @@ -1735,6 +1743,9 @@
>
> uhci = (struct uhci *)urb->dev->bus->hcpriv;
>
> + spin_lock_irqsave(&uhci->urb_list_lock, flags);
> + spin_lock(&urb->lock);
> +
> /* Release bandwidth for Interrupt or Isoc. transfers */
> /* Spinlock needed ? */
> if (urb->bandwidth) {
> @@ -1750,35 +1761,41 @@
> }
> }
>
> - if (urb->status != -EINPROGRESS)
> + if (urb->status != -EINPROGRESS) {
> + spin_unlock(&urb->lock);
> + spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> return 0;
> + }
>
> - spin_lock_irqsave(&uhci->urb_list_lock, flags);
> list_del_init(&urb->urb_list);
> - spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
>
> uhci_unlink_generic(uhci, urb);
>
> /* Short circuit the virtual root hub */
> if (urb->dev == uhci->rh.dev) {
> rh_unlink_urb(urb);
> +
> + spin_unlock(&urb->lock);
> + spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> +
> uhci_call_completion(urb);
> } else {
> if (urb->transfer_flags & USB_ASYNC_UNLINK) {
> - /* urb_list is available now since we called */
> - /* uhci_unlink_generic already */
> -
> urbp->status = urb->status = -ECONNABORTED;
>
> - spin_lock_irqsave(&uhci->urb_remove_list_lock, flags);
> + spin_lock(&uhci->urb_remove_list_lock);
>
> - /* Check to see if the remove list is empty */
> + /* If we're the first, set the next interrupt bit */
> if (list_empty(&uhci->urb_remove_list))
> uhci_set_next_interrupt(uhci);
>
> list_add(&urb->urb_list, &uhci->urb_remove_list);
>
> - spin_unlock_irqrestore(&uhci->urb_remove_list_lock, flags);
> + spin_unlock(&uhci->urb_remove_list_lock);
> +
> + spin_unlock(&urb->lock);
> + spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> +
> } else {
> urb->status = -ENOENT;
>
> @@ -1791,6 +1808,9 @@
> } else
> schedule_timeout(1+1*HZ/1000);
>
> + spin_unlock(&urb->lock);
> + spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> +
> uhci_call_completion(urb);
> }
> }
> @@ -1924,12 +1944,14 @@
> /* prepare Interrupt pipe transaction data; HUB INTERRUPT ENDPOINT */
> static int rh_send_irq(struct urb *urb)
> {
> - int i, len = 1;
> struct uhci *uhci = (struct uhci *)urb->dev->bus->hcpriv;
> + struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
> unsigned int io_addr = uhci->io_addr;
> + unsigned long flags;
> + int i, len = 1;
> __u16 data = 0;
> - struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
>
> + spin_lock_irqsave(&urb->lock, flags);
> for (i = 0; i < uhci->rh.numports; i++) {
> data |= ((inw(io_addr + USBPORTSC1 + i * 2) & 0xa) > 0 ? (1 << (i +
>1)) : 0);
> len = (i + 1) / 8 + 1;
> @@ -1939,6 +1961,8 @@
> urb->actual_length = len;
> urbp->status = 0;
>
> + spin_unlock_irqrestore(&urb->lock, flags);
> +
> if ((data > 0) && (uhci->rh.send != 0)) {
> dbg("root-hub INT complete: port1: %x port2: %x data: %x",
> inw(io_addr + USBPORTSC1), inw(io_addr + USBPORTSC2), data);
> @@ -1965,7 +1989,6 @@
>
> spin_lock_irqsave(&uhci->urb_list_lock, flags);
> head = &uhci->urb_list;
> -
> tmp = head->next;
> while (tmp != head) {
> struct urb *u = list_entry(tmp, struct urb, urb_list);
> @@ -1973,6 +1996,8 @@
>
> tmp = tmp->next;
>
> + spin_lock(&urb->lock);
> +
> /* Check if the FSBR timed out */
> if (urbp->fsbr && !urbp->fsbr_timeout && time_after_eq(jiffies,
>urbp->fsbrtime + IDLE_TIMEOUT))
> uhci_fsbr_timeout(uhci, u);
> @@ -1982,6 +2007,8 @@
> list_del(&u->urb_list);
> list_add_tail(&u->urb_list, &list);
> }
> +
> + spin_unlock(&urb->lock);
> }
> spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
>
> @@ -2215,6 +2242,9 @@
> return stat;
> }
>
> +/*
> + * MUST be called with urb->lock acquired
> + */
> static int rh_unlink_urb(struct urb *urb)
> {
> struct uhci *uhci = (struct uhci *)urb->dev->bus->hcpriv;
> @@ -2250,11 +2280,20 @@
>
> static void uhci_call_completion(struct urb *urb)
> {
> - struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
> + struct urb_priv *urbp;
> struct usb_device *dev = urb->dev;
> struct uhci *uhci = (struct uhci *)dev->bus->hcpriv;
> int is_ring = 0, killed, resubmit_interrupt, status;
> struct urb *nurb;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&urb->lock, flags);
> +
> + urbp = (struct urb_priv *)urb->hcpriv;
> + if (!urbp || !urb->dev) {
> + spin_unlock_irqrestore(&urb->lock, flags);
> + return;
> + }
>
> killed = (urb->status == -ENOENT || urb->status == -ECONNABORTED ||
> urb->status == -ECONNRESET);
> @@ -2302,6 +2341,8 @@
> urb->status = status;
>
> urb->dev = NULL;
> + spin_unlock_irqrestore(&urb->lock, flags);
> +
> if (urb->complete) {
> urb->complete(urb);
>
> @@ -2339,11 +2380,14 @@
> struct urb_priv *urbp = list_entry(tmp, struct urb_priv,
>complete_list);
> struct urb *urb = urbp->urb;
>
> - tmp = tmp->next;
> -
> list_del_init(&urbp->complete_list);
> + spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
>
> uhci_call_completion(urb);
> +
> + spin_lock_irqsave(&uhci->complete_list_lock, flags);
> + head = &uhci->complete_list;
> + tmp = head->next;
> }
> spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
> }
> @@ -2365,7 +2409,8 @@
> list_del_init(&urb->urb_list);
>
> urbp->status = urb->status = -ECONNRESET;
> - uhci_call_completion(urb);
> +
> + uhci_add_complete(urb);
> }
> spin_unlock_irqrestore(&uhci->urb_remove_list_lock, flags);
> }
> @@ -3098,3 +3143,4 @@
> MODULE_AUTHOR(DRIVER_AUTHOR);
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_LICENSE("GPL");
> +
> diff -ur linux-2.4.18-pre9/drivers/usb/uhci.h linux-2.4.18-pre9.je/drivers/usb/uhci.h
> --- linux-2.4.18-pre9/drivers/usb/uhci.h Thu Nov 22 11:49:52 2001
> +++ linux-2.4.18-pre9.je/drivers/usb/uhci.h Sun Feb 17 14:17:13 2002
> @@ -121,15 +121,16 @@
> * for TD <info>: (a.k.a. Token)
> */
> #define TD_TOKEN_TOGGLE 19
> -#define TD_PID 0xFF
> +#define TD_TOKEN_PID_MASK 0xFF
> +#define TD_TOKEN_EXPLEN_MASK 0x7FF /* expected length, encoded as n - 1 */
>
> #define uhci_maxlen(token) ((token) >> 21)
> -#define uhci_expected_length(info) (((info >> 21) + 1) & TD_CTRL_ACTLEN_MASK) /*
>1-based */
> +#define uhci_expected_length(info) (((info >> 21) + 1) & TD_TOKEN_EXPLEN_MASK) /*
>1-based */
> #define uhci_toggle(token) (((token) >> TD_TOKEN_TOGGLE) & 1)
> #define uhci_endpoint(token) (((token) >> 15) & 0xf)
> #define uhci_devaddr(token) (((token) >> 8) & 0x7f)
> #define uhci_devep(token) (((token) >> 8) & 0x7ff)
> -#define uhci_packetid(token) ((token) & 0xff)
> +#define uhci_packetid(token) ((token) & TD_TOKEN_PID_MASK)
> #define uhci_packetout(token) (uhci_packetid(token) != USB_PID_IN)
> #define uhci_packetin(token) (uhci_packetid(token) == USB_PID_IN)
>
> @@ -163,7 +164,7 @@
> struct list_head list; /* P: urb->lock */
>
> int frame;
> - struct list_head fl_list; /* P: frame_list_lock */
> + struct list_head fl_list; /* P: uhci->frame_list_lock */
> } __attribute__((aligned(16)));
>
> /*
> @@ -306,21 +307,25 @@
> struct uhci_qh *skelqh[UHCI_NUM_SKELQH]; /* Skeleton QH's */
>
> spinlock_t frame_list_lock;
> - struct uhci_frame_list *fl; /* Frame list */
> + struct uhci_frame_list *fl; /* P: uhci->frame_list_lock */
> int fsbr; /* Full speed bandwidth reclamation */
> int is_suspended;
>
> + /* Main list of URB's currently controlled by this HC */
> + spinlock_t urb_list_lock;
> + struct list_head urb_list; /* P: uhci->urb_list_lock */
> +
> + /* List of QH's that are done, but waiting to be unlinked (race) */
> spinlock_t qh_remove_list_lock;
> - struct list_head qh_remove_list;
> + struct list_head qh_remove_list; /* P: uhci->qh_remove_list_lock */
>
> + /* List of asynchronously unlinked URB's */
> spinlock_t urb_remove_list_lock;
> - struct list_head urb_remove_list;
> -
> - spinlock_t urb_list_lock;
> - struct list_head urb_list;
> + struct list_head urb_remove_list; /* P: uhci->urb_remove_list_lock */
>
> + /* List of URB's awaiting completion callback */
> spinlock_t complete_list_lock;
> - struct list_head complete_list;
> + struct list_head complete_list; /* P: uhci->complete_list_lock */
>
> struct virt_root_hub rh; /* private data of the virtual root hub */
> };
> @@ -333,7 +338,7 @@
> dma_addr_t transfer_buffer_dma_handle;
>
> struct uhci_qh *qh; /* QH for this URB */
> - struct list_head td_list;
> + struct list_head td_list; /* P: urb->lock */
>
> int fsbr : 1; /* URB turned on FSBR */
> int fsbr_timeout : 1; /* URB timed out on FSBR */
> @@ -345,11 +350,36 @@
> int status; /* Final status */
>
> unsigned long inserttime; /* In jiffies */
> - unsigned long fsbrtime; /* In jiffies */
> + unsigned long fsbrtime; /* In jiffies */
>
> - struct list_head queue_list;
> - struct list_head complete_list;
> + struct list_head queue_list; /* P: uhci->frame_list_lock */
> + struct list_head complete_list; /* P: uhci->complete_list_lock */
> };
> +
> +/*
> + * Locking in uhci.c
> + *
> + * spinlocks are used extensively to protect the many lists and data
> + * structures we have. It's not that pretty, but it's necessary. We
> + * need to be done with all of the locks (except complete_list_lock) when
> + * we call urb->complete. I've tried to make it simple enough so I don't
> + * have to spend hours racking my brain trying to figure out if the
> + * locking is safe.
> + *
> + * Here's the safe locking order to prevent deadlocks:
> + *
> + * #1 uhci->urb_list_lock
> + * #2 urb->lock
> + * #3 uhci->urb_remove_list_lock, uhci->frame_list_lock,
> + * uhci->qh_remove_list_lock
> + * #4 uhci->complete_list_lock
> + *
> + * If you're going to grab 2 or more locks at once, ALWAYS grab the lock
> + * at the lowest level FIRST and NEVER grab locks at the same level at the
> + * same time.
> + *
> + * So, if you need uhci->urb_list_lock, grab it before you grab urb->lock
> + */
>
> /* -------------------------------------------------------------------------
> Virtual Root HUB
>
> _______________________________________________
> [EMAIL PROTECTED]
> To unsubscribe, use the last form field at:
> https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
