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.
Hi,
here it is.
Regards
Oliver
Signed-off-by: Oliver Neukum <[email protected]>
This patch adds support of USB autosuspend to the btusb driver
If the device doesn't support remote wakeup, simple support based
on up/down is provided. If the device supports remote wakeup, support
for autosuspend while the interface is up is provided.
This is done by queueing URBs in an anchor structure and waking the
device up from a work queue.
--
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -35,7 +35,7 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
-#define VERSION "0.5"
+#define VERSION "0.6"
static int ignore_dga;
static int ignore_csr;
@@ -145,6 +145,7 @@ static struct usb_device_id blacklist_table[] = {
#define BTUSB_INTR_RUNNING 0
#define BTUSB_BULK_RUNNING 1
#define BTUSB_ISOC_RUNNING 2
+#define BTUSB_SUSPENDING 3
struct btusb_data {
struct hci_dev *hdev;
@@ -157,11 +158,15 @@ struct btusb_data {
unsigned long flags;
struct work_struct work;
+ struct work_struct waker;
struct usb_anchor tx_anchor;
struct usb_anchor intr_anchor;
struct usb_anchor bulk_anchor;
struct usb_anchor isoc_anchor;
+ struct usb_anchor deferred;
+ int tx_in_flight;
+ spinlock_t txlock;
struct usb_endpoint_descriptor *intr_ep;
struct usb_endpoint_descriptor *bulk_tx_ep;
@@ -174,8 +179,24 @@ struct btusb_data {
unsigned int sco_num;
int isoc_altsetting;
int suspend_count;
+ int did_iso_resume:1;
};
+static int inc_tx(struct btusb_data *data)
+{
+ unsigned long flags;
+ int rv;
+
+ spin_lock_irqsave(&data->txlock, flags);
+ rv = test_bit(BTUSB_SUSPENDING, &data->flags);
+ if (!rv)
+ data->tx_in_flight++;
+ spin_unlock_irqrestore(&data->txlock, flags);
+
+ return rv;
+}
+
+
static void btusb_intr_complete(struct urb *urb)
{
struct hci_dev *hdev = urb->context;
@@ -202,6 +223,7 @@ static void btusb_intr_complete(struct urb *urb)
if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
return;
+ usb_mark_last_busy(data->udev);
usb_anchor_urb(urb, &data->intr_anchor);
err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -327,6 +349,7 @@ static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags)
urb->transfer_flags |= URB_FREE_BUFFER;
+ usb_mark_last_busy(data->udev);
usb_anchor_urb(urb, &data->bulk_anchor);
err = usb_submit_urb(urb, mem_flags);
@@ -465,6 +488,33 @@ static void btusb_tx_complete(struct urb *urb)
{
struct sk_buff *skb = urb->context;
struct hci_dev *hdev = (struct hci_dev *) skb->dev;
+ struct btusb_data *data = hdev->driver_data;
+
+ BT_DBG("%s urb %p status %d count %d", hdev->name,
+ urb, urb->status, urb->actual_length);
+
+ if (!test_bit(HCI_RUNNING, &hdev->flags))
+ goto done;
+
+ if (!urb->status)
+ hdev->stat.byte_tx += urb->transfer_buffer_length;
+ else
+ hdev->stat.err_tx++;
+
+done:
+ spin_lock(&data->txlock);
+ data->tx_in_flight--;
+ spin_unlock(&data->txlock);
+
+ kfree(urb->setup_packet);
+
+ kfree_skb(skb);
+}
+
+static void btusb_isoc_tx_complete(struct urb *urb)
+{
+ struct sk_buff *skb = urb->context;
+ struct hci_dev *hdev = (struct hci_dev *) skb->dev;
BT_DBG("%s urb %p status %d count %d", hdev->name,
urb, urb->status, urb->actual_length);
@@ -490,11 +540,16 @@ static int btusb_open(struct hci_dev *hdev)
BT_DBG("%s", hdev->name);
+ err = usb_autopm_get_interface(data->intf);
+ if (err < 0)
+ return err;
+ data->intf->needs_remote_wakeup = 1;
+
if (test_and_set_bit(HCI_RUNNING, &hdev->flags))
- return 0;
+ goto out;
if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
- return 0;
+ goto out;
err = btusb_submit_intr_urb(hdev, GFP_KERNEL);
if (err < 0)
@@ -509,17 +564,28 @@ static int btusb_open(struct hci_dev *hdev)
set_bit(BTUSB_BULK_RUNNING, &data->flags);
btusb_submit_bulk_urb(hdev, GFP_KERNEL);
+out:
+ usb_autopm_put_interface(data->intf);
return 0;
failed:
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
clear_bit(HCI_RUNNING, &hdev->flags);
+ usb_autopm_put_interface(data->intf);
return err;
}
+static void btusb_stop_traffic(struct btusb_data *data)
+{
+ usb_kill_anchored_urbs(&data->intr_anchor);
+ usb_kill_anchored_urbs(&data->bulk_anchor);
+ usb_kill_anchored_urbs(&data->isoc_anchor);
+}
+
static int btusb_close(struct hci_dev *hdev)
{
struct btusb_data *data = hdev->driver_data;
+ int err;
BT_DBG("%s", hdev->name);
@@ -529,13 +595,15 @@ static int btusb_close(struct hci_dev *hdev)
cancel_work_sync(&data->work);
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->isoc_anchor);
-
clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->bulk_anchor);
-
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
- usb_kill_anchored_urbs(&data->intr_anchor);
+
+ btusb_stop_traffic(data);
+ err = usb_autopm_get_interface(data->intf);
+ if (!err) {
+ data->intf->needs_remote_wakeup = 0;
+ usb_autopm_put_interface(data->intf);
+ }
return 0;
}
@@ -622,7 +690,7 @@ static int btusb_send_frame(struct sk_buff *skb)
urb->dev = data->udev;
urb->pipe = pipe;
urb->context = skb;
- urb->complete = btusb_tx_complete;
+ urb->complete = btusb_isoc_tx_complete;
urb->interval = data->isoc_tx_ep->bInterval;
urb->transfer_flags = URB_ISO_ASAP;
@@ -633,12 +701,21 @@ static int btusb_send_frame(struct sk_buff *skb)
le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
hdev->stat.sco_tx++;
- break;
+ goto skip_waking;
default:
return -EILSEQ;
}
+ err = inc_tx(data);
+ if (err) {
+ usb_anchor_urb(urb, &data->deferred);
+ schedule_work(&data->waker);
+ err = 0;
+ goto out;
+ }
+
+skip_waking:
usb_anchor_urb(urb, &data->tx_anchor);
err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -646,10 +723,13 @@ static int btusb_send_frame(struct sk_buff *skb)
BT_ERR("%s urb %p submission failed", hdev->name, urb);
kfree(urb->setup_packet);
usb_unanchor_urb(urb);
+ } else {
+ usb_mark_last_busy(data->udev);
}
usb_free_urb(urb);
+out:
return err;
}
@@ -721,8 +801,19 @@ static void btusb_work(struct work_struct *work)
{
struct btusb_data *data = container_of(work, struct btusb_data, work);
struct hci_dev *hdev = data->hdev;
+ int err;
if (hdev->conn_hash.sco_num > 0) {
+ if (!data->did_iso_resume) {
+ err = usb_autopm_get_interface(data->isoc);
+ if (!err) {
+ data->did_iso_resume = 1;
+ } else {
+ clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
+ usb_kill_anchored_urbs(&data->isoc_anchor);
+ return;
+ }
+ }
if (data->isoc_altsetting != 2) {
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
usb_kill_anchored_urbs(&data->isoc_anchor);
@@ -742,9 +833,23 @@ static void btusb_work(struct work_struct *work)
usb_kill_anchored_urbs(&data->isoc_anchor);
__set_isoc_interface(hdev, 0);
+ if (data->did_iso_resume) {
+ data->did_iso_resume = 0;
+ usb_autopm_put_interface(data->isoc);
+ }
}
}
+static void btusb_waker(struct work_struct *work)
+{
+ struct btusb_data *data = container_of(work, struct btusb_data, waker);
+ int err;
+
+ err = usb_autopm_get_interface(data->intf);
+ if (!err)
+ usb_autopm_put_interface(data->intf);
+}
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -814,11 +919,14 @@ static int btusb_probe(struct usb_interface *intf,
spin_lock_init(&data->lock);
INIT_WORK(&data->work, btusb_work);
+ INIT_WORK(&data->waker, btusb_waker);
+ spin_lock_init(&data->txlock);
init_usb_anchor(&data->tx_anchor);
init_usb_anchor(&data->intr_anchor);
init_usb_anchor(&data->bulk_anchor);
init_usb_anchor(&data->isoc_anchor);
+ init_usb_anchor(&data->deferred);
hdev = hci_alloc_dev();
if (!hdev) {
@@ -943,6 +1051,7 @@ static void btusb_disconnect(struct usb_interface *intf)
hci_free_dev(hdev);
}
+#ifdef CONFIG_PM
static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
{
struct btusb_data *data = usb_get_intfdata(intf);
@@ -952,22 +1061,45 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
if (data->suspend_count++)
return 0;
+ spin_lock_irq(&data->txlock);
+ if (!(interface_to_usbdev(intf)->auto_pm && data->tx_in_flight)) {
+ set_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->txlock);
+ } else {
+ spin_unlock_irq(&data->txlock);
+ data->suspend_count--;
+ return -EBUSY;
+ }
+
cancel_work_sync(&data->work);
+ btusb_stop_traffic(data);
usb_kill_anchored_urbs(&data->tx_anchor);
- usb_kill_anchored_urbs(&data->isoc_anchor);
- usb_kill_anchored_urbs(&data->bulk_anchor);
- usb_kill_anchored_urbs(&data->intr_anchor);
-
return 0;
}
+static void play_deferred(struct btusb_data *data)
+{
+ struct urb *urb;
+ int err;
+
+ while ((urb = usb_get_from_anchor(&data->deferred))) {
+ err = usb_submit_urb(urb, GFP_ATOMIC);
+ if (err < 0)
+ break;
+ else
+ data->tx_in_flight++;
+
+ }
+ usb_scuttle_anchored_urbs(&data->deferred);
+}
+
static int btusb_resume(struct usb_interface *intf)
{
struct btusb_data *data = usb_get_intfdata(intf);
struct hci_dev *hdev = data->hdev;
- int err;
+ int err = 0;
BT_DBG("intf %p", intf);
@@ -975,13 +1107,13 @@ static int btusb_resume(struct usb_interface *intf)
return 0;
if (!test_bit(HCI_RUNNING, &hdev->flags))
- return 0;
+ goto no_io_needed;
if (test_bit(BTUSB_INTR_RUNNING, &data->flags)) {
err = btusb_submit_intr_urb(hdev, GFP_NOIO);
if (err < 0) {
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
- return err;
+ goto err_out;
}
}
@@ -989,9 +1121,10 @@ static int btusb_resume(struct usb_interface *intf)
err = btusb_submit_bulk_urb(hdev, GFP_NOIO);
if (err < 0) {
clear_bit(BTUSB_BULK_RUNNING, &data->flags);
- return err;
- } else
+ goto err_out;
+ } else {
btusb_submit_bulk_urb(hdev, GFP_NOIO);
+ }
}
if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
@@ -1001,16 +1134,35 @@ static int btusb_resume(struct usb_interface *intf)
btusb_submit_isoc_urb(hdev, GFP_NOIO);
}
+ spin_lock_irq(&data->txlock);
+ play_deferred(data);
+ clear_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->txlock);
+ schedule_work(&data->work);
+
return 0;
+
+err_out:
+ usb_scuttle_anchored_urbs(&data->deferred);
+no_io_needed:
+ spin_lock_irq(&data->txlock);
+ clear_bit(BTUSB_SUSPENDING, &data->flags);
+ spin_unlock_irq(&data->txlock);
+
+ return err;
}
+#endif
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)
Hi Oliver,
> > > +skip_waking:
> > > usb_anchor_urb(urb, &data->tx_anchor);
> > >
> > > err = usb_submit_urb(urb, GFP_ATOMIC);
> > > @@ -646,10 +723,13 @@ static int btusb_send_frame(struct sk_buff *skb)
> > > BT_ERR("%s urb %p submission failed", hdev->name, urb);
> > > kfree(urb->setup_packet);
> > > usb_unanchor_urb(urb);
> > > + } else {
> > > + usb_mark_last_busy(data->udev);
> > > }
> > >
> > > usb_free_urb(urb);
> > >
> > > +out:
> > > return err;
> > > }
> >
> > Please call the labels simply skip and done.
>
> In this particular case skip_waking seems to be clearer because it
> tells you right away what is skipped.
agree, keep it then.
> > > @@ -721,8 +801,19 @@ static void btusb_work(struct work_struct *work)
> > > {
> > > struct btusb_data *data = container_of(work, struct btusb_data,
> > > work); struct hci_dev *hdev = data->hdev;
> > > + int err;
> > >
> > > if (hdev->conn_hash.sco_num > 0) {
> > > + if (!data->did_iso_resume) {
> > > + err = usb_autopm_get_interface(data->isoc);
> > > + if (!err) {
> > > + data->did_iso_resume = 1;
> > > + } else {
> > > + clear_bit(BTUSB_ISOC_RUNNING,
> > > &data->flags);
> > > + usb_kill_anchored_urbs(&data->isoc_anchor);
> > > + return;
> > > + }
> >
> > Having this as like this is simpler to read:
> >
> > if (err < 0) {
> > clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > usb_kill_anchored_urbs(&data->isoc_anchor);
> > return;
> > }
> >
> > data->did_iso_resume = 1;
>
> Do you generally prefer the "else" branch implicit?
I prefer: if error, cleanup and leave function. It is easier to read and
understand by people that are not familiar enough with the code.
If we have a lot of else branches you have to think too much which one
gets executed in case of success. If the command and the follow command
in case of success are on the same indentation, I find it a lot simpler
to follow. My take on these :)
Regards
Marcel
QW0gTW9udGFnLCAyNC4gQXVndXN0IDIwMDkgMTI6MTY6MTUgc2NocmllYiBNYXJjZWwgSG9sdG1h
bm46Cj4gPiArc2tpcF93YWtpbmc6Cj4gPiCgoKCgoKB1c2JfYW5jaG9yX3VyYih1cmIsICZkYXRh
LT50eF9hbmNob3IpOwo+ID4goAo+ID4goKCgoKCgZXJyID0gdXNiX3N1Ym1pdF91cmIodXJiLCBH
RlBfQVRPTUlDKTsKPiA+IEBAIC02NDYsMTAgKzcyMywxMyBAQCBzdGF0aWMgaW50IGJ0dXNiX3Nl
bmRfZnJhbWUoc3RydWN0IHNrX2J1ZmYgKnNrYikKPiA+IKCgoKCgoKCgoKCgoKCgQlRfRVJSKCIl
cyB1cmIgJXAgc3VibWlzc2lvbiBmYWlsZWQiLCBoZGV2LT5uYW1lLCB1cmIpOwo+ID4goKCgoKCg
oKCgoKCgoKBrZnJlZSh1cmItPnNldHVwX3BhY2tldCk7Cj4gPiCgoKCgoKCgoKCgoKCgoHVzYl91
bmFuY2hvcl91cmIodXJiKTsKPiA+ICugoKCgoH0gZWxzZSB7Cj4gPiAroKCgoKCgoKCgoKCgoHVz
Yl9tYXJrX2xhc3RfYnVzeShkYXRhLT51ZGV2KTsKPiA+IKCgoKCgoH0KPiA+IKAKPiA+IKCgoKCg
oHVzYl9mcmVlX3VyYih1cmIpOwo+ID4goAo+ID4gK291dDoKPiA+IKCgoKCgoHJldHVybiBlcnI7
Cj4gPiCgfQo+Cj4gUGxlYXNlIGNhbGwgdGhlIGxhYmVscyBzaW1wbHkgc2tpcCBhbmQgZG9uZS4K
CkluIHRoaXMgcGFydGljdWxhciBjYXNlIHNraXBfd2FraW5nIHNlZW1zIHRvIGJlIGNsZWFyZXIg
YmVjYXVzZSBpdAp0ZWxscyB5b3UgcmlnaHQgYXdheSB3aGF0IGlzIHNraXBwZWQuCgo+ID4gQEAg
LTcyMSw4ICs4MDEsMTkgQEAgc3RhdGljIHZvaWQgYnR1c2Jfd29yayhzdHJ1Y3Qgd29ya19zdHJ1
Y3QgKndvcmspCj4gPiCgewo+ID4goKCgoKCgc3RydWN0IGJ0dXNiX2RhdGEgKmRhdGEgPSBjb250
YWluZXJfb2Yod29yaywgc3RydWN0IGJ0dXNiX2RhdGEsCj4gPiB3b3JrKTsgc3RydWN0IGhjaV9k
ZXYgKmhkZXYgPSBkYXRhLT5oZGV2Owo+ID4gK6CgoKCgaW50IGVycjsKPiA+IKAKPiA+IKCgoKCg
oGlmIChoZGV2LT5jb25uX2hhc2guc2NvX251bSA+IDApIHsKPiA+ICugoKCgoKCgoKCgoKCgaWYg
KCFkYXRhLT5kaWRfaXNvX3Jlc3VtZSkgewo+ID4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoGVyciA9
IHVzYl9hdXRvcG1fZ2V0X2ludGVyZmFjZShkYXRhLT5pc29jKTsKPiA+ICugoKCgoKCgoKCgoKCg
oKCgoKCgoKBpZiAoIWVycikgewo+ID4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgZGF0
YS0+ZGlkX2lzb19yZXN1bWUgPSAxOwo+ID4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoH0gZWxzZSB7
Cj4gPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBjbGVhcl9iaXQoQlRVU0JfSVNPQ19S
VU5OSU5HLAo+ID4gJmRhdGEtPmZsYWdzKTsKPiA+ICugoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgoHVzYl9raWxsX2FuY2hvcmVkX3VyYnMoJmRhdGEtPmlzb2NfYW5jaG9yKTsKPiA+ICugoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoHJldHVybjsKPiA+ICugoKCgoKCgoKCgoKCgoKCgoKCg
oKB9Cj4KPiBIYXZpbmcgdGhpcyBhcyBsaWtlIHRoaXMgaXMgc2ltcGxlciB0byByZWFkOgo+Cj4g
oKCgoKCgoKCgoKCgoKCgoGlmIChlcnIgPCAwKSB7Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
Y2xlYXJfYml0KEJUVVNCX0lTT0NfUlVOTklORywgJmRhdGEtPmZsYWdzKTsKPiCgoKCgoKCgoKCg
oKCgoKCgoKCgoKCgoKB1c2Jfa2lsbF9hbmNob3JlZF91cmJzKCZkYXRhLT5pc29jX2FuY2hvcik7
Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgcmV0dXJuOwo+IKCgoKCgoKCgoKCgoKCgoKB9Cj4K
PiCgoKCgoKCgoKCgoKCgoKCgZGF0YS0+ZGlkX2lzb19yZXN1bWUgPSAxOwoKRG8geW91IGdlbmVy
YWxseSBwcmVmZXIgdGhlICJlbHNlIiBicmFuY2ggaW1wbGljaXQ/CgpUaGUgcmVzdCBpcyBkb25l
LgoKCVJlZ2FyZHMKCQlPbGl2ZXIKCg==
Hi Oliver,
> Signed-off-by: Oliver Neukum <[email protected]>
>
> This patch adds support of USB autosuspend to the btusb driver
>
> If the device doesn't support remote wakeup, simple support based
> on up/down is provided. If the device supports remote wakeup, support
> for autosuspend while the interface is up is provided.
> This is done by queueing URBs in an anchor structure and waking the
> device up from a work queue.
>
> --
>
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -35,7 +35,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
>
> -#define VERSION "0.5"
> +#define VERSION "0.6"
>
> static int ignore_dga;
> static int ignore_csr;
> @@ -145,6 +145,7 @@ static struct usb_device_id blacklist_table[] = {
> #define BTUSB_INTR_RUNNING 0
> #define BTUSB_BULK_RUNNING 1
> #define BTUSB_ISOC_RUNNING 2
> +#define BTUSB_SUSPENDING 3
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -157,11 +158,15 @@ struct btusb_data {
> unsigned long flags;
>
> struct work_struct work;
> + struct work_struct waker;
>
> struct usb_anchor tx_anchor;
> struct usb_anchor intr_anchor;
> struct usb_anchor bulk_anchor;
> struct usb_anchor isoc_anchor;
> + struct usb_anchor deferred;
> + int tx_in_flight;
> + spinlock_t txlock;
>
> struct usb_endpoint_descriptor *intr_ep;
> struct usb_endpoint_descriptor *bulk_tx_ep;
> @@ -174,8 +179,24 @@ struct btusb_data {
> unsigned int sco_num;
> int isoc_altsetting;
> int suspend_count;
> + int did_iso_resume:1;
> };
>
> +static int inc_tx(struct btusb_data *data)
> +{
> + unsigned long flags;
> + int rv;
> +
> + spin_lock_irqsave(&data->txlock, flags);
> + rv = test_bit(BTUSB_SUSPENDING, &data->flags);
> + if (!rv)
> + data->tx_in_flight++;
> + spin_unlock_irqrestore(&data->txlock, flags);
> +
> + return rv;
> +}
> +
> +
> static void btusb_intr_complete(struct urb *urb)
please remove the extra empty line.
> {
> struct hci_dev *hdev = urb->context;
> @@ -202,6 +223,7 @@ static void btusb_intr_complete(struct urb *urb)
> if (!test_bit(BTUSB_INTR_RUNNING, &data->flags))
> return;
>
> + usb_mark_last_busy(data->udev);
> usb_anchor_urb(urb, &data->intr_anchor);
>
> err = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -327,6 +349,7 @@ static int btusb_submit_bulk_urb(struct hci_dev *hdev, gfp_t mem_flags)
>
> urb->transfer_flags |= URB_FREE_BUFFER;
>
> + usb_mark_last_busy(data->udev);
> usb_anchor_urb(urb, &data->bulk_anchor);
>
> err = usb_submit_urb(urb, mem_flags);
> @@ -465,6 +488,33 @@ static void btusb_tx_complete(struct urb *urb)
> {
> struct sk_buff *skb = urb->context;
> struct hci_dev *hdev = (struct hci_dev *) skb->dev;
> + struct btusb_data *data = hdev->driver_data;
> +
> + BT_DBG("%s urb %p status %d count %d", hdev->name,
> + urb, urb->status, urb->actual_length);
> +
> + if (!test_bit(HCI_RUNNING, &hdev->flags))
> + goto done;
> +
> + if (!urb->status)
> + hdev->stat.byte_tx += urb->transfer_buffer_length;
> + else
> + hdev->stat.err_tx++;
> +
> +done:
> + spin_lock(&data->txlock);
> + data->tx_in_flight--;
> + spin_unlock(&data->txlock);
> +
> + kfree(urb->setup_packet);
> +
> + kfree_skb(skb);
> +}
> +
> +static void btusb_isoc_tx_complete(struct urb *urb)
> +{
> + struct sk_buff *skb = urb->context;
> + struct hci_dev *hdev = (struct hci_dev *) skb->dev;
>
> BT_DBG("%s urb %p status %d count %d", hdev->name,
> urb, urb->status, urb->actual_length);
> @@ -490,11 +540,16 @@ static int btusb_open(struct hci_dev *hdev)
>
> BT_DBG("%s", hdev->name);
>
> + err = usb_autopm_get_interface(data->intf);
> + if (err < 0)
> + return err;
> + data->intf->needs_remote_wakeup = 1;
> +
Please add an extra empty line before data->intf->needs...
> if (test_and_set_bit(HCI_RUNNING, &hdev->flags))
> - return 0;
> + goto out;
>
> if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
> - return 0;
> + goto out;
>
> err = btusb_submit_intr_urb(hdev, GFP_KERNEL);
> if (err < 0)
> @@ -509,17 +564,28 @@ static int btusb_open(struct hci_dev *hdev)
> set_bit(BTUSB_BULK_RUNNING, &data->flags);
> btusb_submit_bulk_urb(hdev, GFP_KERNEL);
>
> +out:
> + usb_autopm_put_interface(data->intf);
> return 0;
Please call the out label done instead. I prefer to be consistent inside
the driver.
> failed:
> clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> clear_bit(HCI_RUNNING, &hdev->flags);
> + usb_autopm_put_interface(data->intf);
> return err;
> }
>
> +static void btusb_stop_traffic(struct btusb_data *data)
> +{
> + usb_kill_anchored_urbs(&data->intr_anchor);
> + usb_kill_anchored_urbs(&data->bulk_anchor);
> + usb_kill_anchored_urbs(&data->isoc_anchor);
> +}
> +
> static int btusb_close(struct hci_dev *hdev)
> {
> struct btusb_data *data = hdev->driver_data;
> + int err;
>
> BT_DBG("%s", hdev->name);
>
> @@ -529,13 +595,15 @@ static int btusb_close(struct hci_dev *hdev)
> cancel_work_sync(&data->work);
>
> clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> - usb_kill_anchored_urbs(&data->isoc_anchor);
> -
> clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> - usb_kill_anchored_urbs(&data->bulk_anchor);
> -
> clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> - usb_kill_anchored_urbs(&data->intr_anchor);
> +
> + btusb_stop_traffic(data);
> + err = usb_autopm_get_interface(data->intf);
> + if (!err) {
> + data->intf->needs_remote_wakeup = 0;
> + usb_autopm_put_interface(data->intf);
> + }
Please do this like this:
btusb_stop_traffic(data);
err = usb_autopm_get_interface(data->intf);
if (err < 0)
return 0;
data->intf->needs_remote_wakeup = 0;
usb_autopm_put_interface(data->intf);
>
> return 0;
> }
> @@ -622,7 +690,7 @@ static int btusb_send_frame(struct sk_buff *skb)
> urb->dev = data->udev;
> urb->pipe = pipe;
> urb->context = skb;
> - urb->complete = btusb_tx_complete;
> + urb->complete = btusb_isoc_tx_complete;
> urb->interval = data->isoc_tx_ep->bInterval;
>
> urb->transfer_flags = URB_ISO_ASAP;
> @@ -633,12 +701,21 @@ static int btusb_send_frame(struct sk_buff *skb)
> le16_to_cpu(data->isoc_tx_ep->wMaxPacketSize));
>
> hdev->stat.sco_tx++;
> - break;
> + goto skip_waking;
>
> default:
> return -EILSEQ;
> }
>
> + err = inc_tx(data);
> + if (err) {
> + usb_anchor_urb(urb, &data->deferred);
> + schedule_work(&data->waker);
> + err = 0;
> + goto out;
> + }
> +
> +skip_waking:
> usb_anchor_urb(urb, &data->tx_anchor);
>
> err = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -646,10 +723,13 @@ static int btusb_send_frame(struct sk_buff *skb)
> BT_ERR("%s urb %p submission failed", hdev->name, urb);
> kfree(urb->setup_packet);
> usb_unanchor_urb(urb);
> + } else {
> + usb_mark_last_busy(data->udev);
> }
>
> usb_free_urb(urb);
>
> +out:
> return err;
> }
Please call the labels simply skip and done.
> @@ -721,8 +801,19 @@ static void btusb_work(struct work_struct *work)
> {
> struct btusb_data *data = container_of(work, struct btusb_data, work);
> struct hci_dev *hdev = data->hdev;
> + int err;
>
> if (hdev->conn_hash.sco_num > 0) {
> + if (!data->did_iso_resume) {
> + err = usb_autopm_get_interface(data->isoc);
> + if (!err) {
> + data->did_iso_resume = 1;
> + } else {
> + clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> + usb_kill_anchored_urbs(&data->isoc_anchor);
> + return;
> + }
Having this as like this is simpler to read:
if (err < 0) {
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
usb_kill_anchored_urbs(&data->isoc_anchor);
return;
}
data->did_iso_resume = 1;
> + }
> if (data->isoc_altsetting != 2) {
> clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> usb_kill_anchored_urbs(&data->isoc_anchor);
> @@ -742,9 +833,23 @@ static void btusb_work(struct work_struct *work)
> usb_kill_anchored_urbs(&data->isoc_anchor);
>
> __set_isoc_interface(hdev, 0);
> + if (data->did_iso_resume) {
> + data->did_iso_resume = 0;
> + usb_autopm_put_interface(data->isoc);
> + }
> }
> }
>
> +static void btusb_waker(struct work_struct *work)
> +{
> + struct btusb_data *data = container_of(work, struct btusb_data, waker);
> + int err;
> +
> + err = usb_autopm_get_interface(data->intf);
> + if (!err)
> + usb_autopm_put_interface(data->intf);
> +}
> +
Same as above. Please do it like this:
err = usb_autopm_get_interface(data->intf);
if (err < 0)
return
usb_autopm_put_interface(data->intf);
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> @@ -814,11 +919,14 @@ static int btusb_probe(struct usb_interface *intf,
> spin_lock_init(&data->lock);
>
> INIT_WORK(&data->work, btusb_work);
> + INIT_WORK(&data->waker, btusb_waker);
> + spin_lock_init(&data->txlock);
>
> init_usb_anchor(&data->tx_anchor);
> init_usb_anchor(&data->intr_anchor);
> init_usb_anchor(&data->bulk_anchor);
> init_usb_anchor(&data->isoc_anchor);
> + init_usb_anchor(&data->deferred);
>
> hdev = hci_alloc_dev();
> if (!hdev) {
> @@ -943,6 +1051,7 @@ static void btusb_disconnect(struct usb_interface *intf)
> hci_free_dev(hdev);
> }
>
> +#ifdef CONFIG_PM
> static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> {
> struct btusb_data *data = usb_get_intfdata(intf);
> @@ -952,22 +1061,45 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> if (data->suspend_count++)
> return 0;
>
> + spin_lock_irq(&data->txlock);
> + if (!(interface_to_usbdev(intf)->auto_pm && data->tx_in_flight)) {
> + set_bit(BTUSB_SUSPENDING, &data->flags);
> + spin_unlock_irq(&data->txlock);
> + } else {
> + spin_unlock_irq(&data->txlock);
> + data->suspend_count--;
> + return -EBUSY;
> + }
> +
> cancel_work_sync(&data->work);
>
> + btusb_stop_traffic(data);
> usb_kill_anchored_urbs(&data->tx_anchor);
>
> - usb_kill_anchored_urbs(&data->isoc_anchor);
> - usb_kill_anchored_urbs(&data->bulk_anchor);
> - usb_kill_anchored_urbs(&data->intr_anchor);
> -
> return 0;
> }
>
> +static void play_deferred(struct btusb_data *data)
> +{
> + struct urb *urb;
> + int err;
> +
> + while ((urb = usb_get_from_anchor(&data->deferred))) {
> + err = usb_submit_urb(urb, GFP_ATOMIC);
> + if (err < 0)
> + break;
> + else
> + data->tx_in_flight++;
The else part is totally point less here;
if (err < 0)
break;
data->tx_in_flight++;
> +
> + }
> + usb_scuttle_anchored_urbs(&data->deferred);
> +}
> +
> static int btusb_resume(struct usb_interface *intf)
> {
> struct btusb_data *data = usb_get_intfdata(intf);
> struct hci_dev *hdev = data->hdev;
> - int err;
> + int err = 0;
>
> BT_DBG("intf %p", intf);
>
> @@ -975,13 +1107,13 @@ static int btusb_resume(struct usb_interface *intf)
> return 0;
>
> if (!test_bit(HCI_RUNNING, &hdev->flags))
> - return 0;
> + goto no_io_needed;
>
> if (test_bit(BTUSB_INTR_RUNNING, &data->flags)) {
> err = btusb_submit_intr_urb(hdev, GFP_NOIO);
> if (err < 0) {
> clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> - return err;
> + goto err_out;
> }
> }
>
> @@ -989,9 +1121,10 @@ static int btusb_resume(struct usb_interface *intf)
> err = btusb_submit_bulk_urb(hdev, GFP_NOIO);
> if (err < 0) {
> clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> - return err;
> - } else
> + goto err_out;
> + } else {
> btusb_submit_bulk_urb(hdev, GFP_NOIO);
> + }
> }
>
> if (test_bit(BTUSB_ISOC_RUNNING, &data->flags)) {
> @@ -1001,16 +1134,35 @@ static int btusb_resume(struct usb_interface *intf)
> btusb_submit_isoc_urb(hdev, GFP_NOIO);
> }
>
> + spin_lock_irq(&data->txlock);
> + play_deferred(data);
> + clear_bit(BTUSB_SUSPENDING, &data->flags);
> + spin_unlock_irq(&data->txlock);
> + schedule_work(&data->work);
> +
> return 0;
> +
> +err_out:
> + usb_scuttle_anchored_urbs(&data->deferred);
> +no_io_needed:
> + spin_lock_irq(&data->txlock);
> + clear_bit(BTUSB_SUSPENDING, &data->flags);
> + spin_unlock_irq(&data->txlock);
> +
> + return err;
> }
> +#endif
Please call this labels failed and done to be more consistent.
>
> static struct usb_driver btusb_driver = {
> .name = "btusb",
> .probe = btusb_probe,
> .disconnect = btusb_disconnect,
> +#ifdef CONFIG_PM
> .suspend = btusb_suspend,
> .resume = btusb_resume,
> +#endif
> .id_table = btusb_table,
> + .supports_autosuspend = 1,
> };
>
> static int __init btusb_init(void)
>
Regards
Marcel