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 <oliver@neukum.org>
> > 
> > --
> > 
> > --- 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 majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html