Return-Path: Subject: Re: btusb autosuspend and circular lock dep From: Marcel Holtmann To: Oliver Neukum Cc: linux-bluetooth@vger.kernel.org, linux-usb@vger.kernel.org, Sarah Sharp , Arjan Van De Ven , saharabeara@gmail.com In-Reply-To: <200908241559.59814.oliver@neukum.org> References: <20090615175435.GA4772@gamba.jf.intel.com> <200906210034.42481.oliver@neukum.org> <1251102046.2950.28.camel@localhost.localdomain> <200908241559.59814.oliver@neukum.org> Content-Type: text/plain Date: Mon, 24 Aug 2009 10:03:11 -0700 Message-Id: <1251133391.2950.51.camel@localhost.localdomain> Mime-Version: 1.0 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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. > > Here it is again? Do you still want skip_waking to be replaced with skip ? no. That is just fine. However ... > --- 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; This looks like a typo. > + > + 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); > + } > } Normally we don't put braces around single else or if statement. > 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) > Regards Marcel