Return-Path: From: Oliver Neukum To: Marcel Holtmann Subject: Re: btusb autosuspend and circular lock dep Date: Mon, 24 Aug 2009 21:49:51 +0200 Cc: linux-bluetooth@vger.kernel.org, linux-usb@vger.kernel.org, Sarah Sharp , Arjan Van De Ven , saharabeara@gmail.com References: <20090615175435.GA4772@gamba.jf.intel.com> <200908241559.59814.oliver@neukum.org> <1251133391.2950.51.camel@localhost.localdomain> In-Reply-To: <1251133391.2950.51.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Message-Id: <200908242149.51588.oliver@neukum.org> List-ID: Am Montag, 24. August 2009 19:03:11 schrieb Marcel Holtmann: > Hi Oliver, > > > > > > I see. I removed that case and re-tested. The simple down, auto, > > > > > up, down test worked fine. However, the down, up, wait, auto test > > > > > failed. Logs of the success and the failure are attached. > > > > > > > > Very well, this version works for me. > > > > > > can you send a clean version so we can merge this into 2.6.32. This > > > version still has the debug details in there. And please send it to the > > > mailing list for reference. And again. > > + if (!err < 0) > > + return; > > This looks like a typo. Fixed > > > @@ -989,9 +1123,10 @@ static int btusb_resume(struct usb_interface *intf) > > err = btusb_submit_bulk_urb(hdev, GFP_NOIO); > > if (err < 0) { > > clear_bit(BTUSB_BULK_RUNNING, &data->flags); > > - return err; > > - } else > > + goto failed; > > + } else { > > btusb_submit_bulk_urb(hdev, GFP_NOIO); > > + } > > } > > Normally we don't put braces around single else or if statement. Quoting CodingStyle.txt: This does not apply if one branch of a conditional statement is a single statement. Use braces in both branches. if (condition) { do_this(); do_that(); } else { otherwise(); } Signed-off-by: Oliver Neukum -- diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index e70c57e..8be1471 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -35,7 +35,7 @@ #include #include -#define VERSION "0.5" +#define VERSION "0.6" static int ignore_dga; static int ignore_csr; @@ -145,6 +145,7 @@ static struct usb_device_id blacklist_table[] = { #define BTUSB_INTR_RUNNING 0 #define BTUSB_BULK_RUNNING 1 #define BTUSB_ISOC_RUNNING 2 +#define BTUSB_SUSPENDING 3 struct btusb_data { struct hci_dev *hdev; @@ -157,11 +158,15 @@ struct btusb_data { unsigned long flags; struct work_struct work; + struct work_struct waker; struct usb_anchor tx_anchor; struct usb_anchor intr_anchor; struct usb_anchor bulk_anchor; struct usb_anchor isoc_anchor; + struct usb_anchor deferred; + int tx_in_flight; + spinlock_t txlock; struct usb_endpoint_descriptor *intr_ep; struct usb_endpoint_descriptor *bulk_tx_ep; @@ -174,8 +179,23 @@ struct btusb_data { unsigned int sco_num; int isoc_altsetting; int suspend_count; + int did_iso_resume:1; }; +static int inc_tx(struct btusb_data *data) +{ + unsigned long flags; + int rv; + + spin_lock_irqsave(&data->txlock, flags); + rv = test_bit(BTUSB_SUSPENDING, &data->flags); + if (!rv) + data->tx_in_flight++; + spin_unlock_irqrestore(&data->txlock, flags); + + return rv; +} + static void btusb_intr_complete(struct urb *urb) { struct hci_dev *hdev = urb->context; @@ -202,6 +222,7 @@ static void btusb_intr_complete(struct urb *urb) if (!test_bit(BTUSB_INTR_RUNNING, &data->flags)) return; + usb_mark_last_busy(data->udev); usb_anchor_urb(urb, &data->intr_anchor); err = usb_submit_urb(urb, GFP_ATOMIC); @@ -327,6 +348,7 @@ static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags) urb->transfer_flags |= URB_FREE_BUFFER; + usb_mark_last_busy(data->udev); usb_anchor_urb(urb, &data->bulk_anchor); err = usb_submit_urb(urb, mem_flags); @@ -465,6 +487,33 @@ static void btusb_tx_complete(struct urb *urb) { struct sk_buff *skb = urb->context; struct hci_dev *hdev = (struct hci_dev *) skb->dev; + struct btusb_data *data = hdev->driver_data; + + BT_DBG("%s urb %p status %d count %d", hdev->name, + urb, urb->status, urb->actual_length); + + if (!test_bit(HCI_RUNNING, &hdev->flags)) + goto done; + + if (!urb->status) + hdev->stat.byte_tx += urb->transfer_buffer_length; + else + hdev->stat.err_tx++; + +done: + spin_lock(&data->txlock); + data->tx_in_flight--; + spin_unlock(&data->txlock); + + kfree(urb->setup_packet); + + kfree_skb(skb); +} + +static void btusb_isoc_tx_complete(struct urb *urb) +{ + struct sk_buff *skb = urb->context; + struct hci_dev *hdev = (struct hci_dev *) skb->dev; BT_DBG("%s urb %p status %d count %d", hdev->name, urb, urb->status, urb->actual_length); @@ -490,11 +539,17 @@ static int btusb_open(struct hci_dev *hdev) BT_DBG("%s", hdev->name); + err = usb_autopm_get_interface(data->intf); + if (err < 0) + return err; + + data->intf->needs_remote_wakeup = 1; + if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) - return 0; + goto done; if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags)) - return 0; + goto done; err = btusb_submit_intr_urb(hdev, GFP_KERNEL); if (err < 0) @@ -509,17 +564,28 @@ static int btusb_open(struct hci_dev *hdev) set_bit(BTUSB_BULK_RUNNING, &data->flags); btusb_submit_bulk_urb(hdev, GFP_KERNEL); +done: + usb_autopm_put_interface(data->intf); return 0; failed: clear_bit(BTUSB_INTR_RUNNING, &data->flags); clear_bit(HCI_RUNNING, &hdev->flags); + usb_autopm_put_interface(data->intf); 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); +} + static int btusb_close(struct hci_dev *hdev) { struct btusb_data *data = hdev->driver_data; + int err; BT_DBG("%s", hdev->name); @@ -529,13 +595,16 @@ static int btusb_close(struct hci_dev *hdev) cancel_work_sync(&data->work); clear_bit(BTUSB_ISOC_RUNNING, &data->flags); - usb_kill_anchored_urbs(&data->isoc_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); + err = usb_autopm_get_interface(data->intf); + if (err < 0) + return 0; + + data->intf->needs_remote_wakeup = 0; + usb_autopm_put_interface(data->intf); return 0; } @@ -622,7 +691,7 @@ static int btusb_send_frame(struct sk_buff *skb) urb->dev = data->udev; urb->pipe = pipe; urb->context = skb; - urb->complete = btusb_tx_complete; + urb->complete = btusb_isoc_tx_complete; urb->interval = data->isoc_tx_ep->bInterval; urb->transfer_flags = URB_ISO_ASAP; @@ -633,12 +702,21 @@ static int btusb_send_frame(struct sk_buff *skb) le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize)); hdev->stat.sco_tx++; - break; + goto skip_waking; default: return -EILSEQ; } + err = inc_tx(data); + if (err) { + usb_anchor_urb(urb, &data->deferred); + schedule_work(&data->waker); + err = 0; + goto done; + } + +skip_waking: usb_anchor_urb(urb, &data->tx_anchor); err = usb_submit_urb(urb, GFP_ATOMIC); @@ -646,10 +724,13 @@ static int btusb_send_frame(struct sk_buff *skb) BT_ERR("%s urb %p submission failed", hdev->name, urb); kfree(urb->setup_packet); usb_unanchor_urb(urb); + } else { + usb_mark_last_busy(data->udev); } usb_free_urb(urb); +done: return err; } @@ -721,8 +802,19 @@ static void btusb_work(struct work_struct *work) { struct btusb_data *data = container_of(work, struct btusb_data, work); struct hci_dev *hdev = data->hdev; + int err; if (hdev->conn_hash.sco_num > 0) { + if (!data->did_iso_resume) { + err = usb_autopm_get_interface(data->isoc); + if (err < 0) { + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); + usb_kill_anchored_urbs(&data->isoc_anchor); + return; + } + + data->did_iso_resume = 1; + } if (data->isoc_altsetting != 2) { clear_bit(BTUSB_ISOC_RUNNING, &data->flags); usb_kill_anchored_urbs(&data->isoc_anchor); @@ -742,9 +834,25 @@ static void btusb_work(struct work_struct *work) usb_kill_anchored_urbs(&data->isoc_anchor); __set_isoc_interface(hdev, 0); + if (data->did_iso_resume) { + data->did_iso_resume = 0; + usb_autopm_put_interface(data->isoc); + } } } +static void btusb_waker(struct work_struct *work) +{ + struct btusb_data *data = container_of(work, struct btusb_data, waker); + int err; + + err = usb_autopm_get_interface(data->intf); + if (err < 0) + return; + + usb_autopm_put_interface(data->intf); +} + static int btusb_probe(struct usb_interface *intf, const struct usb_device_id *id) { @@ -814,11 +922,14 @@ static int btusb_probe(struct usb_interface *intf, spin_lock_init(&data->lock); INIT_WORK(&data->work, btusb_work); + INIT_WORK(&data->waker, btusb_waker); + spin_lock_init(&data->txlock); init_usb_anchor(&data->tx_anchor); init_usb_anchor(&data->intr_anchor); init_usb_anchor(&data->bulk_anchor); init_usb_anchor(&data->isoc_anchor); + init_usb_anchor(&data->deferred); hdev = hci_alloc_dev(); if (!hdev) { @@ -943,6 +1054,7 @@ static void btusb_disconnect(struct usb_interface *intf) hci_free_dev(hdev); } +#ifdef CONFIG_PM static int btusb_suspend(struct usb_interface *intf, pm_message_t message) { struct btusb_data *data = usb_get_intfdata(intf); @@ -952,22 +1064,44 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) if (data->suspend_count++) return 0; + spin_lock_irq(&data->txlock); + if (!(interface_to_usbdev(intf)->auto_pm && data->tx_in_flight)) { + set_bit(BTUSB_SUSPENDING, &data->flags); + spin_unlock_irq(&data->txlock); + } else { + spin_unlock_irq(&data->txlock); + data->suspend_count--; + return -EBUSY; + } + cancel_work_sync(&data->work); + btusb_stop_traffic(data); usb_kill_anchored_urbs(&data->tx_anchor); - usb_kill_anchored_urbs(&data->isoc_anchor); - usb_kill_anchored_urbs(&data->bulk_anchor); - usb_kill_anchored_urbs(&data->intr_anchor); - return 0; } +static void play_deferred(struct btusb_data *data) +{ + struct urb *urb; + int err; + + while ((urb = usb_get_from_anchor(&data->deferred))) { + err = usb_submit_urb(urb, GFP_ATOMIC); + if (err < 0) + break; + + data->tx_in_flight++; + } + usb_scuttle_anchored_urbs(&data->deferred); +} + static int btusb_resume(struct usb_interface *intf) { struct btusb_data *data = usb_get_intfdata(intf); struct hci_dev *hdev = data->hdev; - int err; + int err = 0; BT_DBG("intf %p", intf); @@ -975,13 +1109,13 @@ static int btusb_resume(struct usb_interface *intf) return 0; if (!test_bit(HCI_RUNNING, &hdev->flags)) - return 0; + goto done; if (test_bit(BTUSB_INTR_RUNNING, &data->flags)) { err = btusb_submit_intr_urb(hdev, GFP_NOIO); if (err < 0) { clear_bit(BTUSB_INTR_RUNNING, &data->flags); - return err; + goto failed; } } @@ -989,9 +1123,10 @@ static int btusb_resume(struct usb_interface *intf) err = btusb_submit_bulk_urb(hdev, GFP_NOIO); if (err < 0) { clear_bit(BTUSB_BULK_RUNNING, &data->flags); - return err; - } else + goto failed; + } else { btusb_submit_bulk_urb(hdev, GFP_NOIO); + } } if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) { @@ -1001,16 +1136,35 @@ static int btusb_resume(struct usb_interface *intf) btusb_submit_isoc_urb(hdev, GFP_NOIO); } + spin_lock_irq(&data->txlock); + play_deferred(data); + clear_bit(BTUSB_SUSPENDING, &data->flags); + spin_unlock_irq(&data->txlock); + schedule_work(&data->work); + return 0; + +failed: + usb_scuttle_anchored_urbs(&data->deferred); +done: + spin_lock_irq(&data->txlock); + clear_bit(BTUSB_SUSPENDING, &data->flags); + spin_unlock_irq(&data->txlock); + + return err; } +#endif static struct usb_driver btusb_driver = { .name = "btusb", .probe = btusb_probe, .disconnect = btusb_disconnect, +#ifdef CONFIG_PM .suspend = btusb_suspend, .resume = btusb_resume, +#endif .id_table = btusb_table, + .supports_autosuspend = 1, }; static int __init btusb_init(void)