LinuxLists.cc - [rft]autosuspend for btusb

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

On Tue 2008-08-26 13:02:51, Oliver Neukum wrote:
> Am Dienstag 26 August 2008 12:05:19 schrieb Pavel Machek:
>
> Hi,
>
> > And it worked for me, but suspend died. I'll verify if that's a
> > problem in vanilla -rc4-git, too.
>
> what exactly does "died" mean?

Black screen during resume, backlight on, but otherwise unresposnive machine.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2008-08-26 11:02:51

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Am Dienstag 26 August 2008 12:05:19 schrieb Pavel Machek:

Hi,

> And it worked for me, but suspend died. I'll verify if that's a
> problem in vanilla -rc4-git, too.

what exactly does "died" mean?

Regards
Oliver

2008-08-26 10:05:19

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

On Tue 2008-08-26 11:56:47, Pavel Machek wrote:
> On Mon 2008-08-25 12:43:46, Oliver Neukum wrote:
> > Am Freitag 22 August 2008 16:31:10 schrieb Marcel Holtmann:
> > > > > Please explain the tx_in_flight stuff to me. It looks unneeded since we
> > > > > anchor all TX URBs anyway.
> > > >
> > > > The completion of an URB may happen after the autosuspend timeout passed.
> > > > But we cannot use the pm counters as they are not accessible in interrupt.
> > > > Hence we must maintain a counter ourselves.
> > >
> > > Can we not just check the number of URBs in the anchor? I am against
> > > just duplicating a counter, but then lets call it it what it is to make
> > > it gets not misused. It is a purely a PM counter.
> > >
> >
> > An excellent suggestion allowing major simplifications. This version works
> > for me. It required an extension of the anchor API, so it obsoletes
> > the split-up you've done. Therefore the whole thing comes as a big patch
> > against rc4, easy to test.
>
> I could not get it to apply over -rc4-git, usb seems to be changing
> under us :-(. After manual fixup (conflict was in comment), I still
> get
>
> LD .tmp_vmlinux1
> drivers/built-in.o: In function `btusb_suspend':
> btusb.c:(.text+0x20a578): undefined reference to `usb_anchor_empty'
> drivers/built-in.o: In function `btusb_resume':
> btusb.c:(.text+0x20ade6): undefined reference to `usb_get_from_anchor'
> btusb.c:(.text+0x20ae01): undefined reference to
> `usb_scuttle_anchored_urbs'
> make: *** [.tmp_vmlinux1] Error 1
> 40.09user 7.55system 47.97 (0m47.971s) elapsed 99.34%CPU
> pavel@amd:/data/l/linux-good$
>
> ...probably the "already applied" patch it complained about was not so
> applied after all.
>
> ...ok, I got it to apply, compile, resulting patch is attached.

And it worked for me, but suspend died. I'll verify if that's a
problem in vanilla -rc4-git, too.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2008-08-26 09:56:47

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

On Mon 2008-08-25 12:43:46, Oliver Neukum wrote:
> Am Freitag 22 August 2008 16:31:10 schrieb Marcel Holtmann:
> > > > Please explain the tx_in_flight stuff to me. It looks unneeded since we
> > > > anchor all TX URBs anyway.
> > >
> > > The completion of an URB may happen after the autosuspend timeout passed.
> > > But we cannot use the pm counters as they are not accessible in interrupt.
> > > Hence we must maintain a counter ourselves.
> >
> > Can we not just check the number of URBs in the anchor? I am against
> > just duplicating a counter, but then lets call it it what it is to make
> > it gets not misused. It is a purely a PM counter.
> >
>
> An excellent suggestion allowing major simplifications. This version works
> for me. It required an extension of the anchor API, so it obsoletes
> the split-up you've done. Therefore the whole thing comes as a big patch
> against rc4, easy to test.

I could not get it to apply over -rc4-git, usb seems to be changing
under us :-(. After manual fixup (conflict was in comment), I still
get

LD .tmp_vmlinux1
drivers/built-in.o: In function `btusb_suspend':
btusb.c:(.text+0x20a578): undefined reference to `usb_anchor_empty'
drivers/built-in.o: In function `btusb_resume':
btusb.c:(.text+0x20ade6): undefined reference to `usb_get_from_anchor'
btusb.c:(.text+0x20ae01): undefined reference to
`usb_scuttle_anchored_urbs'
make: *** [.tmp_vmlinux1] Error 1
40.09user 7.55system 47.97 (0m47.971s) elapsed 99.34%CPU
pavel@amd:/data/l/linux-good$

...probably the "already applied" patch it complained about was not so
applied after all.

...ok, I got it to apply, compile, resulting patch is attached.
Pavel

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 6a01068..62a9a5b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -35,13 +35,13 @@ #include <linux/usb.h>
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>

-//#define CONFIG_BT_HCIBTUSB_DEBUG
+#define CONFIG_BT_HCIBTUSB_DEBUG
#ifndef CONFIG_BT_HCIBTUSB_DEBUG
#undef BT_DBG
#define BT_DBG(D...)
#endif

-#define VERSION "0.3"
+#define VERSION "0.4"

static int ignore_dga;
static int ignore_csr;
@@ -165,10 +165,12 @@ #define BTUSB_MAX_ISOC_FRAMES 10
#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;
struct usb_device *udev;
+ struct usb_interface *acl;
struct usb_interface *isoc;

spinlock_t lock;
@@ -176,11 +178,13 @@ 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;

struct usb_endpoint_descriptor *intr_ep;
struct usb_endpoint_descriptor *bulk_tx_ep;
@@ -189,6 +193,8 @@ struct btusb_data {
struct usb_endpoint_descriptor *isoc_rx_ep;

int isoc_altsetting;
+ int did_iso_resume:1;
+ int susp_count;
};

static void btusb_intr_complete(struct urb *urb)
@@ -217,6 +223,7 @@ static void btusb_intr_complete(struct u
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);
@@ -227,7 +234,7 @@ static void btusb_intr_complete(struct u
}
}

-static int btusb_submit_intr_urb(struct hci_dev *hdev)
+static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t gfp)
{
struct btusb_data *data = hdev->driver_data;
struct urb *urb;
@@ -240,13 +247,13 @@ static int btusb_submit_intr_urb(struct
if (!data->intr_ep)
return -ENODEV;

- urb = usb_alloc_urb(0, GFP_ATOMIC);
+ urb = usb_alloc_urb(0, gfp);
if (!urb)
return -ENOMEM;

size = le16_to_cpu(data->intr_ep->wMaxPacketSize);

- buf = kmalloc(size, GFP_ATOMIC);
+ buf = kmalloc(size, gfp);
if (!buf) {
usb_free_urb(urb);
return -ENOMEM;
@@ -262,7 +269,7 @@ static int btusb_submit_intr_urb(struct

usb_anchor_urb(urb, &data->intr_anchor);

- err = usb_submit_urb(urb, GFP_ATOMIC);
+ err = usb_submit_urb(urb, gfp);
if (err < 0) {
BT_ERR("%s urb %p submission failed (%d)",
hdev->name, urb, -err);
@@ -301,6 +308,7 @@ static void btusb_bulk_complete(struct u
if (!test_bit(BTUSB_BULK_RUNNING, &data->flags))
return;

+ usb_mark_last_busy(data->udev);
usb_anchor_urb(urb, &data->bulk_anchor);

err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -311,7 +319,7 @@ static void btusb_bulk_complete(struct u
}
}

-static int btusb_submit_bulk_urb(struct hci_dev *hdev)
+static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t gfp)
{
struct btusb_data *data = hdev->driver_data;
struct urb *urb;
@@ -324,18 +332,19 @@ static int btusb_submit_bulk_urb(struct
if (!data->bulk_rx_ep)
return -ENODEV;

- urb = usb_alloc_urb(0, GFP_KERNEL);
+ urb = usb_alloc_urb(0, gfp);
if (!urb)
return -ENOMEM;

size = le16_to_cpu(data->bulk_rx_ep->wMaxPacketSize);

- buf = kmalloc(size, GFP_KERNEL);
+ buf = kmalloc(size, gfp);
if (!buf) {
usb_free_urb(urb);
return -ENOMEM;
}

+ usb_mark_last_busy(data->udev);
pipe = usb_rcvbulkpipe(data->udev, data->bulk_rx_ep->bEndpointAddress);

usb_fill_bulk_urb(urb, data->udev, pipe,
@@ -345,7 +354,7 @@ static int btusb_submit_bulk_urb(struct

usb_anchor_urb(urb, &data->bulk_anchor);

- err = usb_submit_urb(urb, GFP_KERNEL);
+ err = usb_submit_urb(urb, gfp);
if (err < 0) {
BT_ERR("%s urb %p submission failed (%d)",
hdev->name, urb, -err);
@@ -508,13 +517,19 @@ static int btusb_open(struct hci_dev *hd

BT_DBG("%s", hdev->name);

+ err = usb_autopm_get_interface(data->acl);
+ if (err < 0)
+ return err;
+ data->acl->needs_remote_wakeup = 1;
+ usb_autopm_put_interface(data->acl);
+
if (test_and_set_bit(HCI_RUNNING, &hdev->flags))
return 0;

if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
return 0;

- err = btusb_submit_intr_urb(hdev);
+ err = btusb_submit_intr_urb(hdev, GFP_ATOMIC);
if (err < 0) {
clear_bit(BTUSB_INTR_RUNNING, &hdev->flags);
clear_bit(HCI_RUNNING, &hdev->flags);
@@ -523,24 +538,34 @@ static int btusb_open(struct hci_dev *hd
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);

if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
return 0;

- clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
+ flush_work(&data->work);

+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
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->acl);
+ if (!err) {
+ data->acl->needs_remote_wakeup = 0;
+ usb_autopm_put_interface(data->acl);
+ }
return 0;
}

@@ -562,7 +587,7 @@ static int btusb_send_frame(struct sk_bu
struct usb_ctrlrequest *dr;
struct urb *urb;
unsigned int pipe;
- int err;
+ int err, susp;

BT_DBG("%s", hdev->name);

@@ -571,6 +596,7 @@ static int btusb_send_frame(struct sk_bu

switch (bt_cb(skb)->pkt_type) {
case HCI_COMMAND_PKT:
+ BT_DBG("HCI_COMMAND_PKT");
urb = usb_alloc_urb(0, GFP_ATOMIC);
if (!urb)
return -ENOMEM;
@@ -596,6 +622,7 @@ static int btusb_send_frame(struct sk_bu
break;

case HCI_ACLDATA_PKT:
+ BT_DBG("HCI_ACLDATA_PKT");
if (!data->bulk_tx_ep || hdev->conn_hash.acl_num < 1)
return -ENODEV;

@@ -613,6 +640,7 @@ static int btusb_send_frame(struct sk_bu
break;

case HCI_SCODATA_PKT:
+ BT_DBG("HCI_SCODATA_PKT");
if (!data->isoc_tx_ep || hdev->conn_hash.sco_num < 1)
return -ENODEV;

@@ -643,17 +671,22 @@ static int btusb_send_frame(struct sk_bu
return -EILSEQ;
}

+ spin_lock(&data->lock);
+ susp = test_bit(BTUSB_SUSPENDING, &data->flags);
usb_anchor_urb(urb, &data->tx_anchor);
-
- err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0) {
- BT_ERR("%s urb %p submission failed", hdev->name, urb);
- kfree(urb->setup_packet);
- usb_unanchor_urb(urb);
+ if (susp) {
+ schedule_work(&data->waker);
+ err = 0;
+ } else {
+ err = usb_submit_urb(urb, GFP_ATOMIC);
+ if (err < 0) {
+ BT_ERR("%s urb %p submission failed", hdev->name, urb);
+ kfree(urb->setup_packet);
+ usb_unanchor_urb(urb);
+ }
+ usb_free_urb(urb);
}
-
- usb_free_urb(urb);
-
+ spin_unlock(&data->lock);
return err;
}

@@ -672,8 +705,19 @@ static void btusb_notify(struct hci_dev

BT_DBG("%s evt %d", hdev->name, evt);

- if (evt == HCI_NOTIFY_CONN_ADD || evt == HCI_NOTIFY_CONN_DEL)
- schedule_work(&data->work);
+ if (hdev->conn_hash.acl_num > 0) {
+ if (!test_and_set_bit(BTUSB_BULK_RUNNING, &data->flags)) {
+ if (btusb_submit_bulk_urb(hdev, GFP_ATOMIC) < 0)
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ else
+ btusb_submit_bulk_urb(hdev, GFP_ATOMIC);
+ }
+ } else {
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ usb_unlink_anchored_urbs(&data->bulk_anchor);
+ }
+
+ schedule_work(&data->work);
}

static int inline __set_isoc_interface(struct hci_dev *hdev, int altsetting)
@@ -723,20 +767,19 @@ static void btusb_work(struct work_struc
{
struct btusb_data *data = container_of(work, struct btusb_data, work);
struct hci_dev *hdev = data->hdev;
-
- if (hdev->conn_hash.acl_num > 0) {
- if (!test_and_set_bit(BTUSB_BULK_RUNNING, &data->flags)) {
- if (btusb_submit_bulk_urb(hdev) < 0)
- clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- else
- btusb_submit_bulk_urb(hdev);
- }
- } else {
- clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->bulk_anchor);
- }
+ 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;
+ }
+ }
if (data->isoc_altsetting != 2) {
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
usb_kill_anchored_urbs(&data->isoc_anchor);
@@ -756,9 +799,26 @@ static void btusb_work(struct work_struc
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;
+
+ BUG_ON(data == NULL);
+ BT_DBG("about to resume");
+ BUG_ON(data->acl == NULL);
+ err = usb_autopm_get_interface(data->acl);
+ if (!err)
+ usb_autopm_put_interface(data->acl);
+}
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -821,15 +881,18 @@ static int btusb_probe(struct usb_interf
}

data->udev = interface_to_usbdev(intf);
+ data->acl = intf;

spin_lock_init(&data->lock);

INIT_WORK(&data->work, btusb_work);
+ INIT_WORK(&data->waker, btusb_waker);

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) {
@@ -889,7 +952,7 @@ static int btusb_probe(struct usb_interf

if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
- data->isoc, NULL);
+ data->isoc, data);
if (err < 0) {
hci_free_dev(hdev);
kfree(data);
@@ -921,21 +984,128 @@ static void btusb_disconnect(struct usb_

hdev = data->hdev;

- if (data->isoc)
- usb_driver_release_interface(&btusb_driver, data->isoc);
+ /* make sure we have a reference */
+ __hci_dev_hold(hdev);

- usb_set_intfdata(intf, NULL);
+ usb_set_intfdata(data->acl, NULL);
+ if (data->isoc)
+ usb_set_intfdata(data->isoc, NULL);

+ /* unregister before releasing any interface */
hci_unregister_dev(hdev);

+ if (intf == data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->acl);
+ else if (data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->isoc);
+
+ /* release the reference */
+ __hci_dev_put(hdev);
hci_free_dev(hdev);
}

+static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct btusb_data *data = usb_get_intfdata(intf);
+
+ BT_DBG("%s called\n", __func__);
+
+ if (data->susp_count++)
+ return 0;
+ spin_lock_irq(&data->lock);
+ if ( interface_to_usbdev(intf)->auto_pm &&
+ !usb_anchor_empty(&data->tx_anchor)) {
+ spin_unlock_irq(&data->lock);
+ return -EBUSY;
+ }
+
+ set_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->lock);
+
+ cancel_work_sync(&data->work);
+ btusb_stop_traffic(data);
+ usb_kill_anchored_urbs(&data->tx_anchor);
+ return 0;
+}
+
+static int play_deferred(struct btusb_data *data)
+{
+ struct urb *urb;
+ int err = 0;
+
+ while ((urb = usb_get_from_anchor(&data->tx_anchor))) {
+ err = usb_submit_urb(urb, GFP_ATOMIC);
+ if (err < 0)
+ break;
+ }
+ usb_scuttle_anchored_urbs(&data->tx_anchor);
+ return err;
+}
+
+static int btusb_resume(struct usb_interface *intf)
+{
+ struct btusb_data *data = usb_get_intfdata(intf);
+ struct hci_dev *hdev = data->hdev;
+ int ret;
+
+ if (--data->susp_count)
+ return 0;
+ if (test_bit(HCI_RUNNING, &hdev->flags)) {
+
+ spin_lock_irq(&data->lock);
+ ret = play_deferred(data);
+ clear_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->lock);
+
+ if (ret < 0) {
+ clear_bit(HCI_RUNNING, &hdev->flags);
+ return ret;
+ }
+
+ ret = btusb_submit_intr_urb(hdev, GFP_NOIO);
+ if (ret < 0) {
+ clear_bit(HCI_RUNNING, &hdev->flags);
+ return ret;
+ }
+ }
+
+ if (hdev->conn_hash.acl_num > 0) {
+ ret = btusb_submit_bulk_urb(hdev, GFP_NOIO);
+ if (ret < 0) {
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ return ret;
+ } else {
+ ret = btusb_submit_bulk_urb(hdev, GFP_NOIO);
+ if (ret < 0) {
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ usb_kill_anchored_urbs(&data->bulk_anchor);
+ return ret;
+ }
+ }
+ }
+
+ if (data->isoc) {
+ if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+ ret = btusb_submit_isoc_urb(hdev);
+ if (ret < 0)
+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+ else
+ btusb_submit_isoc_urb(hdev);
+ }
+ }
+
+ schedule_work(&data->work);
+ return 0;
+}
+
static struct usb_driver btusb_driver = {
.name = "btusb",
.probe = btusb_probe,
.disconnect = btusb_disconnect,
+ .suspend = btusb_suspend,
+ .resume = btusb_resume,
.id_table = btusb_table,
+ .supports_autosuspend = 1,
};

static int __init btusb_init(void)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 47111e8..0ceb312 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -633,3 +633,61 @@ int usb_wait_anchor_empty_timeout(struct
msecs_to_jiffies(timeout));
}
EXPORT_SYMBOL_GPL(usb_wait_anchor_empty_timeout);
+
+struct urb *usb_get_from_anchor(struct usb_anchor *anchor)
+{
+ struct urb *victim;
+ unsigned long flags;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ if (!list_empty(&anchor->urb_list)) {
+ victim = list_entry(anchor->urb_list.next, struct urb,
+ anchor_list);
+ usb_get_urb(victim);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+ usb_unanchor_urb(victim);
+ } else {
+ spin_unlock_irqrestore(&anchor->lock, flags);
+ victim = NULL;
+ }
+
+ return victim;
+}
+
+EXPORT_SYMBOL_GPL(usb_get_from_anchor);
+
+void usb_scuttle_anchored_urbs(struct usb_anchor *anchor)
+{
+ struct urb *victim;
+ unsigned long flags;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ while (!list_empty(&anchor->urb_list)) {
+ victim = list_entry(anchor->urb_list.prev, struct urb,
+ anchor_list);
+ usb_get_urb(victim);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+ /* this will unanchor the URB */
+ usb_unanchor_urb(victim);
+ usb_put_urb(victim);
+ spin_lock_irqsave(&anchor->lock, flags);
+ }
+ spin_unlock_irqrestore(&anchor->lock, flags);
+}
+
+EXPORT_SYMBOL_GPL(usb_scuttle_anchored_urbs);
+
+int usb_anchor_empty(struct usb_anchor *anchor)
+{
+ unsigned long flags;
+ int rv;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ rv = list_empty(&anchor->urb_list);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+
+ return rv;
+}
+
+EXPORT_SYMBOL_GPL(usb_anchor_empty);
+
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 94ac74a..4f09a57 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -112,8 +112,6 @@ enum usb_interface_condition {
* capability during autosuspend.
* @needs_altsetting0: flag set when a set-interface request for altsetting 0
* has been deferred.
- * @needs_binding: flag set when the driver should be re-probed or unbound
- * following a reset or suspend operation it doesn't support.
* @dev: driver model's view of this device
* @usb_dev: if an interface is bound to the USB major, this will point
* to the sysfs representation for that device.
@@ -1465,6 +1463,9 @@ extern void usb_anchor_urb(struct urb *u
extern void usb_unanchor_urb(struct urb *urb);
extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
unsigned int timeout);
+extern struct urb *usb_get_from_anchor(struct usb_anchor *anchor);
+extern void usb_scuttle_anchored_urbs(struct usb_anchor *anchor);
+extern int usb_anchor_empty(struct usb_anchor *anchor);

/**
* usb_urb_dir_in - check if an URB describes an IN transfer

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2008-08-25 12:45:17

[permalink] [raw]

Subject: Re: btusb hibernation/suspend breakage in current -git

Hi!

> Speaking of btusb, I'm obseving a reproducible breakage of suspend and
> hibernation (for hibernation it's 100% reproducible, for suspend to RAM about
> 50%) on my hp nx6325 with the current -git kernel.

I have been using btusb + s2ram for quite a long now... (and major
difference between hci_usb and btusb is that btusb will not randomly
corrupt memory when suspending with bluetooth in use...) lets see if I
can update to latest git...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2008-08-25 11:55:50

[permalink] [raw]

Subject: Re: btusb hibernation/suspend breakage in current -git

Hi Oliver,

>> Since the bluetooth adapter in this box is apparently handled by
>> btusb, I'm
>> suspecting the problem may be related to it or to the USB subsystem
>> (ehci-hcd
>> in particular). I'm going to debug it further, but if you have any
>> ideas to
>> try, I'll appreciate any help.
>
> btusb in current git doesn't implement suspend/resume. Thus the
> driver will be
> unbound. Possibly the bluetooth subsystem cannot stand that while
> suspended.

that should work. It then really looks like a bug. Something more
weird must be going on here.

Regards

Marcel

2008-08-25 11:53:10

[permalink] [raw]

Subject: Re: btusb hibernation/suspend breakage in current -git

Am Montag 25 August 2008 13:37:14 schrieb Rafael J. Wysocki:
> Since the bluetooth adapter in this box is apparently handled by btusb, I=
'm
> suspecting the problem may be related to it or to the USB subsystem (ehci=
=2Dhcd
> in particular). =A0I'm going to debug it further, but if you have any ide=
as to
> try, I'll appreciate any help.

btusb in current git doesn't implement suspend/resume. Thus the driver will=
be
unbound. Possibly the bluetooth subsystem cannot stand that while suspended.

Regards
Oliver

2008-08-25 11:51:30

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Hi Oliver,

>>>> Please explain the tx_in_flight stuff to me. It looks unneeded
>>>> since we
>>>> anchor all TX URBs anyway.
>>>
>>> The completion of an URB may happen after the autosuspend timeout
>>> passed.
>>> But we cannot use the pm counters as they are not accessible in
>>> interrupt.
>>> Hence we must maintain a counter ourselves.
>>
>> Can we not just check the number of URBs in the anchor? I am against
>> just duplicating a counter, but then lets call it it what it is to
>> make
>> it gets not misused. It is a purely a PM counter.
>>
>
> An excellent suggestion allowing major simplifications. This version
> works
> for me. It required an extension of the anchor API, so it obsoletes
> the split-up you've done. Therefore the whole thing comes as a big
> patch
> against rc4, easy to test.

I will break up the patch into pieces. However right now I am missing
my beloved test machine since I am traveling. So everybody reporting
success or failure would be nice.

Can you break out the USB anchor extensions and make sure they get
merged into 2.6.28 in an early stage of the merge window. It really
wanna get all of this stuff into the next kernel release. Finally we
are getting somewhere with this driver and can kill the broken hci_usb.

Regards

Marcel

2008-08-25 11:37:14

by Rafael J. Wysocki

[permalink] [raw]

Subject: btusb hibernation/suspend breakage in current -git

Hi,

Speaking of btusb, I'm obseving a reproducible breakage of suspend and
hibernation (for hibernation it's 100% reproducible, for suspend to RAM about
50%) on my hp nx6325 with the current -git kernel.

The system crashes as soon as user space is thawed after a resume and it's
sufficient to switch the bluetooth subsystem off to avoid this. After such a
crash I'm usually unable to get any information from the box (from the stack
traces I've seen once after such a crash it looks like interrupt handlers are
involved somehow).

Since the bluetooth adapter in this box is apparently handled by btusb, I'm
suspecting the problem may be related to it or to the USB subsystem (ehci-hcd
in particular). I'm going to debug it further, but if you have any ideas to
try, I'll appreciate any help.

Thanks,
Rafael

2008-08-25 10:43:46

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Am Freitag 22 August 2008 16:31:10 schrieb Marcel Holtmann:
> > > Please explain the tx_in_flight stuff to me. It looks unneeded since we
> > > anchor all TX URBs anyway.
> >
> > The completion of an URB may happen after the autosuspend timeout passed.
> > But we cannot use the pm counters as they are not accessible in interrupt.
> > Hence we must maintain a counter ourselves.
>
> Can we not just check the number of URBs in the anchor? I am against
> just duplicating a counter, but then lets call it it what it is to make
> it gets not misused. It is a purely a PM counter.
>

An excellent suggestion allowing major simplifications. This version works
for me. It required an extension of the anchor API, so it obsoletes
the split-up you've done. Therefore the whole thing comes as a big patch
against rc4, easy to test.

Regards
Oliver

----

--- linux-2.6.27-rc4/drivers/usb/core/urb.c 2008-08-21 10:03:44.000000000 +0200
+++ linux-2.6.27-rc3/drivers/usb/core/urb.c 2008-08-22 17:25:49.000000000 +0200
@@ -601,15 +601,20 @@ EXPORT_SYMBOL_GPL(usb_kill_anchored_urbs
void usb_unlink_anchored_urbs(struct usb_anchor *anchor)
{
struct urb *victim;
+ unsigned long flags;

- spin_lock_irq(&anchor->lock);
+ spin_lock_irqsave(&anchor->lock, flags);
while (!list_empty(&anchor->urb_list)) {
victim = list_entry(anchor->urb_list.prev, struct urb,
anchor_list);
+ usb_get_urb(victim);
+ spin_unlock_irqrestore(&anchor->lock, flags);
/* this will unanchor the URB */
usb_unlink_urb(victim);
+ usb_put_urb(victim);
+ spin_lock_irqsave(&anchor->lock, flags);
}
- spin_unlock_irq(&anchor->lock);
+ spin_unlock_irqrestore(&anchor->lock, flags);
}
EXPORT_SYMBOL_GPL(usb_unlink_anchored_urbs);

@@ -628,3 +633,61 @@ int usb_wait_anchor_empty_timeout(struct
msecs_to_jiffies(timeout));
}
EXPORT_SYMBOL_GPL(usb_wait_anchor_empty_timeout);
+
+struct urb *usb_get_from_anchor(struct usb_anchor *anchor)
+{
+ struct urb *victim;
+ unsigned long flags;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ if (!list_empty(&anchor->urb_list)) {
+ victim = list_entry(anchor->urb_list.next, struct urb,
+ anchor_list);
+ usb_get_urb(victim);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+ usb_unanchor_urb(victim);
+ } else {
+ spin_unlock_irqrestore(&anchor->lock, flags);
+ victim = NULL;
+ }
+
+ return victim;
+}
+
+EXPORT_SYMBOL_GPL(usb_get_from_anchor);
+
+void usb_scuttle_anchored_urbs(struct usb_anchor *anchor)
+{
+ struct urb *victim;
+ unsigned long flags;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ while (!list_empty(&anchor->urb_list)) {
+ victim = list_entry(anchor->urb_list.prev, struct urb,
+ anchor_list);
+ usb_get_urb(victim);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+ /* this will unanchor the URB */
+ usb_unanchor_urb(victim);
+ usb_put_urb(victim);
+ spin_lock_irqsave(&anchor->lock, flags);
+ }
+ spin_unlock_irqrestore(&anchor->lock, flags);
+}
+
+EXPORT_SYMBOL_GPL(usb_scuttle_anchored_urbs);
+
+int usb_anchor_empty(struct usb_anchor *anchor)
+{
+ unsigned long flags;
+ int rv;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ rv = list_empty(&anchor->urb_list);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+
+ return rv;
+}
+
+EXPORT_SYMBOL_GPL(usb_anchor_empty);
+
--- linux-2.6.27-rc4/include/linux/usb.h 2008-08-21 10:04:29.000000000 +0200
+++ linux-2.6.27-rc3/include/linux/usb.h 2008-08-22 17:25:47.000000000 +0200
@@ -110,8 +110,6 @@ enum usb_interface_condition {
* @sysfs_files_created: sysfs attributes exist
* @needs_remote_wakeup: flag set when the driver requires remote-wakeup
* capability during autosuspend.
- * @needs_binding: flag set when the driver should be re-probed or unbound
- * following a reset or suspend operation it doesn't support.
* @dev: driver model's view of this device
* @usb_dev: if an interface is bound to the USB major, this will point
* to the sysfs representation for that device.
@@ -1462,6 +1460,9 @@ extern void usb_anchor_urb(struct urb *u
extern void usb_unanchor_urb(struct urb *urb);
extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
unsigned int timeout);
+extern struct urb *usb_get_from_anchor(struct usb_anchor *anchor);
+extern void usb_scuttle_anchored_urbs(struct usb_anchor *anchor);
+extern int usb_anchor_empty(struct usb_anchor *anchor);

/**
* usb_urb_dir_in - check if an URB describes an IN transfer
--- linux-2.6.27-rc4/drivers/bluetooth/btusb.c 2008-08-21 10:04:11.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c 2008-08-25 12:01:15.000000000 +0200
@@ -35,13 +35,13 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>

-//#define CONFIG_BT_HCIBTUSB_DEBUG
+#define CONFIG_BT_HCIBTUSB_DEBUG
#ifndef CONFIG_BT_HCIBTUSB_DEBUG
#undef BT_DBG
#define BT_DBG(D...)
#endif

-#define VERSION "0.3"
+#define VERSION "0.4"

static int ignore_dga;
static int ignore_csr;
@@ -165,10 +165,12 @@ static struct usb_device_id blacklist_ta
#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;
struct usb_device *udev;
+ struct usb_interface *acl;
struct usb_interface *isoc;

spinlock_t lock;
@@ -176,11 +178,13 @@ 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;

struct usb_endpoint_descriptor *intr_ep;
struct usb_endpoint_descriptor *bulk_tx_ep;
@@ -189,6 +193,8 @@ struct btusb_data {
struct usb_endpoint_descriptor *isoc_rx_ep;

int isoc_altsetting;
+ int did_iso_resume:1;
+ int susp_count;
};

static void btusb_intr_complete(struct urb *urb)
@@ -217,6 +223,7 @@ static void btusb_intr_complete(struct u
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);
@@ -227,7 +234,7 @@ static void btusb_intr_complete(struct u
}
}

-static int btusb_submit_intr_urb(struct hci_dev *hdev)
+static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t gfp)
{
struct btusb_data *data = hdev->driver_data;
struct urb *urb;
@@ -240,13 +247,13 @@ static int btusb_submit_intr_urb(struct
if (!data->intr_ep)
return -ENODEV;

- urb = usb_alloc_urb(0, GFP_ATOMIC);
+ urb = usb_alloc_urb(0, gfp);
if (!urb)
return -ENOMEM;

size = le16_to_cpu(data->intr_ep->wMaxPacketSize);

- buf = kmalloc(size, GFP_ATOMIC);
+ buf = kmalloc(size, gfp);
if (!buf) {
usb_free_urb(urb);
return -ENOMEM;
@@ -262,7 +269,7 @@ static int btusb_submit_intr_urb(struct

usb_anchor_urb(urb, &data->intr_anchor);

- err = usb_submit_urb(urb, GFP_ATOMIC);
+ err = usb_submit_urb(urb, gfp);
if (err < 0) {
BT_ERR("%s urb %p submission failed (%d)",
hdev->name, urb, -err);
@@ -301,6 +308,7 @@ static void btusb_bulk_complete(struct u
if (!test_bit(BTUSB_BULK_RUNNING, &data->flags))
return;

+ usb_mark_last_busy(data->udev);
usb_anchor_urb(urb, &data->bulk_anchor);

err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -311,7 +319,7 @@ static void btusb_bulk_complete(struct u
}
}

-static int btusb_submit_bulk_urb(struct hci_dev *hdev)
+static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t gfp)
{
struct btusb_data *data = hdev->driver_data;
struct urb *urb;
@@ -324,18 +332,19 @@ static int btusb_submit_bulk_urb(struct
if (!data->bulk_rx_ep)
return -ENODEV;

- urb = usb_alloc_urb(0, GFP_KERNEL);
+ urb = usb_alloc_urb(0, gfp);
if (!urb)
return -ENOMEM;

size = le16_to_cpu(data->bulk_rx_ep->wMaxPacketSize);

- buf = kmalloc(size, GFP_KERNEL);
+ buf = kmalloc(size, gfp);
if (!buf) {
usb_free_urb(urb);
return -ENOMEM;
}

+ usb_mark_last_busy(data->udev);
pipe = usb_rcvbulkpipe(data->udev, data->bulk_rx_ep->bEndpointAddress);

usb_fill_bulk_urb(urb, data->udev, pipe,
@@ -345,7 +354,7 @@ static int btusb_submit_bulk_urb(struct

usb_anchor_urb(urb, &data->bulk_anchor);

- err = usb_submit_urb(urb, GFP_KERNEL);
+ err = usb_submit_urb(urb, gfp);
if (err < 0) {
BT_ERR("%s urb %p submission failed (%d)",
hdev->name, urb, -err);
@@ -508,13 +517,19 @@ static int btusb_open(struct hci_dev *hd

BT_DBG("%s", hdev->name);

+ err = usb_autopm_get_interface(data->acl);
+ if (err < 0)
+ return err;
+ data->acl->needs_remote_wakeup = 1;
+ usb_autopm_put_interface(data->acl);
+
if (test_and_set_bit(HCI_RUNNING, &hdev->flags))
return 0;

if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
return 0;

- err = btusb_submit_intr_urb(hdev);
+ err = btusb_submit_intr_urb(hdev, GFP_ATOMIC);
if (err < 0) {
clear_bit(BTUSB_INTR_RUNNING, &hdev->flags);
clear_bit(HCI_RUNNING, &hdev->flags);
@@ -523,24 +538,34 @@ static int btusb_open(struct hci_dev *hd
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);

if (!test_and_clear_bit(HCI_RUNNING, &hdev->flags))
return 0;

- clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
+ flush_work(&data->work);

+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
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->acl);
+ if (!err) {
+ data->acl->needs_remote_wakeup = 0;
+ usb_autopm_put_interface(data->acl);
+ }
return 0;
}

@@ -562,7 +587,7 @@ static int btusb_send_frame(struct sk_bu
struct usb_ctrlrequest *dr;
struct urb *urb;
unsigned int pipe;
- int err;
+ int err, susp;

BT_DBG("%s", hdev->name);

@@ -571,6 +596,7 @@ static int btusb_send_frame(struct sk_bu

switch (bt_cb(skb)->pkt_type) {
case HCI_COMMAND_PKT:
+ BT_DBG("HCI_COMMAND_PKT");
urb = usb_alloc_urb(0, GFP_ATOMIC);
if (!urb)
return -ENOMEM;
@@ -596,6 +622,7 @@ static int btusb_send_frame(struct sk_bu
break;

case HCI_ACLDATA_PKT:
+ BT_DBG("HCI_ACLDATA_PKT");
if (!data->bulk_tx_ep || hdev->conn_hash.acl_num < 1)
return -ENODEV;

@@ -613,6 +640,7 @@ static int btusb_send_frame(struct sk_bu
break;

case HCI_SCODATA_PKT:
+ BT_DBG("HCI_SCODATA_PKT");
if (!data->isoc_tx_ep || hdev->conn_hash.sco_num < 1)
return -ENODEV;

@@ -643,17 +671,22 @@ static int btusb_send_frame(struct sk_bu
return -EILSEQ;
}

+ spin_lock(&data->lock);
+ susp = test_bit(BTUSB_SUSPENDING, &data->flags);
usb_anchor_urb(urb, &data->tx_anchor);
-
- err = usb_submit_urb(urb, GFP_ATOMIC);
- if (err < 0) {
- BT_ERR("%s urb %p submission failed", hdev->name, urb);
- kfree(urb->setup_packet);
- usb_unanchor_urb(urb);
+ if (susp) {
+ schedule_work(&data->waker);
+ err = 0;
+ } else {
+ err = usb_submit_urb(urb, GFP_ATOMIC);
+ if (err < 0) {
+ BT_ERR("%s urb %p submission failed", hdev->name, urb);
+ kfree(urb->setup_packet);
+ usb_unanchor_urb(urb);
+ }
+ usb_free_urb(urb);
}
-
- usb_free_urb(urb);
-
+ spin_unlock(&data->lock);
return err;
}

@@ -672,8 +705,19 @@ static void btusb_notify(struct hci_dev

BT_DBG("%s evt %d", hdev->name, evt);

- if (evt == HCI_NOTIFY_CONN_ADD || evt == HCI_NOTIFY_CONN_DEL)
- schedule_work(&data->work);
+ if (hdev->conn_hash.acl_num > 0) {
+ if (!test_and_set_bit(BTUSB_BULK_RUNNING, &data->flags)) {
+ if (btusb_submit_bulk_urb(hdev, GFP_ATOMIC) < 0)
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ else
+ btusb_submit_bulk_urb(hdev, GFP_ATOMIC);
+ }
+ } else {
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ usb_unlink_anchored_urbs(&data->bulk_anchor);
+ }
+
+ schedule_work(&data->work);
}

static int inline __set_isoc_interface(struct hci_dev *hdev, int altsetting)
@@ -723,20 +767,19 @@ static void btusb_work(struct work_struc
{
struct btusb_data *data = container_of(work, struct btusb_data, work);
struct hci_dev *hdev = data->hdev;
-
- if (hdev->conn_hash.acl_num > 0) {
- if (!test_and_set_bit(BTUSB_BULK_RUNNING, &data->flags)) {
- if (btusb_submit_bulk_urb(hdev) < 0)
- clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- else
- btusb_submit_bulk_urb(hdev);
- }
- } else {
- clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->bulk_anchor);
- }
+ 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;
+ }
+ }
if (data->isoc_altsetting != 2) {
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
usb_kill_anchored_urbs(&data->isoc_anchor);
@@ -756,9 +799,26 @@ static void btusb_work(struct work_struc
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;
+
+ BUG_ON(data == NULL);
+ BT_DBG("about to resume");
+ BUG_ON(data->acl == NULL);
+ err = usb_autopm_get_interface(data->acl);
+ if (!err)
+ usb_autopm_put_interface(data->acl);
+}
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -821,15 +881,18 @@ static int btusb_probe(struct usb_interf
}

data->udev = interface_to_usbdev(intf);
+ data->acl = intf;

spin_lock_init(&data->lock);

INIT_WORK(&data->work, btusb_work);
+ INIT_WORK(&data->waker, btusb_waker);

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) {
@@ -889,7 +952,7 @@ static int btusb_probe(struct usb_interf

if (data->isoc) {
err = usb_driver_claim_interface(&btusb_driver,
- data->isoc, NULL);
+ data->isoc, data);
if (err < 0) {
hci_free_dev(hdev);
kfree(data);
@@ -921,21 +984,128 @@ static void btusb_disconnect(struct usb_

hdev = data->hdev;

- if (data->isoc)
- usb_driver_release_interface(&btusb_driver, data->isoc);
+ /* make sure we have a reference */
+ __hci_dev_hold(hdev);

- usb_set_intfdata(intf, NULL);
+ usb_set_intfdata(data->acl, NULL);
+ if (data->isoc)
+ usb_set_intfdata(data->isoc, NULL);

+ /* unregister before releasing any interface */
hci_unregister_dev(hdev);

+ if (intf == data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->acl);
+ else if (data->isoc)
+ usb_driver_release_interface(&btusb_driver, data->isoc);
+
+ /* release the reference */
+ __hci_dev_put(hdev);
hci_free_dev(hdev);
}

+static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct btusb_data *data = usb_get_intfdata(intf);
+
+ BT_DBG("%s called\n", __func__);
+
+ if (data->susp_count++)
+ return 0;
+ spin_lock_irq(&data->lock);
+ if ( interface_to_usbdev(intf)->auto_pm &&
+ !usb_anchor_empty(&data->tx_anchor)) {
+ spin_unlock_irq(&data->lock);
+ return -EBUSY;
+ }
+
+ set_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->lock);
+
+ cancel_work_sync(&data->work);
+ btusb_stop_traffic(data);
+ usb_kill_anchored_urbs(&data->tx_anchor);
+ return 0;
+}
+
+static int play_deferred(struct btusb_data *data)
+{
+ struct urb *urb;
+ int err = 0;
+
+ while ((urb = usb_get_from_anchor(&data->tx_anchor))) {
+ err = usb_submit_urb(urb, GFP_ATOMIC);
+ if (err < 0)
+ break;
+ }
+ usb_scuttle_anchored_urbs(&data->tx_anchor);
+ return err;
+}
+
+static int btusb_resume(struct usb_interface *intf)
+{
+ struct btusb_data *data = usb_get_intfdata(intf);
+ struct hci_dev *hdev = data->hdev;
+ int ret;
+
+ if (--data->susp_count)
+ return 0;
+ if (test_bit(HCI_RUNNING, &hdev->flags)) {
+
+ spin_lock_irq(&data->lock);
+ ret = play_deferred(data);
+ clear_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->lock);
+
+ if (ret < 0) {
+ clear_bit(HCI_RUNNING, &hdev->flags);
+ return ret;
+ }
+
+ ret = btusb_submit_intr_urb(hdev, GFP_NOIO);
+ if (ret < 0) {
+ clear_bit(HCI_RUNNING, &hdev->flags);
+ return ret;
+ }
+ }
+
+ if (hdev->conn_hash.acl_num > 0) {
+ ret = btusb_submit_bulk_urb(hdev, GFP_NOIO);
+ if (ret < 0) {
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ return ret;
+ } else {
+ ret = btusb_submit_bulk_urb(hdev, GFP_NOIO);
+ if (ret < 0) {
+ clear_bit(BTUSB_BULK_RUNNING, &data->flags);
+ usb_kill_anchored_urbs(&data->bulk_anchor);
+ return ret;
+ }
+ }
+ }
+
+ if (data->isoc) {
+ if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
+ ret = btusb_submit_isoc_urb(hdev);
+ if (ret < 0)
+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+ else
+ btusb_submit_isoc_urb(hdev);
+ }
+ }
+
+ schedule_work(&data->work);
+ return 0;
+}
+
static struct usb_driver btusb_driver = {
.name = "btusb",
.probe = btusb_probe,
.disconnect = btusb_disconnect,
+ .suspend = btusb_suspend,
+ .resume = btusb_resume,
.id_table = btusb_table,
+ .supports_autosuspend = 1,
};

static int __init btusb_init(void)

2008-08-22 14:38:41

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Am Freitag 22 August 2008 16:31:10 schrieb Marcel Holtmann:

Hi,

> > > Please explain the tx_in_flight stuff to me. It looks unneeded since we
> > > anchor all TX URBs anyway.
> >
> > The completion of an URB may happen after the autosuspend timeout passed.
> > But we cannot use the pm counters as they are not accessible in interrupt.
> > Hence we must maintain a counter ourselves.
>
> Can we not just check the number of URBs in the anchor? I am against
> just duplicating a counter, but then lets call it it what it is to make
> it gets not misused. It is a purely a PM counter.

Yes, we can do that. In fact a bool empty/!empty will do. I'll do that.

Regards
Oliver

2008-08-22 14:31:10

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Hi Oliver,

> > > this patch against vanilla 2.6.27-rc4 implements full autosuspend for
> > > btusb. It should allow the HCI to be suspended during periods of inactivity
> > > while retaining full service if the device supports USB remote wakeup.
> >
> > actually we do have two cases. An inactive device can be woken up by an
> > HCI Connection Request coming via the interrupt endpoint or if we have a
> > send_frame callback via the Bluetooth stack itself.
>
> Yes, if send_frame triggers it, we wake using a work queue. A connection
> request will wake us via remote wakeup.
>
> > What do we do if a device does not support remote wakeup?
>
> Autosuspend on close, resume on open. I don't think we can do more.

sounds good to me. I wanna split the whole work into small patches so we
can have an easier review. I attached the first two of this series and
they should apply cleanly against 2.6.27-rc4, but keep in mind that you
need the usb_unlink_anchored_urbs() patch too.

> > > Please test and/or comment on the code.
> > > It works for me with a few glitches but still needs to be a bit polished.
> >
> > Please explain the tx_in_flight stuff to me. It looks unneeded since we
> > anchor all TX URBs anyway.
>
> The completion of an URB may happen after the autosuspend timeout passed.
> But we cannot use the pm counters as they are not accessible in interrupt.
> Hence we must maintain a counter ourselves.

Can we not just check the number of URBs in the anchor? I am against
just duplicating a counter, but then lets call it it what it is to make
it gets not misused. It is a purely a PM counter.

> > So can we just leave the ISOC interface stuff out of it and try to get
>
> The isoc stuff should be handled already, but I haven't tested that.

I know, but I wanna get the basic logic clean and tested. The SCO part
comes after that. Even if I think it should be good already.

> > the case right where we have control and interrupt URBs only and maybe
> > bulk URBs in case we an ACL link up.
> >
> > This reminds me that we should extend the notify() callback to inform
> > about sniff and active state changes. Since in theory when a connection
> > goes into sniff mode, we could suspend it.
>
> We should definitely do that.

I will work on a patch for that one.

Regards

Marcel

Attachments:

patch-btusb-001 (5.13 kB)
patch-btusb-002 (2.56 kB)
Download all attachments

2008-08-22 13:51:56

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Am Freitag 22 August 2008 15:33:53 schrieb Marcel Holtmann:
> Hi Oliver,
>
> > this patch against vanilla 2.6.27-rc4 implements full autosuspend for
> > btusb. It should allow the HCI to be suspended during periods of inactivity
> > while retaining full service if the device supports USB remote wakeup.
>
> actually we do have two cases. An inactive device can be woken up by an
> HCI Connection Request coming via the interrupt endpoint or if we have a
> send_frame callback via the Bluetooth stack itself.

Yes, if send_frame triggers it, we wake using a work queue. A connection
request will wake us via remote wakeup.

> What do we do if a device does not support remote wakeup?

Autosuspend on close, resume on open. I don't think we can do more.

> > Please test and/or comment on the code.
> > It works for me with a few glitches but still needs to be a bit polished.
>
> Please explain the tx_in_flight stuff to me. It looks unneeded since we
> anchor all TX URBs anyway.

The completion of an URB may happen after the autosuspend timeout passed.
But we cannot use the pm counters as they are not accessible in interrupt.
Hence we must maintain a counter ourselves.

> So can we just leave the ISOC interface stuff out of it and try to get

The isoc stuff should be handled already, but I haven't tested that.

> the case right where we have control and interrupt URBs only and maybe
> bulk URBs in case we an ACL link up.
>
> This reminds me that we should extend the notify() callback to inform
> about sniff and active state changes. Since in theory when a connection
> goes into sniff mode, we could suspend it.

We should definitely do that.

Regards
Oliver

2008-08-22 13:33:53

[permalink] [raw]

Subject: Re: [rft]autosuspend for btusb

Hi Oliver,

> this patch against vanilla 2.6.27-rc4 implements full autosuspend for
> btusb. It should allow the HCI to be suspended during periods of inactivity
> while retaining full service if the device supports USB remote wakeup.

actually we do have two cases. An inactive device can be woken up by an
HCI Connection Request coming via the interrupt endpoint or if we have a
send_frame callback via the Bluetooth stack itself.

What do we do if a device does not support remote wakeup?

> Please test and/or comment on the code.
> It works for me with a few glitches but still needs to be a bit polished.

Please explain the tx_in_flight stuff to me. It looks unneeded since we
anchor all TX URBs anyway.

So can we just leave the ISOC interface stuff out of it and try to get
the case right where we have control and interrupt URBs only and maybe
bulk URBs in case we an ACL link up.

This reminds me that we should extend the notify() callback to inform
about sniff and active state changes. Since in theory when a connection
goes into sniff mode, we could suspend it.

Regards

Marcel

2009-02-11 16:55:28