On Thu, 8 Apr 2004, Marcel Holtmann wrote:
> Hi Alan,
>
> many thanks for looking at the problem.
You're welcome.
> I am not an expert when it comes
> to USB internals, but from your explanation I ask myself why we don't
> have an unlink function that will wait until the last reference to the
> URB is gone. Why do we have to do it in the driver itself?
I feel the same way. The API works as designed, but the design itself
should be improved. A function very much like the one I added to your
driver will probably go into the USB core, but I don't know when.
Another change I would like to see would be to have separate
usb_unlink_urb functions for synchronous and asynchronous; that would
affect so many drivers that it's not likely to happen any time soon.
> > By the way, I noticed a couple of other little things in the driver that
> > could be improved. The first is _urb_alloc() -- it should call
> > usb_init_urb() rather than initializing the URB by hand. The second was
> > the test of the HCI_RUNNING flags bit in the hci_usb_{t,r}x_complete()
> > functions. Those tests should be made while holding hci->completion_lock
> > for proper synchronization. Otherwise you can have a race on an SMP
> > system in which hci_usb_rx_complete() finds the bit is set; then on
> > another processor hci_usb_close() clears the bit, grabs & releases the
> > lock, and unlinks all the active URBs; then the first processor acquires
> > the lock and resubmits the URB.
>
> Good points. Do you have a patch for me?
Here's a combined patch. Would you like to submit these changes to Greg?
Alan Stern
===== drivers/bluetooth/hci_usb.c 1.42 vs edited =====
--- 1.42/drivers/bluetooth/hci_usb.c Tue Mar 30 17:57:21 2004
+++ edited/drivers/bluetooth/hci_usb.c Fri Apr 9 11:28:24 2004
@@ -109,8 +109,7 @@
sizeof(struct usb_iso_packet_descriptor) * isoc, gfp);
if (_urb) {
memset(_urb, 0, sizeof(*_urb));
- _urb->urb.count = (atomic_t)ATOMIC_INIT(1);
- spin_lock_init(&_urb->urb.lock);
+ usb_init_urb(&_urb->urb);
}
return _urb;
}
@@ -341,6 +340,14 @@
return 0;
}
+static void inline wait_for_urb(struct urb *urb)
+{
+ while (atomic_read(&urb->count) > 1) {
+ current->state = TASK_UNINTERRUPTIBLE;
+ schedule_timeout((5 * HZ + 999) / 1000);
+ }
+}
+
static void hci_usb_unlink_urbs(struct hci_usb *husb)
{
int i;
@@ -357,6 +364,7 @@
BT_DBG("%s unlinking _urb %p type %d urb %p",
husb->hdev->name, _urb, _urb->type, urb);
usb_unlink_urb(urb);
+ wait_for_urb(urb);
_urb_queue_tail(__completed_q(husb, _urb->type), _urb);
}
@@ -699,11 +707,11 @@
BT_DBG("%s urb %p type %d status %d count %d flags %x", hdev->name, urb,
_urb->type, urb->status, count, urb->transfer_flags);
- if (!test_bit(HCI_RUNNING, &hdev->flags))
- return;
-
read_lock(&husb->completion_lock);
+ if (!test_bit(HCI_RUNNING, &hdev->flags))
+ goto done;
+
if (urb->status || !count)
goto resubmit;
@@ -739,6 +747,7 @@
BT_DBG("%s urb %p type %d resubmit status %d", hdev->name, urb,
_urb->type, err);
+done:
read_unlock(&husb->completion_lock);
}
-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
