Return-Path: From: Oliver Neukum To: Marcel Holtmann Subject: Re: [patch]race condition in btusb disconnect() handling Date: Tue, 19 Aug 2008 17:14:49 +0200 Cc: linux-bluetooth@vger.kernel.org, linux-usb@vger.kernel.org References: <200808191423.52657.oliver@neukum.org> <200808191605.40987.oliver@neukum.org> <1219156278.7591.145.camel@violet.holtmann.net> In-Reply-To: <1219156278.7591.145.camel@violet.holtmann.net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Message-Id: <200808191714.50309.oliver@neukum.org> List-ID: 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 --- --- 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); }