btusb has some races in disconnect()
- it doesn't deal with only the iso interface disconnected
- it releases the data interface before unregistering the iso interface
- it isn't carefull to kill all URBs in case of soft disconnect
Signed-off-by: Oliver Neukum <[email protected]>
Regards
Oliver
---
--- linux-2.6.27-rc3/drivers/bluetooth/btusb.c.marcel 2008-08-19 10:21:51.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c 2008-08-19 14:04:49.000000000 +0200
@@ -169,6 +169,7 @@ static struct usb_device_id blacklist_ta
struct btusb_data {
struct hci_dev *hdev;
struct usb_device *udev;
+ struct usb_interface *acl;
struct usb_interface *isoc;
spinlock_t lock;
@@ -523,6 +524,14 @@ static int btusb_open(struct hci_dev *hd
return err;
}
+static void btusb_stop_traffic(struct btusb_data *data)
+{
+ usb_kill_anchored_urbs(&data->intr_anchor);
+ usb_kill_anchored_urbs(&data->bulk_anchor);
+ usb_kill_anchored_urbs(&data->isoc_anchor);
+ usb_kill_anchored_urbs(&data->tx_anchor);
+}
+
static int btusb_close(struct hci_dev *hdev)
{
struct btusb_data *data = hdev->driver_data;
@@ -535,13 +544,9 @@ static int btusb_close(struct hci_dev *h
flush_work(&data->work);
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
-
clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->bulk_anchor);
-
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
+ btusb_stop_traffic(data);
return 0;
}
@@ -822,6 +827,7 @@ static int btusb_probe(struct usb_interf
}
data->udev = interface_to_usbdev(intf);
+ data->acl = intf;
spin_lock_init(&data->lock);
@@ -890,7 +896,7 @@ static int btusb_probe(struct usb_interf
if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
- data->isoc, NULL);
+ data->isoc, data);
if (err < 0) {
hci_free_dev(hdev);
kfree(data);
@@ -922,13 +928,21 @@ static void btusb_disconnect(struct usb_
hdev = data->hdev;
+ usb_set_intfdata(data->acl, NULL);
if (data->isoc)
- usb_driver_release_interface(&btusb_driver, data->isoc);
-
- usb_set_intfdata(intf, NULL);
+ usb_set_intfdata(data->isoc, NULL);
+ /* unregister before releasing any interface */
hci_unregister_dev(hdev);
+ cancel_work_sync(&data->work);
+ btusb_stop_traffic(data);
+
+ if (intf == data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->acl);
+ else if (data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->isoc);
+
hci_free_dev(hdev);
}
Am Dienstag 19 August 2008 17:27:23 schrieb Marcel Holtmann:
> > > I am not sure about hci_dev_hold. We ensure that a reference count to
> > > the module is hold when the HCI device is brought up.
> >
> > But won't the module be marked as going away and make try_module_get()
> > fail?
>
> when calling "hciconfig hci0 up" then it is impossible to unload the
> btusb module since we keep a module reference.
There's no guarantee the hci is up when disconnect() is called.
Regards
Oliver
Hi Oliver,
> > > > > First you release the interface, then you unregister the hci.
> > > >
> > > > Putting it after hci_unregister_dev is an issue. The unregister will
> > > > call __hci_dev_put and then in return will call the destruct callback
> > > > which will free the data pointer.
> > > >
> > > > So either we use hci_dev_hold or make sure that disconnect can only
> > > > disable the SCO interface. That comes directly with the first comment
> > > > from you.
> > >
> > > Here is a new version. I am afraid normal hci_dev_hold would fail if
> > > the disconnect happens because the btusb module is being unloaded.
> >
> > the btusb_stop_traffic change is bogus. You don't need that. The flush
> > callback will be called to ensure that all TX traffic is released.
>
> OK.
>
> > I am not sure about hci_dev_hold. We ensure that a reference count to
> > the module is hold when the HCI device is brought up.
>
> But won't the module be marked as going away and make try_module_get()
> fail?
when calling "hciconfig hci0 up" then it is impossible to unload the
btusb module since we keep a module reference.
I am thinking that just __set_isoc_interface(..., 0) when disconnect
gets called for the isoc interface and the releasing the interface is
the more cleaner way. I have to think about this.
Regards
Marcel
Am Dienstag 19 August 2008 16:31:18 schrieb Marcel Holtmann:
> Hi Oliver,
>
> > > > First you release the interface, then you unregister the hci.
> > >
> > > Putting it after hci_unregister_dev is an issue. The unregister will
> > > call __hci_dev_put and then in return will call the destruct callback
> > > which will free the data pointer.
> > >
> > > So either we use hci_dev_hold or make sure that disconnect can only
> > > disable the SCO interface. That comes directly with the first comment
> > > from you.
> >
> > Here is a new version. I am afraid normal hci_dev_hold would fail if
> > the disconnect happens because the btusb module is being unloaded.
>
> the btusb_stop_traffic change is bogus. You don't need that. The flush
> callback will be called to ensure that all TX traffic is released.
OK.
> I am not sure about hci_dev_hold. We ensure that a reference count to
> the module is hold when the HCI device is brought up.
But won't the module be marked as going away and make try_module_get()
fail?
Regards
Oliver
Signed-off-by: Oliver Neukum <[email protected]>
---
--- linux-2.6.27-rc3/drivers/bluetooth/btusb.c.marcel 2008-08-19 10:21:51.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c 2008-08-19 15:50:06.000000000 +0200
@@ -169,6 +169,7 @@ static struct usb_device_id blacklist_ta
struct btusb_data {
struct hci_dev *hdev;
struct usb_device *udev;
+ struct usb_interface *acl;
struct usb_interface *isoc;
spinlock_t lock;
@@ -822,6 +827,7 @@ static int btusb_probe(struct usb_interf
}
data->udev = interface_to_usbdev(intf);
+ data->acl = intf;
spin_lock_init(&data->lock);
@@ -890,7 +896,7 @@ static int btusb_probe(struct usb_interf
if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
- data->isoc, NULL);
+ data->isoc, data);
if (err < 0) {
hci_free_dev(hdev);
kfree(data);
@@ -922,13 +928,23 @@ static void btusb_disconnect(struct usb_
hdev = data->hdev;
- if (data->isoc)
- usb_driver_release_interface(&btusb_driver, data->isoc);
+ /* make sure we have a reference */
+ __hci_dev_hold(hdev);
- usb_set_intfdata(intf, NULL);
+ usb_set_intfdata(data->acl, NULL);
+ if (data->isoc)
+ usb_set_intfdata(data->isoc, NULL);
+ /* unregister before releasing any interface */
hci_unregister_dev(hdev);
+ if (intf == data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->acl);
+ else if (data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->isoc);
+
+ /* release the reference */
+ __hci_dev_put(hdev);
hci_free_dev(hdev);
}
Hi Oliver,
> > > First you release the interface, then you unregister the hci.
> >
> > Putting it after hci_unregister_dev is an issue. The unregister will
> > call __hci_dev_put and then in return will call the destruct callback
> > which will free the data pointer.
> >
> > So either we use hci_dev_hold or make sure that disconnect can only
> > disable the SCO interface. That comes directly with the first comment
> > from you.
>
> Here is a new version. I am afraid normal hci_dev_hold would fail if
> the disconnect happens because the btusb module is being unloaded.
the btusb_stop_traffic change is bogus. You don't need that. The flush
callback will be called to ensure that all TX traffic is released.
I am not sure about hci_dev_hold. We ensure that a reference count to
the module is hold when the HCI device is brought up.
Regards
Marcel
Am Dienstag 19 August 2008 15:28:26 schrieb Marcel Holtmann:
> > First you release the interface, then you unregister the hci.
>
> Putting it after hci_unregister_dev is an issue. The unregister will
> call __hci_dev_put and then in return will call the destruct callback
> which will free the data pointer.
>
> So either we use hci_dev_hold or make sure that disconnect can only
> disable the SCO interface. That comes directly with the first comment
> from you.
Here is a new version. I am afraid normal hci_dev_hold would fail if
the disconnect happens because the btusb module is being unloaded.
Regards
Oliver
Signed-off-by: Oliver Neukum <[email protected]>
----
--- linux-2.6.27-rc3/drivers/bluetooth/btusb.c.marcel 2008-08-19 10:21:51.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c 2008-08-19 15:50:06.000000000 +0200
@@ -169,6 +169,7 @@ static struct usb_device_id blacklist_ta
struct btusb_data {
struct hci_dev *hdev;
struct usb_device *udev;
+ struct usb_interface *acl;
struct usb_interface *isoc;
spinlock_t lock;
@@ -523,6 +524,14 @@ static int btusb_open(struct hci_dev *hd
return err;
}
+static void btusb_stop_traffic(struct btusb_data *data)
+{
+ usb_kill_anchored_urbs(&data->intr_anchor);
+ usb_kill_anchored_urbs(&data->bulk_anchor);
+ usb_kill_anchored_urbs(&data->isoc_anchor);
+ usb_kill_anchored_urbs(&data->tx_anchor);
+}
+
static int btusb_close(struct hci_dev *hdev)
{
struct btusb_data *data = hdev->driver_data;
@@ -535,13 +544,9 @@ static int btusb_close(struct hci_dev *h
flush_work(&data->work);
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
-
clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->bulk_anchor);
-
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
+ btusb_stop_traffic(data);
return 0;
}
@@ -822,6 +827,7 @@ static int btusb_probe(struct usb_interf
}
data->udev = interface_to_usbdev(intf);
+ data->acl = intf;
spin_lock_init(&data->lock);
@@ -890,7 +896,7 @@ static int btusb_probe(struct usb_interf
if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
- data->isoc, NULL);
+ data->isoc, data);
if (err < 0) {
hci_free_dev(hdev);
kfree(data);
@@ -922,13 +928,23 @@ static void btusb_disconnect(struct usb_
hdev = data->hdev;
- if (data->isoc)
- usb_driver_release_interface(&btusb_driver, data->isoc);
+ /* make sure we have a reference */
+ __hci_dev_hold(hdev);
- usb_set_intfdata(intf, NULL);
+ usb_set_intfdata(data->acl, NULL);
+ if (data->isoc)
+ usb_set_intfdata(data->isoc, NULL);
+ /* unregister before releasing any interface */
hci_unregister_dev(hdev);
+ if (intf == data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->acl);
+ else if (data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->isoc);
+
+ /* release the reference */
+ __hci_dev_put(hdev);
hci_free_dev(hdev);
}
Am Dienstag 19 August 2008 15:28:26 schrieb Marcel Holtmann:
Hello,
> Hi Oliver,
>
> > > > btusb has some races in disconnect()
> > > >
> > > > - it doesn't deal with only the iso interface disconnected
> > >
> > > It see the point here, but then we have to release the main interface
> > > cleanly or just disable SCO. I would prefer to just disable SCO.
> >
> > The patch disables the main interface. Just disabling SCO means that
> > btusb_data.isoc could no longer be treated as constant and locking for it
> > would be necessary. This seems extravagant for a minor use case.
>
> I am fine with both ways. Will decide which code looks cleaner :)
>
> > > > - it releases the data interface before unregistering the iso interface
> > >
> > > How does this happen? It doesn't do it that way.
> >
> > From your version:
> > if (data->isoc)
> > usb_driver_release_interface(&btusb_driver, data->isoc);
> >
> > usb_set_intfdata(intf, NULL);
> >
> > hci_unregister_dev(hdev);
> >
> > First you release the interface, then you unregister the hci.
>
> Putting it after hci_unregister_dev is an issue. The unregister will
> call __hci_dev_put and then in return will call the destruct callback
> which will free the data pointer.
>
> So either we use hci_dev_hold or make sure that disconnect can only
> disable the SCO interface. That comes directly with the first comment
> from you.
I'll make a patch with hci_dev_hold. It seems the cleaner solution.
Regards
Oliver
Hi Oliver,
> > > btusb has some races in disconnect()
> > >
> > > - it doesn't deal with only the iso interface disconnected
> >
> > It see the point here, but then we have to release the main interface
> > cleanly or just disable SCO. I would prefer to just disable SCO.
>
> The patch disables the main interface. Just disabling SCO means that
> btusb_data.isoc could no longer be treated as constant and locking for it
> would be necessary. This seems extravagant for a minor use case.
I am fine with both ways. Will decide which code looks cleaner :)
> > > - it releases the data interface before unregistering the iso interface
> >
> > How does this happen? It doesn't do it that way.
>
> From your version:
> if (data->isoc)
> usb_driver_release_interface(&btusb_driver, data->isoc);
>
> usb_set_intfdata(intf, NULL);
>
> hci_unregister_dev(hdev);
>
> First you release the interface, then you unregister the hci.
Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.
So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.
Regards
Marcel
Am Dienstag 19 August 2008 14:43:01 schrieb Marcel Holtmann:
> Hi Oliver,
>=20
> > btusb has some races in disconnect()
> >=20
> > - it doesn't deal with only the iso interface disconnected
>=20
> It see the point here, but then we have to release the main interface
> cleanly or just disable SCO. I would prefer to just disable SCO.
The patch disables the main interface. Just disabling SCO means that
btusb_data.isoc could no longer be treated as constant and locking for it
would be necessary. This seems extravagant for a minor use case.
> > - it releases the data interface before unregistering the iso interface
>=20
> How does this happen? It doesn't do it that way.
=46rom your version:
if (data->isoc)
usb_driver_release_interface(&btusb_driver, data->isoc);
usb_set_intfdata(intf, NULL);
hci_unregister_dev(hdev);
=46irst you release the interface, then you unregister the hci.
>=20
> > - it isn't carefull to kill all URBs in case of soft disconnect
>=20
> The hci_unregister_dev will be calling the close() callback. Check
> hci_core.c and hci_dev_do_close().
OK, I'll modify the patch.
Regards
Oliver
Hi Oliver,
> btusb has some races in disconnect()
>
> - it doesn't deal with only the iso interface disconnected
It see the point here, but then we have to release the main interface
cleanly or just disable SCO. I would prefer to just disable SCO.
> - it releases the data interface before unregistering the iso interface
How does this happen? It doesn't do it that way.
> - it isn't carefull to kill all URBs in case of soft disconnect
The hci_unregister_dev will be calling the close() callback. Check
hci_core.c and hci_dev_do_close().
Regards
Marcel