[2.4] "Fix" introduced in 2.4.27pre2 for bluetooth hci_usb race causes kernel hang

[2.4] "Fix" introduced in 2.4.27pre2 for bluetooth hci_usb race causes kernel hang

[Tomas Ögren]

Hello.

I have noticed a problem with a race condition fix introduced in
2.4.27-pre2 that causes the kernel to hang when disconnecting a
Bluetooth USB dongle or doing 'hciconfig hci0 down'. No message is
printed, the kernel just doesn't respond anymore.

Seen in Changelog:
Marcel Holtmann:
  o [Bluetooth] Fix race in RX complete routine of the USB drivers

Reversing the following patch to hci_usb_rx_complete() makes 2.4.27-pre2
up until 2.4.30 happy and does not hang when removing the dongle
anymore. (bfusb.c has the same patch applied)

2.6.11.7 does not show the same problem, but has similar code to the
"fixed" (that hangs) code in 2.4, so the real problem is probably
somewhere else.

I have tested this on Dell Optiplex GX150, 260 and 280's which has Intel
P3 and P4 with Intel UHCI USB chipset. I have tested both usb-uhci.o and
uhci.o with the same results. Tested with USB Bluetooth dongles with
both Broadcom and Cambridge Silicon Radio chipsets, same results.

modules loaded: l2cap, hci_usb, bluez, (usb-)uhci, usbcore

diff -ruN linux-2.4.27-pre1/drivers/bluetooth/hci_usb.c linux-2.4.27-pre2/drivers/bluetooth/hci_usb.c
--- linux-2.4.27-pre1/drivers/bluetooth/hci_usb.c       2004-04-14 15:05:29.000000000 +0200
+++ linux-2.4.27-pre2/drivers/bluetooth/hci_usb.c       2005-04-08 20:16:51.000000000 +0200
@@ -699,11 +699,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 unlock;
+
        if (urb->status || !count)
                goto resubmit;

@@ -740,6 +740,8 @@
                BT_DBG("%s urb %p type %d resubmit status %d", hdev->name, urb,
                                _urb->type, err);
        }
+
+unlock:
        read_unlock(&husb->completion_lock);
 }


Please CC me for any responses, not on the list.

/Tomas
-- 
Tomas Ögren, stric@acc.umu.se, http://www.acc.umu.se/~stric/
|- Student at Computing Science, University of Umeå
`- Sysadmin at {cs,acc}.umu.se

Re: [2.4] "Fix" introduced in 2.4.27pre2 for bluetooth hci_usb race causes kernel hang

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

Hi Tomas,

> I have noticed a problem with a race condition fix introduced in
> 2.4.27-pre2 that causes the kernel to hang when disconnecting a
> Bluetooth USB dongle or doing 'hciconfig hci0 down'. No message is
> printed, the kernel just doesn't respond anymore.
> 
> Seen in Changelog:
> Marcel Holtmann:
>   o [Bluetooth] Fix race in RX complete routine of the USB drivers
> 
> Reversing the following patch to hci_usb_rx_complete() makes 2.4.27-pre2
> up until 2.4.30 happy and does not hang when removing the dongle
> anymore. (bfusb.c has the same patch applied)
> 
> 2.6.11.7 does not show the same problem, but has similar code to the
> "fixed" (that hangs) code in 2.4, so the real problem is probably
> somewhere else.

does the attached patch makes any difference?

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 547 bytes --]

===== drivers/bluetooth/hci_usb.c 1.23 vs edited =====
--- 1.23/drivers/bluetooth/hci_usb.c	2004-07-31 13:02:43 +02:00
+++ edited/drivers/bluetooth/hci_usb.c	2005-04-09 15:37:12 +02:00
@@ -398,12 +398,12 @@
 
 	BT_DBG("%s", hdev->name);
 
+	/* Synchronize with completion handlers */
 	write_lock_irqsave(&husb->completion_lock, flags);
-	
+	write_unlock_irqrestore(&husb->completion_lock, flags);
+
 	hci_usb_unlink_urbs(husb);
 	hci_usb_flush(hdev);
-
-	write_unlock_irqrestore(&husb->completion_lock, flags);
 
 	MOD_DEC_USE_COUNT;
 	return 0;

Re: [2.4] "Fix" introduced in 2.4.27pre2 for bluetooth hci_usb race causes kernel hang

[Tomas Ögren]

On 09 April, 2005 - Marcel Holtmann sent me these 1,6K bytes:

> Hi Tomas,
> 
> > I have noticed a problem with a race condition fix introduced in
> > 2.4.27-pre2 that causes the kernel to hang when disconnecting a
> > Bluetooth USB dongle or doing 'hciconfig hci0 down'. No message is
> > printed, the kernel just doesn't respond anymore.
> > 
> > Seen in Changelog:
> > Marcel Holtmann:
> >   o [Bluetooth] Fix race in RX complete routine of the USB drivers
> > 
> > Reversing the following patch to hci_usb_rx_complete() makes 2.4.27-pre2
> > up until 2.4.30 happy and does not hang when removing the dongle
> > anymore. (bfusb.c has the same patch applied)
> > 
> > 2.6.11.7 does not show the same problem, but has similar code to the
> > "fixed" (that hangs) code in 2.4, so the real problem is probably
> > somewhere else.
> 
> does the attached patch makes any difference?

It works just fine with pristine 2.4.30 and this patch. No deadlocks
anymore.

/Tomas
-- 
Tomas Ögren, stric@acc.umu.se, http://www.acc.umu.se/~stric/
|- Student at Computing Science, University of Umeå
`- Sysadmin at {cs,acc}.umu.se

Re: [2.4] "Fix" introduced in 2.4.27pre2 for bluetooth hci_usb race causes kernel hang

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

Hi Tomas,

> > > I have noticed a problem with a race condition fix introduced in
> > > 2.4.27-pre2 that causes the kernel to hang when disconnecting a
> > > Bluetooth USB dongle or doing 'hciconfig hci0 down'. No message is
> > > printed, the kernel just doesn't respond anymore.
> > > 
> > > Seen in Changelog:
> > > Marcel Holtmann:
> > >   o [Bluetooth] Fix race in RX complete routine of the USB drivers
> > > 
> > > Reversing the following patch to hci_usb_rx_complete() makes 2.4.27-pre2
> > > up until 2.4.30 happy and does not hang when removing the dongle
> > > anymore. (bfusb.c has the same patch applied)
> > > 
> > > 2.6.11.7 does not show the same problem, but has similar code to the
> > > "fixed" (that hangs) code in 2.4, so the real problem is probably
> > > somewhere else.
> > 
> > does the attached patch makes any difference?
> 
> It works just fine with pristine 2.4.30 and this patch. No deadlocks
> anymore.

if this works then we should do the same change in the bfusb driver. A
patch that fixes both drivers is attached.

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 978 bytes --]

===== drivers/bluetooth/bfusb.c 1.3 vs edited =====
--- 1.3/drivers/bluetooth/bfusb.c	2004-04-16 14:01:40 +02:00
+++ edited/drivers/bluetooth/bfusb.c	2005-04-13 12:49:55 +02:00
@@ -470,11 +470,10 @@
 		return 0;
 
 	write_lock_irqsave(&bfusb->lock, flags);
+	write_unlock_irqrestore(&bfusb->lock, flags);
 
 	bfusb_unlink_urbs(bfusb);
 	bfusb_flush(hdev);
-
-	write_unlock_irqrestore(&bfusb->lock, flags);
 
 	MOD_DEC_USE_COUNT;
 
===== drivers/bluetooth/hci_usb.c 1.23 vs edited =====
--- 1.23/drivers/bluetooth/hci_usb.c	2004-07-31 13:02:43 +02:00
+++ edited/drivers/bluetooth/hci_usb.c	2005-04-09 15:37:12 +02:00
@@ -398,12 +398,12 @@
 
 	BT_DBG("%s", hdev->name);
 
+	/* Synchronize with completion handlers */
 	write_lock_irqsave(&husb->completion_lock, flags);
-	
+	write_unlock_irqrestore(&husb->completion_lock, flags);
+
 	hci_usb_unlink_urbs(husb);
 	hci_usb_flush(hdev);
-
-	write_unlock_irqrestore(&husb->completion_lock, flags);
 
 	MOD_DEC_USE_COUNT;
 	return 0;