Hi Alan, thanks for the patch. Yes, it works. Comments below.
On Thursday 24 July 2003 20:58, Alan Stern wrote:
> On Fri, 18 Jul 2003, Duncan Sands wrote:
> > Hi Alan, Johannes, how about this? I tried to make as few changes to the
> > existing code as possible. In my opinion it would make more sense to
> > have the list of TDs belong to the QH and not the urb_priv, and free them
> > when the QH is freed, but that would mean making many more and more
> > invasive changes. By the way, does the last
> > + uhci_free_pending_tds(uhci);
> > make sense? If so, should there be a
> > + uhci_remove_pending_qhs(uhci);
> > as well?
>
> Duncan:
>
> I looked at your suggested patch; it seems like it ought to work.
:)
> I'm trying a different approach. It's a little more complicated, but it
> doesn't need an extra list header added to each TD. The idea is not to
> free the urbp as soon as the URB is given back, but to wait until
> uhci_free_pending_qhs() has released the QH as well.
My patch also doesn't need to add a list header to each TD: you can just
reuse the list entry. I added the new list head to keep things symmetric
with QHs.
I guess you preferred to keep the TDs as part of the urbp (rather than
making them part of the QH) because isochronous transfers have no QH?
Fair enough. However, wouldn't a cleaner approach be to do the same for
QHs as you do for TDs? I mean: just have a list of urbp's to be removed
(rather than a list of QHs), and when the urbp goes down destroy the TDs
and any QH? That way you can remove a list_head (remove_list) from the
QH as well :)
Comments on the patch are below.
> Here's the patch. Please try it and let me know if it fixes that
> Controller Halted problem. (You will probably have to use a little fuzz
> when applying it, because I wrote it on top of a couple of other patches.)
>
> Alan Stern
>
>
> ===== uhci-hcd.c 1.55 vs edited =====
> --- 1.55/drivers/usb/host/uhci-hcd.c Tue Jul 22 11:19:19 2003
> +++ edited/drivers/usb/host/uhci-hcd.c Wed Jul 23 16:45:35 2003
> @@ -403,7 +403,6 @@
>
> /*
> * Start removal of QH from schedule; it finishes next frame.
> - * TDs should be unlinked before this is called.
> */
> static void uhci_remove_qh(struct uhci_hcd *uhci, struct uhci_qh *qh)
> {
> @@ -413,8 +412,6 @@
> if (!qh)
> return;
>
> - qh->urbp = NULL;
> -
> /*
> * Only go through the hoops if it's actually linked in
> * Queued QHs are removed in uhci_delete_queued_urb,
> @@ -666,6 +663,7 @@
> list_add_tail(&urbp->urb_list, &uhci->urb_list);
>
> urb->hcpriv = urbp;
> + atomic_set(&urbp->refcount, 1);
>
> return urbp;
> }
> @@ -696,22 +694,20 @@
> }
>
> /*
> - * MUST be called with urb->lock acquired
> + * MUST be called with urbp->urb == NULL
> */
> -static void uhci_destroy_urb_priv(struct uhci_hcd *uhci, struct urb *urb)
> +static void uhci_destroy_urb_priv(struct uhci_hcd *uhci, struct urb_priv
> *urbp) {
> struct list_head *head, *tmp;
> - struct urb_priv *urbp;
>
> - urbp = (struct urb_priv *)urb->hcpriv;
> if (!urbp)
> return;
>
> if (!list_empty(&urbp->urb_list))
> - warn("uhci_destroy_urb_priv: urb %p still on uhci->urb_list or
> uhci->remove_list", urb); + warn("uhci_destroy_urb_priv: urbp %p still on
> uhci->urb_list or uhci->remove_list", urbp);
>
> if (!list_empty(&urbp->complete_list))
> - warn("uhci_destroy_urb_priv: urb %p still on uhci->complete_list",
> urb);
> + warn("uhci_destroy_urb_priv: urbp %p still on uhci->complete_list",
> urbp);
>
> head = &urbp->td_list;
> tmp = head->next;
> @@ -725,7 +721,6 @@
> uhci_free_td(uhci, td);
> }
>
> - urb->hcpriv = NULL;
> kmem_cache_free(uhci_up_cachep, urbp);
> }
>
> @@ -891,6 +886,7 @@
>
> urbp->qh = qh;
> qh->urbp = urbp;
> + atomic_inc(&urbp->refcount);
Why not define uhci_get_urbp and uhci_put_urbp methods?
Especially uhci_put_urbp, which would call uhci_destroy_urb_priv
when the urbp goes down. At the moment you do this by hand
each time.
> uhci_insert_tds_in_qh(qh, urb, UHCI_PTR_BREADTH);
>
> @@ -926,6 +922,7 @@
> urbp->short_control_packet = 1;
>
> /* Create a new QH to avoid pointer overwriting problems */
> + urbp->qh->urbp = NULL;
> uhci_remove_qh(uhci, urbp->qh);
>
> /* Delete all of the TD's except for the status TD at the end */
> @@ -943,6 +940,7 @@
>
> urbp->qh = uhci_alloc_qh(uhci, urb->dev);
> if (!urbp->qh) {
> + atomic_dec(&urbp->refcount);
So no destroy_urb_priv here...
> err("unable to allocate new QH for control retrigger");
> return -ENOMEM;
> }
> @@ -1150,6 +1148,7 @@
>
> urbp->qh = qh;
> qh->urbp = urbp;
> + atomic_inc(&urbp->refcount);
>
> /* Always breadth first */
> uhci_insert_tds_in_qh(qh, urb, UHCI_PTR_BREADTH);
> @@ -1491,7 +1490,9 @@
>
> list_del_init(&urbp->urb_list);
> spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
> - uhci_destroy_urb_priv (uhci, urb);
> + urb->hcpriv = NULL;
> + urbp->urb = NULL;
> + uhci_destroy_urb_priv (uhci, urbp);
>
> return ret;
> }
> @@ -1630,9 +1631,8 @@
>
> uhci_delete_queued_urb(uhci, urb);
>
> - /* The interrupt loop will reclaim the QH's */
> + /* The interrupt loop will reclaim the QH's and TD's */
> uhci_remove_qh(uhci, urbp->qh);
> - urbp->qh = NULL;
> }
>
> static int uhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb)
> @@ -1781,19 +1781,24 @@
>
> static void uhci_free_pending_qhs(struct uhci_hcd *uhci)
> {
> - struct list_head *tmp, *head;
> + struct list_head *head;
> unsigned long flags;
>
> spin_lock_irqsave(&uhci->qh_remove_list_lock, flags);
> head = &uhci->qh_remove_list;
> - tmp = head->next;
> - while (tmp != head) {
> - struct uhci_qh *qh = list_entry(tmp, struct uhci_qh, remove_list);
> -
> - tmp = tmp->next;
> + while (!list_empty(head)) {
> + struct uhci_qh *qh = list_entry(head->next,
> + struct uhci_qh, remove_list);
> + struct urb_priv *urbp = qh->urbp;
>
> list_del_init(&qh->remove_list);
> -
> + if (urbp && atomic_dec_and_test(&urbp->refcount)) {
> + qh->urbp = NULL;
> + urbp->qh = NULL;
> + spin_unlock_irqrestore(&uhci->qh_remove_list_lock, flags);
> + uhci_destroy_urb_priv(uhci, urbp);
Why release the spinlock before calling this?
> + spin_lock_irqsave(&uhci->qh_remove_list_lock, flags);
> + }
> uhci_free_qh(uhci, qh);
> }
> spin_unlock_irqrestore(&uhci->qh_remove_list_lock, flags);
> @@ -1802,10 +1807,14 @@
> static void uhci_finish_urb(struct usb_hcd *hcd, struct urb *urb, struct
> pt_regs *regs) {
> struct uhci_hcd *uhci = hcd_to_uhci(hcd);
> + struct urb_priv *urbp = (struct urb_priv *) urb->hcpriv;
> unsigned long flags;
>
> spin_lock_irqsave(&urb->lock, flags);
> - uhci_destroy_urb_priv(uhci, urb);
> + urbp->urb = NULL;
> + urb->hcpriv = NULL;
> + if (atomic_dec_and_test(&urbp->refcount))
> + uhci_destroy_urb_priv(uhci, urbp);
> spin_unlock_irqrestore(&urb->lock, flags);
>
> usb_hcd_giveback_urb(hcd, urb, regs);
> @@ -1814,24 +1823,21 @@
> static void uhci_finish_completion(struct usb_hcd *hcd, struct pt_regs
> *regs) {
> struct uhci_hcd *uhci = hcd_to_uhci(hcd);
> - struct list_head *tmp, *head;
> + struct list_head *head;
> unsigned long flags;
>
> spin_lock_irqsave(&uhci->complete_list_lock, flags);
> head = &uhci->complete_list;
> - tmp = head->next;
> - while (tmp != head) {
> - struct urb_priv *urbp = list_entry(tmp, struct urb_priv,
> complete_list);
> - struct urb *urb = urbp->urb;
> + while (!list_empty(head)) {
> + struct urb_priv *urbp = list_entry(head->next,
> + struct urb_priv, complete_list);
>
> list_del_init(&urbp->complete_list);
> spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
>
> - uhci_finish_urb(hcd, urb, regs);
> + uhci_finish_urb(hcd, urbp->urb, regs);
>
> spin_lock_irqsave(&uhci->complete_list_lock, flags);
> - head = &uhci->complete_list;
> - tmp = head->next;
> }
> spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
> }
> ===== uhci-hcd.h 1.15 vs edited =====
> --- 1.15/drivers/usb/host/uhci-hcd.h Tue Jul 22 11:26:32 2003
> +++ edited/drivers/usb/host/uhci-hcd.h Wed Jul 23 16:29:16 2003
> @@ -382,6 +382,7 @@
>
> unsigned long inserttime; /* In jiffies */
> unsigned long fsbrtime; /* In jiffies */
> + atomic_t refcount; /* 1 for URB, 1 for QH */
>
> struct list_head queue_list; /* P: uhci->frame_list_lock */
> struct list_head complete_list; /* P: uhci->complete_list_lock */
By the way, looks like much of the list-handling in uhci-hcd could be done
more cleanly using the macros from list.h.
Ciao,
Duncan.
-------------------------------------------------------
This SF.Net email sponsored by: Free pre-built ASP.NET sites including
Data Reports, E-commerce, Portals, and Forums are available now.
Download today and enter to win an XBOX or Visual Studio .NET.
http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
