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: <200908241149.34224.oliver@neukum.org> References: <20090615175435.GA4772@gamba.jf.intel.com> <200906210034.42481.oliver@neukum.org> <1251102046.2950.28.camel@localhost.localdomain> <200908241149.34224.oliver@neukum.org> Content-Type: text/plain Date: Mon, 24 Aug 2009 03:16:15 -0700 Message-Id: <1251108975.2950.37.camel@localhost.localdomain> Mime-Version: 1.0 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Oliver, > Signed-off-by: Oliver Neukum > > 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, 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. > > -- > > --- 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,24 @@ 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) please remove the extra empty line. > { > struct hci_dev *hdev = urb->context; > @@ -202,6 +223,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 +349,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 +488,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 +540,16 @@ 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; > + Please add an extra empty line before data->intf->needs... > if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) > - return 0; > + goto out; > > if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags)) > - return 0; > + goto out; > > 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); > > +out: > + usb_autopm_put_interface(data->intf); > return 0; Please call the out label done instead. I prefer to be consistent inside the driver. > 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,15 @@ 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) { > + data->intf->needs_remote_wakeup = 0; > + usb_autopm_put_interface(data->intf); > + } Please do this like this: 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 +690,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 +701,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 out; > + } > + > +skip_waking: > usb_anchor_urb(urb, &data->tx_anchor); > > err = usb_submit_urb(urb, GFP_ATOMIC); > @@ -646,10 +723,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); > > +out: > return err; > } Please call the labels simply skip and done. > @@ -721,8 +801,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) { > + data->did_iso_resume = 1; > + } else { > + clear_bit(BTUSB_ISOC_RUNNING, &data->flags); > + usb_kill_anchored_urbs(&data->isoc_anchor); > + return; > + } Having this as like this is simpler to read: 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 +833,23 @@ 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) > + usb_autopm_put_interface(data->intf); > +} > + Same as above. Please do it like this: 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 +919,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 +1051,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 +1061,45 @@ 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; > + else > + data->tx_in_flight++; The else part is totally point less here; 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 +1107,13 @@ static int btusb_resume(struct usb_interface *intf) > return 0; > > if (!test_bit(HCI_RUNNING, &hdev->flags)) > - return 0; > + goto no_io_needed; > > 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 err_out; > } > } > > @@ -989,9 +1121,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 err_out; > + } else { > btusb_submit_bulk_urb(hdev, GFP_NOIO); > + } > } > > if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) { > @@ -1001,16 +1134,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; > + > +err_out: > + usb_scuttle_anchored_urbs(&data->deferred); > +no_io_needed: > + spin_lock_irq(&data->txlock); > + clear_bit(BTUSB_SUSPENDING, &data->flags); > + spin_unlock_irq(&data->txlock); > + > + return err; > } > +#endif Please call this labels failed and done to be more consistent. > > 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