[PATCH] hci_usb: implement suspend/resume

[PATCH] hci_usb: implement suspend/resume

[Johannes Berg]

The attached patch implements suspend/resume for the hci_usb bluetooth
driver by simply killing all outstanding urbs on suspend, and re-issuing
them on resume.

This allows me to actually use the internal bluetooth "dongle" in my
powerbook after suspend-to-ram without taking down all userland programs
(sdpd, ...) and the hci device and reloading the module.

Signed-Off-By: Johannes Berg <johannes@sipsolutions.net>

--- linux-2.6.15.1.orig/drivers/bluetooth/hci_usb.c	2006-01-18 00:08:54.840000000 +0100
+++ linux-2.6.15.1/drivers/bluetooth/hci_usb.c	2006-01-18 00:06:35.080000000 +0100
@@ -1043,11 +1043,55 @@
 	hci_free_dev(hdev);
 }
 
+static int hci_usb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct hci_usb *husb = usb_get_intfdata(intf);
+	int i;
+	unsigned long flags;
+	if (!husb || intf == husb->isoc_iface)
+		return 0;
+	
+	for (i = 0; i < 4; i++) {
+		struct _urb_queue *q = &husb->pending_q[i];
+		struct _urb *_urb;
+		spin_lock_irqsave(&q->lock, flags);
+		list_for_each_entry(_urb, &q->head, list)
+			usb_kill_urb(&_urb->urb);
+		spin_unlock_irqrestore(&q->lock, flags);
+	}
+	return 0;
+}
+
+static int hci_usb_resume(struct usb_interface *intf)
+{
+	struct hci_usb *husb = usb_get_intfdata(intf);
+	int i, err;
+	unsigned long flags;
+	if (!husb || intf == husb->isoc_iface)
+		return 0;
+	
+	for (i = 0; i < 4; i++) {
+		struct _urb_queue *q = &husb->pending_q[i];
+		struct _urb *_urb;
+		spin_lock_irqsave(&q->lock, flags);
+		list_for_each_entry(_urb, &q->head, list) {
+			err = usb_submit_urb(&_urb->urb, GFP_ATOMIC);
+			if (err) break;
+		}
+		spin_unlock_irqrestore(&q->lock, flags);
+		if (err)
+			return -EIO;
+	}
+	return 0;
+}
+
 static struct usb_driver hci_usb_driver = {
 	.owner		= THIS_MODULE,
 	.name		= "hci_usb",
 	.probe		= hci_usb_probe,
 	.disconnect	= hci_usb_disconnect,
+	.suspend	= hci_usb_suspend,
+	.resume		= hci_usb_resume,
 	.id_table	= bluetooth_ids,
 };
 

Re: [PATCH] hci_usb: implement suspend/resume

[Marcel Holtmann]

Hi Johannes,

> The attached patch implements suspend/resume for the hci_usb bluetooth
> driver by simply killing all outstanding urbs on suspend, and re-issuing
> them on resume.
> 
> This allows me to actually use the internal bluetooth "dongle" in my
> powerbook after suspend-to-ram without taking down all userland programs
> (sdpd, ...) and the hci device and reloading the module.

thanks for the patch. Due to the removed owner field it won't apply
cleanly to 2.6.16-rc1, but I can fix this easily by myself.

> +static int hci_usb_resume(struct usb_interface *intf)
> +{
> +	struct hci_usb *husb = usb_get_intfdata(intf);
> +	int i, err;
> +	unsigned long flags;
> +	if (!husb || intf == husb->isoc_iface)
> +		return 0;
> +	
> +	for (i = 0; i < 4; i++) {
> +		struct _urb_queue *q = &husb->pending_q[i];
> +		struct _urb *_urb;
> +		spin_lock_irqsave(&q->lock, flags);
> +		list_for_each_entry(_urb, &q->head, list) {
> +			err = usb_submit_urb(&_urb->urb, GFP_ATOMIC);
> +			if (err) break;
> +		}
> +		spin_unlock_irqrestore(&q->lock, flags);
> +		if (err)
> +			return -EIO;
> +	}
> +	return 0;
> +}

What happens if hci_usb_resume() really returns -EIO? Do we have to kill
the URBs again or does the USB subsystems disconnect the device in this
case?

Regards

Marcel

Re: [PATCH] hci_usb: implement suspend/resume

[Johannes Berg]

The attached patch implements suspend/resume for the hci_usb bluetooth
driver by simply killing all outstanding urbs on suspend, and re-issuing
them on resume.

This allows me to actually use the internal bluetooth "dongle" in my
powerbook after suspend-to-ram without taking down all userland programs
(sdpd, ...) and the hci device and reloading the module.

Signed-Off-By: Johannes Berg <johannes@sipsolutions.net>

--- linux-2.6.git.orig/drivers/bluetooth/hci_usb.c
+++ linux-2.6.git/drivers/bluetooth/hci_usb.c
@@ -1043,10 +1043,65 @@
 	hci_free_dev(hdev);
 }
 
+static int hci_usb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct hci_usb *husb = usb_get_intfdata(intf);
+	int i;
+	unsigned long flags;
+	struct list_head killed;
+
+	if (!husb || intf == husb->isoc_iface)
+		return 0;
+
+	INIT_LIST_HEAD(&killed);
+
+	for (i = 0; i < 4; i++) {
+		struct _urb_queue *q = &husb->pending_q[i];
+		struct _urb *_urb, *_tmp;
+		while ((_urb = _urb_dequeue(q))) {
+			/* reset queue since _urb_dequeue sets it to NULL */
+			_urb->queue = q;
+			usb_kill_urb(&_urb->urb);
+			list_add(&_urb->list, &killed);
+		}
+		spin_lock_irqsave(&q->lock, flags);
+		list_for_each_entry_safe(_urb, _tmp, &killed, list) {
+			list_move_tail(&_urb->list, &q->head);
+		}
+		spin_unlock_irqrestore(&q->lock, flags);
+	}
+	return 0;
+}
+
+static int hci_usb_resume(struct usb_interface *intf)
+{
+	struct hci_usb *husb = usb_get_intfdata(intf);
+	int i, err = 0;
+	unsigned long flags;
+	if (!husb || intf == husb->isoc_iface)
+		return 0;
+	
+	for (i = 0; i < 4; i++) {
+		struct _urb_queue *q = &husb->pending_q[i];
+		struct _urb *_urb;
+		spin_lock_irqsave(&q->lock, flags);
+		list_for_each_entry(_urb, &q->head, list) {
+			err = usb_submit_urb(&_urb->urb, GFP_ATOMIC);
+			if (err) break;
+		}
+		spin_unlock_irqrestore(&q->lock, flags);
+		if (err)
+			return -EIO;
+	}
+	return 0;
+}
+
 static struct usb_driver hci_usb_driver = {
 	.name		= "hci_usb",
 	.probe		= hci_usb_probe,
 	.disconnect	= hci_usb_disconnect,
+	.suspend	= hci_usb_suspend,
+	.resume		= hci_usb_resume,
 	.id_table	= bluetooth_ids,
 };
 

Re: [PATCH] hci_usb: implement suspend/resume

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Wed, 2006-01-18 at 19:39 +0100, Johannes Berg wrote:
> The attached patch implements suspend/resume for the hci_usb bluetooth
> driver by simply killing all outstanding urbs on suspend, and re-issuing
> them on resume.
> 
> This allows me to actually use the internal bluetooth "dongle" in my
> powerbook after suspend-to-ram without taking down all userland programs
> (sdpd, ...) and the hci device and reloading the module.

Can someone push this patch for 2.6.17 now that 2.6.16 is out? Or is
there still anything fundamentally wrong with it? I've been waiting for
it forever now ;)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH] hci_usb: implement suspend/resume

[Marcel Holtmann]

Hi Johannes,

> > The attached patch implements suspend/resume for the hci_usb bluetooth
> > driver by simply killing all outstanding urbs on suspend, and re-issuing
> > them on resume.
> > 
> > This allows me to actually use the internal bluetooth "dongle" in my
> > powerbook after suspend-to-ram without taking down all userland programs
> > (sdpd, ...) and the hci device and reloading the module.
> 
> Can someone push this patch for 2.6.17 now that 2.6.16 is out? Or is
> there still anything fundamentally wrong with it? I've been waiting for
> it forever now ;)

I will push it with some other small changes in the next few days.

Regards

Marcel

Re: [PATCH] hci_usb: implement suspend/resume

[Johannes Berg]

On Tue, 2006-03-21 at 10:52 +0100, Marcel Holtmann wrote:

> I will push it with some other small changes in the next few days.

I don't see it yet, should I just post it instead?

johannes

Re: [PATCH] hci_usb: implement suspend/resume

[Oliver Neukum]

Am Mittwoch, 18. Januar 2006 00:21 schrieb Johannes Berg:
> The attached patch implements suspend/resume for the hci_usb bluetooth
> driver by simply killing all outstanding urbs on suspend, and re-issuing
> them on resume.
> 
> This allows me to actually use the internal bluetooth "dongle" in my
> powerbook after suspend-to-ram without taking down all userland programs
> (sdpd, ...) and the hci device and reloading the module.
> 
> Signed-Off-By: Johannes Berg <johannes@sipsolutions.net>
> 
> --- linux-2.6.15.1.orig/drivers/bluetooth/hci_usb.c	2006-01-18 00:08:54.840000000 +0100
> +++ linux-2.6.15.1/drivers/bluetooth/hci_usb.c	2006-01-18 00:06:35.080000000 +0100
> @@ -1043,11 +1043,55 @@
>  	hci_free_dev(hdev);
>  }
>  
> +static int hci_usb_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct hci_usb *husb = usb_get_intfdata(intf);
> +	int i;
> +	unsigned long flags;
> +	if (!husb || intf == husb->isoc_iface)
> +		return 0;
> +	
> +	for (i = 0; i < 4; i++) {
> +		struct _urb_queue *q = &husb->pending_q[i];
> +		struct _urb *_urb;
> +		spin_lock_irqsave(&q->lock, flags);
> +		list_for_each_entry(_urb, &q->head, list)
> +			usb_kill_urb(&_urb->urb);
> +		spin_unlock_irqrestore(&q->lock, flags);
> +	}
> +	return 0;

This patch is wrong. usb_kill_urb() will sleep. You must not use it under
a spinlock.

	Regards
		Oliver

Re: [PATCH] hci_usb: implement suspend/resume

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

On Wed, 2006-01-18 at 14:25 +0100, Oliver Neukum wrote:

> This patch is wrong. usb_kill_urb() will sleep. You must not use it under
> a spinlock.

Whoops. Good catch. I'll have to analyse the logic with the lists being
used here (and probably add a temporary list). Will try to get a new
patch until tomorrow.

[side note: how about adding might_sleep() to usb_kill_urb? Then I'd at
least have noticed this right away]

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] hci_usb: implement suspend/resume

[Oliver Neukum]

Am Mittwoch, 18. Januar 2006 15:13 schrieb Johannes Berg:
> On Wed, 2006-01-18 at 14:25 +0100, Oliver Neukum wrote:
> 
> > This patch is wrong. usb_kill_urb() will sleep. You must not use it under
> > a spinlock.
> 
> Whoops. Good catch. I'll have to analyse the logic with the lists being
> used here (and probably add a temporary list). Will try to get a new
> patch until tomorrow.
> 
> [side note: how about adding might_sleep() to usb_kill_urb? Then I'd at
> least have noticed this right away]

Good idea.

	Regards
		Oliver

Re: [linux-usb-devel] Re: [PATCH] hci_usb: implement suspend/resume

[Greg KH]

On Wed, Jan 18, 2006 at 04:34:08PM +0100, Oliver Neukum wrote:
> Am Mittwoch, 18. Januar 2006 15:13 schrieb Johannes Berg:
> > On Wed, 2006-01-18 at 14:25 +0100, Oliver Neukum wrote:
> > 
> > > This patch is wrong. usb_kill_urb() will sleep. You must not use it under
> > > a spinlock.
> > 
> > Whoops. Good catch. I'll have to analyse the logic with the lists being
> > used here (and probably add a temporary list). Will try to get a new
> > patch until tomorrow.
> > 
> > [side note: how about adding might_sleep() to usb_kill_urb? Then I'd at
> > least have noticed this right away]
> 
> Good idea.

Done.

thanks,

greg k-h