Return-Path: Date: Mon, 24 Aug 2009 14:58:38 -0700 From: Sarah Sharp To: Oliver Neukum Cc: Marcel Holtmann , linux-bluetooth@vger.kernel.org, linux-usb@vger.kernel.org, Arjan Van De Ven , saharabeara@gmail.com Subject: Re: btusb autosuspend and circular lock dep Message-ID: <20090824215838.GA9991@gamba.jf.intel.com> References: <20090615175435.GA4772@gamba.jf.intel.com> <200908242326.01149.oliver@neukum.org> <1251149817.2950.72.camel@localhost.localdomain> <200908242344.59861.oliver@neukum.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <200908242344.59861.oliver@neukum.org> List-ID: Hi Oliver, I just tested this patch (against 2.6.31-rc7) on my X200s laptop, and it works fine with the Broadcom USB bluetooth device with VID:PID 0a5c:2145. I tested transmitting audio to a bluetooth headset, so the isoc transfers to work fine. ISTR you were concerned about them. I'm still testing on my T61 (Broadcom 0a5c:2110). The autosuspend patch seems to work fine, but I'm getting a circular lock dependency warning. I'm recompiling a vanilla 2.6.31-rc7 to see if the lock warning is still there. Sarah Sharp On Mon, Aug 24, 2009 at 11:44:59PM +0200, Oliver Neukum wrote: > This patch adds support of USB autosuspend to the btusb driver > > If the device doesn't support remote wakeup, simple support based > on up/down is provided. If the device supports remote wakeup, > additional support for autosuspend while the interface is up is provided. > This is done by queueing URBs in an anchor structure and waking the > device up from a work queue on sending. Reception triggers remote > wakeup. > The last busy facility of the USB autosuspend code is used and > to close a race between autosuspend and transmission a counter > of ongoing transmissions is maintained. > #ifdefs for CONFIG_PM are added as necessary. > > Signed-off-by: Oliver Neukum > > -- > > --- 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 > - btusb_submit_bulk_urb(hdev, GFP_NOIO); > + goto failed; > + } > + > + 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) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html