2009-08-24 13:59:58

by Oliver Neukum

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

Am Montag, 24. August 2009 10:20:46 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.

Here it is again? Do you still want skip_waking to be replaced with skip ?

Regards
Oliver

Signed-off-by: Oliver Neukum <[email protected]>

--

--- 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,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)


2009-08-24 23:30:42

by Marcel Holtmann

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

Hi Oliver,

> 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.

actually git-am complaint about some whitespaces, but I fixed that
manually. Thanks for the patch.

Regards

Marcel



2009-08-24 23:07:11

by Marcel Holtmann

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

Hi Sara,

> > 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.
>
> The lock dep warning is still present in 2.6.31-rc7, so it's not your
> auto-suspend patch. The dmesg is attached. The warning came right
> after I stopped mplayer streaming audio to the bluetooth headset (with
> CTRL+c). I'm running an older gnome bluetooth stack from June 14
> (from commit 7b1f2782). I haven't tested whether updating the userspace
> bits makes the lock dep warning go way.

can you try the bluetooth-testing.git tree from kernel.org with the
patch from Oliver. The SCO shutdown patch might fix this.

Regards

Marcel



2009-08-24 22:49:24

by Sarah Sharp

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

On Mon, Aug 24, 2009 at 02:58:38PM -0700, Sarah Sharp wrote:
> 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.

The lock dep warning is still present in 2.6.31-rc7, so it's not your
auto-suspend patch. The dmesg is attached. The warning came right
after I stopped mplayer streaming audio to the bluetooth headset (with
CTRL+c). I'm running an older gnome bluetooth stack from June 14
(from commit 7b1f2782). I haven't tested whether updating the userspace
bits makes the lock dep warning go way.

Sarah

> 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 <[email protected]>
> >
> > --
> >
> > --- 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,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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


Attachments:
(No filename) (13.83 kB)
btusb-lockdep-warn-vanilla-2.6.31-rc7.txt (38.57 kB)
Download all attachments

2009-08-24 21:58:38

by Sarah Sharp

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

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 <[email protected]>
>
> --
>
> --- 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,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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2009-08-24 21:44:59

by Oliver Neukum

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

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 <[email protected]>

--

--- 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,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)

2009-08-24 21:36:57

by Marcel Holtmann

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

Hi Oliver,

> > okay, then we have to violate coding style here since throughout the
> > whole Bluetooth subsystem we don't do { } around single statements. And
> > this applies to single else statements.
>
> Next trial.
>
> Signed-off-by: Oliver Neukum <[email protected]>

except that I have no commit message now :(

Regards

Marcel



2009-08-24 21:26:00

by Oliver Neukum

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

Am Montag, 24. August 2009 21:59:22 schrieb Marcel Holtmann:
> okay, then we have to violate coding style here since throughout the
> whole Bluetooth subsystem we don't do { } around single statements. And
> this applies to single else statements.

Next trial.

Signed-off-by: Oliver Neukum <[email protected]>

--

--- 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,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)

2009-08-24 19:59:22

by Marcel Holtmann

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

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();
> }

okay, then we have to violate coding style here since throughout the
whole Bluetooth subsystem we don't do { } around single statements. And
this applies to single else statements.

Regards

Marcel



2009-08-24 19:49:51

by Oliver Neukum

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

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 <[email protected]>

--

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 <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,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)

2009-08-24 17:03:11

by Marcel Holtmann

[permalink] [raw]
Subject: Re: btusb autosuspend and circular lock dep

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 <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,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