[PATCH] hci-usb bugfix

[PATCH] hci-usb bugfix

[Sebastian Schmidt]

Hi,

this is a little patch against 2.6.6-rc3 which fixes the Oops when
unplugging an USB Bluetooth device.

hci_usb_disconnect() got called recursively which caused
sysfs_hash_and_remove() finally to dereference a NULL pointer.

--- SNIP ---
diff -uNr linux-2.6.6-rc3.old/drivers/bluetooth/hci_usb.c linux-2.6.6-rc3/drivers/bluetooth/hci_usb.c
--- linux-2.6.6-rc3.old/drivers/bluetooth/hci_usb.c     2004-05-09 20:25:00.000000000 +0200
+++ linux-2.6.6-rc3/drivers/bluetooth/hci_usb.c 2004-05-09 20:28:30.000000000 +0200
@@ -986,8 +986,21 @@

        hci_usb_close(hdev);

-       if (husb->isoc_iface)
+       if (husb->isoc_iface) {
+#if 0
                usb_driver_release_interface(&hci_usb_driver, husb->isoc_iface);
+#else
+               /* do the same as usb_driver_release_interface would do,
+                * except calling diconnect().
+                * usb_driver_release_interface() _does_ check if
+                *  are in disconnect() or add, but I really dunno 
+                *  where dev->driver_list or dev->bus_list gets set.
+                *      -- yath
+                */
+               husb->isoc_iface->dev.driver = NULL;
+               usb_set_intfdata(husb->isoc_iface, NULL);
+#endif
+       }

        if (hci_unregister_dev(hdev) < 0)
                BT_ERR("Can't unregister HCI device %s", hdev->name);
--- SNAP ---

Please apply it,

Sebastian

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Sebastian,

> this is a little patch against 2.6.6-rc3 which fixes the Oops when
> unplugging an USB Bluetooth device.
> 
> hci_usb_disconnect() got called recursively which caused
> sysfs_hash_and_remove() finally to dereference a NULL pointer.
> 
> --- SNIP ---
> diff -uNr linux-2.6.6-rc3.old/drivers/bluetooth/hci_usb.c linux-2.6.6-rc3/drivers/bluetooth/hci_usb.c
> --- linux-2.6.6-rc3.old/drivers/bluetooth/hci_usb.c     2004-05-09 20:25:00.000000000 +0200
> +++ linux-2.6.6-rc3/drivers/bluetooth/hci_usb.c 2004-05-09 20:28:30.000000000 +0200
> @@ -986,8 +986,21 @@
> 
>         hci_usb_close(hdev);
> 
> -       if (husb->isoc_iface)
> +       if (husb->isoc_iface) {
> +#if 0
>                 usb_driver_release_interface(&hci_usb_driver, husb->isoc_iface);
> +#else
> +               /* do the same as usb_driver_release_interface would do,
> +                * except calling diconnect().
> +                * usb_driver_release_interface() _does_ check if
> +                *  are in disconnect() or add, but I really dunno 
> +                *  where dev->driver_list or dev->bus_list gets set.
> +                *      -- yath
> +                */
> +               husb->isoc_iface->dev.driver = NULL;
> +               usb_set_intfdata(husb->isoc_iface, NULL);
> +#endif
> +       }
> 
>         if (hci_unregister_dev(hdev) < 0)
>                 BT_ERR("Can't unregister HCI device %s", hdev->name);
> --- SNAP ---
> 
> Please apply it,

thanks for finding the reason for this problem, because the only thing
that I knew, was that enabled SCO support caused this problem. I prefer
to let the USB guys fix it, because this bug should be present for all
drivers that claimed more than one interface.

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Alan Stern]

On Sun, 9 May 2004, Marcel Holtmann wrote:

> Hi Sebastian,
> 
> > this is a little patch against 2.6.6-rc3 which fixes the Oops when
> > unplugging an USB Bluetooth device.
> > 
> > hci_usb_disconnect() got called recursively which caused
> > sysfs_hash_and_remove() finally to dereference a NULL pointer.

> thanks for finding the reason for this problem, because the only thing
> that I knew, was that enabled SCO support caused this problem. I prefer
> to let the USB guys fix it, because this bug should be present for all
> drivers that claimed more than one interface.
> 
> Regards
> 
> Marcel

It looks like the problem is that hci_usb_disconnect() is trying to do too 
much.  When called for the SCO interface it should simply return.

Does this patch improve the situation?

Alan Stern



===== drivers/bluetooth/hci_usb.c 1.44 vs edited =====
--- 1.44/drivers/bluetooth/hci_usb.c	Thu Apr 29 13:33:56 2004
+++ edited/drivers/bluetooth/hci_usb.c	Mon May 10 12:10:31 2004
@@ -918,7 +918,7 @@
 			BT_ERR("Can't set isoc interface settings");
 			isoc_iface = NULL;
 		}
-		usb_driver_claim_interface(&hci_usb_driver, isoc_iface, husb);
+		usb_driver_claim_interface(&hci_usb_driver, isoc_iface, NULL);
 		husb->isoc_iface  = isoc_iface;
 		husb->isoc_in_ep  = isoc_in_ep[isoc_ifnum];
 		husb->isoc_out_ep = isoc_out_ep[isoc_ifnum];

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

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

Hi Alan,

> It looks like the problem is that hci_usb_disconnect() is trying to do too 
> much.  When called for the SCO interface it should simply return.

I came to the same conclusion, but I used the attached patch.

> Does this patch improve the situation?

Not tested ;)

Regards

Marcel


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

===== drivers/bluetooth/hci_usb.c 1.49 vs edited =====
--- 1.49/drivers/bluetooth/hci_usb.c	Sat Apr 17 00:23:48 2004
+++ edited/drivers/bluetooth/hci_usb.c	Mon May 10 12:03:05 2004
@@ -976,11 +971,13 @@
 static void hci_usb_disconnect(struct usb_interface *intf)
 {
 	struct hci_usb *husb = usb_get_intfdata(intf);
-	struct hci_dev *hdev = husb->hdev;
+	struct hci_dev *hdev;
 
-	if (!husb)
+	if (!husb || intf == husb->isoc_iface)
 		return;
+
 	usb_set_intfdata(intf, NULL);
+	hdev = husb->hdev;
 
 	BT_DBG("%s", hdev->name);
 

Re: [PATCH] hci-usb bugfix

[Oliver Neukum]

Am Montag, 10. Mai 2004 18:19 schrieb Marcel Holtmann:
> Hi Alan,
> 
> > It looks like the problem is that hci_usb_disconnect() is trying to do too 
> > much.  When called for the SCO interface it should simply return.
> 
> I came to the same conclusion, but I used the attached patch.

Which is wrong. It assumes that interfaces are disconnected in a certain
order, which happens only by chance in your case.

	Regards
		Oliver

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Oliver,

> > > It looks like the problem is that hci_usb_disconnect() is trying to do too 
> > > much.  When called for the SCO interface it should simply return.
> > 
> > I came to the same conclusion, but I used the attached patch.
> 
> Which is wrong. It assumes that interfaces are disconnected in a certain
> order, which happens only by chance in your case.

why is it wrong? If interface 0 is disconnected first then we go into
the disconnect routine and cleanup. But if interface 1 is disconnected
first, we don't do anything and "wait" for the disconnect on first
interface.

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Oliver Neukum]

Am Montag, 10. Mai 2004 18:52 schrieb Marcel Holtmann:
> Hi Oliver,
> 
> > > > It looks like the problem is that hci_usb_disconnect() is trying to do too 
> > > > much.  When called for the SCO interface it should simply return.
> > > 
> > > I came to the same conclusion, but I used the attached patch.
> > 
> > Which is wrong. It assumes that interfaces are disconnected in a certain
> > order, which happens only by chance in your case.
> 
> why is it wrong? If interface 0 is disconnected first then we go into
> the disconnect routine and cleanup. But if interface 1 is disconnected
> first, we don't do anything and "wait" for the disconnect on first
> interface.

Which not necessarily will ever come. It is possible that just one
interface is disconnected.

	Regards
		Oliver

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Oliver,

> > > Which is wrong. It assumes that interfaces are disconnected in a certain
> > > order, which happens only by chance in your case.
> > 
> > why is it wrong? If interface 0 is disconnected first then we go into
> > the disconnect routine and cleanup. But if interface 1 is disconnected
> > first, we don't do anything and "wait" for the disconnect on first
> > interface.
> 
> Which not necessarily will ever come. It is possible that just one
> interface is disconnected.

which results in the same as if we set NULL for the private pointer when
we claim the second interface. If this really happens then we have more
problems in the driver itself, because this case won't be handled in
either way. However I don't think that this will happen, because for
Bluetooth devices interface 0 and 1 can be seen as a unit. The only
reason that this was split over two interfaces, was that you don't have
to stop the bulk transfers when you change the altsetting on the second
interface.

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Oliver Neukum]

> which results in the same as if we set NULL for the private pointer when
> we claim the second interface. If this really happens then we have more
> problems in the driver itself, because this case won't be handled in
> either way. However I don't think that this will happen, because for

You can trigger it in software through usbfs.

> Bluetooth devices interface 0 and 1 can be seen as a unit. The only
> reason that this was split over two interfaces, was that you don't have
> to stop the bulk transfers when you change the altsetting on the second
> interface.

Yes, but you should really stop using the second interface _before_
returning returning from disconnect() for _that_ interface. You will
operate correctly if the primary interface is disconnected first,
but you cannot depend on that. If the secondary interface is
disconnected first, you have a window where you illegally use an
interface you no longer own.

	Regards
		Oliver

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Oliver,

> > which results in the same as if we set NULL for the private pointer when
> > we claim the second interface. If this really happens then we have more
> > problems in the driver itself, because this case won't be handled in
> > either way. However I don't think that this will happen, because for
> 
> You can trigger it in software through usbfs.

I've never done this before. Can you show me how?

> > Bluetooth devices interface 0 and 1 can be seen as a unit. The only
> > reason that this was split over two interfaces, was that you don't have
> > to stop the bulk transfers when you change the altsetting on the second
> > interface.
> 
> Yes, but you should really stop using the second interface _before_
> returning returning from disconnect() for _that_ interface. You will
> operate correctly if the primary interface is disconnected first,
> but you cannot depend on that. If the secondary interface is
> disconnected first, you have a window where you illegally use an
> interface you no longer own.

You are absolutely right and this needs to be fixed, but this problem is
a different one than that my patch fixes. However this problem is now on
my todo list. Thanks for making me aware of.

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Alan Stern]

On Mon, 10 May 2004, Marcel Holtmann wrote:

> Hi Alan,
> 
> > It looks like the problem is that hci_usb_disconnect() is trying to do too 
> > much.  When called for the SCO interface it should simply return.
> 
> I came to the same conclusion, but I used the attached patch.
> 
> > Does this patch improve the situation?
> 
> Not tested ;)

Better not test it -- I just noticed that without your patch installed the 
driver would try to dereference a NULL pointer!  Probably the safest 
approach is to use both patches.

Alan Stern

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Alan,

> Better not test it -- I just noticed that without your patch installed the 
> driver would try to dereference a NULL pointer!  Probably the safest 
> approach is to use both patches.

actually I don't see a reason for using both patches, because I can't
follow the point from Oliver that I've assumed a certain order of
disconnect.

However is it the best practice to claim additional interfaces with the
private pointer set to NULL?

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Alan Stern]

On Mon, 10 May 2004, Marcel Holtmann wrote:

> Hi Alan,
> 
> actually I don't see a reason for using both patches, because I can't
> follow the point from Oliver that I've assumed a certain order of
> disconnect.

I can't follow his point either.  Maybe he forgot that when the main 
interface is disconnected the driver will release the SCO interface as 
well.

> However is it the best practice to claim additional interfaces with the
> private pointer set to NULL?

The private pointer can be set to anything at all, since it's private.  
If your driver doesn't need to use it, then set it to NULL.  Nobody else
will try to access the pointer when your driver has claimed the interface.

Alan Stern

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Alan,

> > actually I don't see a reason for using both patches, because I can't
> > follow the point from Oliver that I've assumed a certain order of
> > disconnect.
> 
> I can't follow his point either.  Maybe he forgot that when the main 
> interface is disconnected the driver will release the SCO interface as 
> well.

I am going to submit my patch along with your altsettings patch for
inclusion now. So Greg don't have to worry about it.

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Alan Stern]

Oliver was right about not using interfaces after they have been released.
I think the best approach will be for the hci_usb_disconnect() routine to 
release both interfaces when it is called.  Something like this:

    In hci_usb.h, add to struct hci_usb:
	struct usb_interface		*acl_iface;

    In hci_usb_probe, add near the end:
	husb->acl_iface = intf;

    In hci_usb_disconnect, add:

	/* Always release both interfaces whenever we must release either */
	if (intf == husb->isoc_iface)
		usb_driver_release_interface(&hci_usb_driver,
				husb->acl_iface);
	else if (husb->isoc_iface)
		usb_driver_release_interface(&hci_usb_driver, 
				husb->isoc_iface);

It's not necessary to set the interface private pointers back to NULL, 
since the USB core will do that for you when the interface is released.

Alan Stern

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Alan,

> Oliver was right about not using interfaces after they have been released.
> I think the best approach will be for the hci_usb_disconnect() routine to 
> release both interfaces when it is called.  Something like this:
> 
>     In hci_usb.h, add to struct hci_usb:
> 	struct usb_interface		*acl_iface;
> 
>     In hci_usb_probe, add near the end:
> 	husb->acl_iface = intf;
> 
>     In hci_usb_disconnect, add:
> 
> 	/* Always release both interfaces whenever we must release either */
> 	if (intf == husb->isoc_iface)
> 		usb_driver_release_interface(&hci_usb_driver,
> 				husb->acl_iface);
> 	else if (husb->isoc_iface)
> 		usb_driver_release_interface(&hci_usb_driver, 
> 				husb->isoc_iface);
> 
> It's not necessary to set the interface private pointers back to NULL, 
> since the USB core will do that for you when the interface is released.

actually I was thinking about setting a flag that no more ISOC URB's are
submitted, because the device will still work if the second interface is
disconnected. The ISOC interface is only needed if an SCO link is opened
which would never happen when the Bluetooth chip is configured to route
the SCO traffic over an external PCM interface. Let me think a little
bit about the best way.

Regards

Marcel

Re: [PATCH] hci-usb bugfix

[Soeren Sonnenburg]

On Sun, 09 May 2004 19:47:15 +0000, Sebastian Schmidt wrote:

> Hi,
> 
> this is a little patch against 2.6.6-rc3 which fixes the Oops when
> unplugging an USB Bluetooth device.
> 
> hci_usb_disconnect() got called recursively which caused
> sysfs_hash_and_remove() finally to dereference a NULL pointer.
> 
> --- SNIP ---
> diff -uNr linux-2.6.6-rc3.old/drivers/bluetooth/hci_usb.c linux-2.6.6-rc3/drivers/bluetooth/hci_usb.c
> --- linux-2.6.6-rc3.old/drivers/bluetooth/hci_usb.c     2004-05-09 20:25:00.000000000 +0200
> +++ linux-2.6.6-rc3/drivers/bluetooth/hci_usb.c 2004-05-09 20:28:30.000000000 +0200
> @@ -986,8 +986,21 @@
> 
>         hci_usb_close(hdev);
> 
> -       if (husb->isoc_iface)
> +       if (husb->isoc_iface) {
> +#if 0
>                 usb_driver_release_interface(&hci_usb_driver, husb->isoc_iface);
> +#else
> +               /* do the same as usb_driver_release_interface would do,
> +                * except calling diconnect().
> +                * usb_driver_release_interface() _does_ check if
> +                *  are in disconnect() or add, but I really dunno 
> +                *  where dev->driver_list or dev->bus_list gets set.
> +                *      -- yath
> +                */
> +               husb->isoc_iface->dev.driver = NULL;
> +               usb_set_intfdata(husb->isoc_iface, NULL);
> +#endif
> +       }
> 
>         if (hci_unregister_dev(hdev) < 0)
>                 BT_ERR("Can't unregister HCI device %s", hdev->name);
> --- SNAP ---
> 
> Please apply it,
>

indeed it fixes this oops ... however I still get 

usb 2-1: USB disconnect, address 4
usb 2-1: new full speed USB device using address 5
devfs_mk_dev: could not append to parent for bluetooth/rfcomm/0
devfs_remove: bluetooth/rfcomm/0 not found, cannot remove
Call trace:
 [c00099c4] dump_stack+0x18/0x28
 [c010af1c] devfs_remove+0xcc/0xd0
 [c01e4a5c] tty_unregister_device+0x30/0x5c
 [f2080b64] rfcomm_dev_destruct+0x50/0xd4 [rfcomm]
 [f2081310] rfcomm_release_dev+0xf8/0x148 [rfcomm]
 [f20807a8] rfcomm_sock_ioctl+0x34/0x58 [rfcomm]
 [c02a8668] sock_ioctl+0xc0/0x2c0
 [c00726e0] sys_ioctl+0x100/0x318
 [c0005d60] ret_from_syscall+0x0/0x44
usb 2-1: USB disconnect, address 5

occasionally...

Soeren

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

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

Hi Soeren,

> indeed it fixes this oops ... however I still get 

please try the attached patch. It should fix it, too.

> usb 2-1: USB disconnect, address 4
> usb 2-1: new full speed USB device using address 5
> devfs_mk_dev: could not append to parent for bluetooth/rfcomm/0
> devfs_remove: bluetooth/rfcomm/0 not found, cannot remove
> Call trace:
>  [c00099c4] dump_stack+0x18/0x28
>  [c010af1c] devfs_remove+0xcc/0xd0
>  [c01e4a5c] tty_unregister_device+0x30/0x5c
>  [f2080b64] rfcomm_dev_destruct+0x50/0xd4 [rfcomm]
>  [f2081310] rfcomm_release_dev+0xf8/0x148 [rfcomm]
>  [f20807a8] rfcomm_sock_ioctl+0x34/0x58 [rfcomm]
>  [c02a8668] sock_ioctl+0xc0/0x2c0
>  [c00726e0] sys_ioctl+0x100/0x318
>  [c0005d60] ret_from_syscall+0x0/0x44
> usb 2-1: USB disconnect, address 5

Do you use DevFS? Does the same problem exists if you disable it and use
udev instead?

Regards

Marcel


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

===== drivers/bluetooth/hci_usb.c 1.49 vs edited =====
--- 1.49/drivers/bluetooth/hci_usb.c	Sat Apr 17 00:23:48 2004
+++ edited/drivers/bluetooth/hci_usb.c	Mon May 10 12:03:05 2004
@@ -976,11 +971,13 @@
 static void hci_usb_disconnect(struct usb_interface *intf)
 {
 	struct hci_usb *husb = usb_get_intfdata(intf);
-	struct hci_dev *hdev = husb->hdev;
+	struct hci_dev *hdev;
 
-	if (!husb)
+	if (!husb || intf == husb->isoc_iface)
 		return;
+
 	usb_set_intfdata(intf, NULL);
+	hdev = husb->hdev;
 
 	BT_DBG("%s", hdev->name);
 

Re: [PATCH] hci-usb bugfix

[Soeren Sonnenburg]

On Mon, 2004-05-10 at 12:40, Marcel Holtmann wrote:
> Hi Soeren,
> 
> > indeed it fixes this oops ... however I still get 
> 
> please try the attached patch. It should fix it, too.

well, at least I tried like 5 times plugin/out and it did not oops :)
hmhh why is the device address always incrementing ?

[...]
usb 1-1: USB disconnect, address 3
usb 1-1: new full speed USB device using address 4
usb 1-1: USB disconnect, address 4
usb 1-1: new full speed USB device using address 5
usb 1-1: USB disconnect, address 5
[...]

hmmhhh at least it works as far as I can tell now (the reproducable
oopses seem gone now)

> > usb 2-1: USB disconnect, address 4
> > usb 2-1: new full speed USB device using address 5
> > devfs_mk_dev: could not append to parent for bluetooth/rfcomm/0
> > devfs_remove: bluetooth/rfcomm/0 not found, cannot remove
> > Call trace:
> >  [c00099c4] dump_stack+0x18/0x28
> >  [c010af1c] devfs_remove+0xcc/0xd0
> >  [c01e4a5c] tty_unregister_device+0x30/0x5c
> >  [f2080b64] rfcomm_dev_destruct+0x50/0xd4 [rfcomm]
> >  [f2081310] rfcomm_release_dev+0xf8/0x148 [rfcomm]
> >  [f20807a8] rfcomm_sock_ioctl+0x34/0x58 [rfcomm]
> >  [c02a8668] sock_ioctl+0xc0/0x2c0
> >  [c00726e0] sys_ioctl+0x100/0x318
> >  [c0005d60] ret_from_syscall+0x0/0x44
> > usb 2-1: USB disconnect, address 5
> 
> Do you use DevFS? Does the same problem exists if you disable it and use
> udev instead?

ok, so I went to udev and also no oops so far... 

Success!

Thanks!
Soeren.

Re: [PATCH] hci-usb bugfix

[Marcel Holtmann]

Hi Soeren,

> well, at least I tried like 5 times plugin/out and it did not oops :)

this is good news.

> hmhh why is the device address always incrementing ?

Ask the USB guys. I don't know it.

> > > usb 2-1: USB disconnect, address 4
> > > usb 2-1: new full speed USB device using address 5
> > > devfs_mk_dev: could not append to parent for bluetooth/rfcomm/0
> > > devfs_remove: bluetooth/rfcomm/0 not found, cannot remove
> > > Call trace:
> > >  [c00099c4] dump_stack+0x18/0x28
> > >  [c010af1c] devfs_remove+0xcc/0xd0
> > >  [c01e4a5c] tty_unregister_device+0x30/0x5c
> > >  [f2080b64] rfcomm_dev_destruct+0x50/0xd4 [rfcomm]
> > >  [f2081310] rfcomm_release_dev+0xf8/0x148 [rfcomm]
> > >  [f20807a8] rfcomm_sock_ioctl+0x34/0x58 [rfcomm]
> > >  [c02a8668] sock_ioctl+0xc0/0x2c0
> > >  [c00726e0] sys_ioctl+0x100/0x318
> > >  [c0005d60] ret_from_syscall+0x0/0x44
> > > usb 2-1: USB disconnect, address 5
> > 
> > Do you use DevFS? Does the same problem exists if you disable it and use
> > udev instead?
> 
> ok, so I went to udev and also no oops so far... 

If you still want to use DevFS you have to investigate by yourself,
because DevFS is obsolete and I don't really care about it.

Regards

Marcel