Hi Oliver,
> Signed-off-by: Oliver Neukum <oliver@xxxxxxxxxx>
>
> 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 <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
