[PATCH v2] usbnet: enable more aggressive autosuspend

[PATCH v2] usbnet: enable more aggressive autosuspend

[Per Hallsmark]

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

Enable more aggressive autosuspend in usbnet.
Some commenting and cleanups done.
Signed-off-by: Per Hallsmark <per-46Ss7X5r2El6nIa31vAvqA@public.gmane.org>

[-- Attachment #2: usbnet_autosuspend_v2.patch --]
[-- Type: text/plain, Size: 7751 bytes --]

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 8463efb..a2f0c8b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -87,6 +87,8 @@ static int msg_level = -1;
 module_param (msg_level, int, 0);
 MODULE_PARM_DESC (msg_level, "Override default message level");
 
+static void waker(struct work_struct *work);
+
 /*-------------------------------------------------------------------------*/
 
 /* handles CDC Ethernet and many other network "bulk data" interfaces */
@@ -325,6 +327,7 @@ static void rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
 	if (netif_running (dev->net)
 			&& netif_device_present (dev->net)
 			&& !test_bit (EVENT_RX_HALT, &dev->flags)) {
+		usb_mark_last_busy(dev->udev);
 		switch (retval = usb_submit_urb (urb, GFP_ATOMIC)) {
 		case -EPIPE:
 			usbnet_defer_kevent (dev, EVENT_RX_HALT);
@@ -496,6 +499,7 @@ static void intr_complete (struct urb *urb)
 		return;
 
 	memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
+	usb_mark_last_busy(dev->udev);
 	status = usb_submit_urb (urb, GFP_ATOMIC);
 	if (status != 0 && netif_msg_timer (dev))
 		deverr(dev, "intr resubmit --> %d", status);
@@ -589,6 +593,10 @@ static int usbnet_stop (struct net_device *net)
 	dev->flags = 0;
 	del_timer_sync (&dev->delay);
 	tasklet_kill (&dev->bh);
+
+	dev->used--;
+
+	dev->intf->needs_remote_wakeup = 0;
 	usb_autopm_put_interface(dev->intf);
 
 	return 0;
@@ -669,6 +677,11 @@ static int usbnet_open (struct net_device *net)
 
 	// delay posting reads until we're fully open
 	tasklet_schedule (&dev->bh);
+
+	dev->used++;
+
+	dev->intf->needs_remote_wakeup = 1;
+	usb_autopm_put_interface(dev->intf);
 	return retval;
 done:
 	usb_autopm_put_interface(dev->intf);
@@ -921,7 +934,7 @@ static void usbnet_tx_timeout (struct net_device *net)
 
 /*-------------------------------------------------------------------------*/
 
-static int usbnet_start_xmit (struct sk_buff *skb, struct net_device *net)
+static int __usbnet_start_xmit (struct sk_buff *skb, struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
 	int			length;
@@ -955,6 +968,7 @@ static int usbnet_start_xmit (struct sk_buff *skb, struct net_device *net)
 	entry->state = tx_start;
 	entry->length = length;
 
+	dev->tx_goingon = 1;
 	usb_fill_bulk_urb (urb, dev->udev, dev->out,
 			skb->data, skb->len, tx_complete, skb);
 
@@ -972,6 +986,7 @@ static int usbnet_start_xmit (struct sk_buff *skb, struct net_device *net)
 
 	spin_lock_irqsave (&dev->txq.lock, flags);
 
+	usb_mark_last_busy(dev->udev);
 	switch ((retval = usb_submit_urb (urb, GFP_ATOMIC))) {
 	case -EPIPE:
 		netif_stop_queue (net);
@@ -1005,6 +1020,28 @@ drop:
 	return retval;
 }
 
+static int usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
+{
+	struct usbnet		*dev = netdev_priv(net);
+	int			retval;
+	unsigned long		flags;
+
+	spin_lock_irqsave(&dev->txq.lock, flags);
+	if (dev->suspend_count) {
+		netif_stop_queue(net);
+		dev->tx_skb = skb;
+		if (!schedule_work (&dev->waker))
+			deverr(dev, "waker may have been dropped");
+		else
+			devdbg(dev, "waker scheduled");
+		spin_unlock_irqrestore(&dev->txq.lock, flags);
+		return NET_XMIT_SUCCESS;
+	}
+	spin_unlock_irqrestore(&dev->txq.lock, flags);
+
+	retval = __usbnet_start_xmit(skb, net);
+	return retval;
+}
 
 /*-------------------------------------------------------------------------*/
 
@@ -1024,6 +1061,8 @@ static void usbnet_bh (unsigned long param)
 			rx_process (dev, skb);
 			continue;
 		case tx_done:
+			dev->tx_goingon = 0;
+			/* fall through */
 		case rx_cleanup:
 			usb_free_urb (entry->urb);
 			dev_kfree_skb (skb);
@@ -1043,6 +1082,7 @@ static void usbnet_bh (unsigned long param)
 	} else if (netif_running (dev->net)
 			&& netif_device_present (dev->net)
 			&& !timer_pending (&dev->delay)
+			&& !dev->suspend_count
 			&& !test_bit (EVENT_RX_HALT, &dev->flags)) {
 		int	temp = dev->rxq.qlen;
 		int	qlen = RX_QLEN (dev);
@@ -1161,6 +1201,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	dev->bh.func = usbnet_bh;
 	dev->bh.data = (unsigned long) dev;
 	INIT_WORK (&dev->kevent, kevent);
+	INIT_WORK (&dev->waker, waker);
 	dev->delay.function = usbnet_bh;
 	dev->delay.data = (unsigned long) dev;
 	init_timer (&dev->delay);
@@ -1269,24 +1313,66 @@ EXPORT_SYMBOL_GPL(usbnet_probe);
  * resume only when the last interface is resumed
  */
 
+static void waker(struct work_struct *work)
+{
+	struct usbnet *dev = container_of(work, struct usbnet, waker);
+
+	if (!usb_autopm_get_interface(dev->intf)) {
+		usb_autopm_put_interface(dev->intf);
+	} else {
+		devdbg(dev, "autoresume failed");
+	}
+}
+
+static void stop_traffic(struct usbnet *dev)
+{
+	int temp;
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK (unlink_wakeup);
+	DECLARE_WAITQUEUE (wait, current);
+
+	/* ensure there are no more active urbs */
+	add_wait_queue (&unlink_wakeup, &wait);
+	dev->wait = &unlink_wakeup;
+	temp = unlink_urbs (dev, &dev->txq) + unlink_urbs (dev, &dev->rxq);
+
+	/* maybe wait for deletions to finish. */
+	while (!skb_queue_empty(&dev->rxq)
+			&& !skb_queue_empty(&dev->txq)
+			&& !skb_queue_empty(&dev->done)) {
+		msleep(UNLINK_TIMEOUT_MS);
+		if (netif_msg_ifdown (dev))
+			devdbg (dev, "waited for %d urb completions", temp);
+	}
+	dev->wait = NULL;
+	remove_wait_queue (&unlink_wakeup, &wait);
+
+	usb_kill_urb(dev->interrupt);
+}
+
 int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 {
-	struct usbnet		*dev = usb_get_intfdata(intf);
+	struct usbnet *dev = usb_get_intfdata(intf);
 
-	if (!dev->suspend_count++) {
-		/*
-		 * accelerate emptying of the rx and queues, to avoid
-		 * having everything error out.
-		 */
-		netif_device_detach (dev->net);
-		(void) unlink_urbs (dev, &dev->rxq);
-		(void) unlink_urbs (dev, &dev->txq);
-		/*
-		 * reattach so runtime management can use and
-		 * wake the device
-		 */
-		netif_device_attach (dev->net);
+	devdbg(dev, "%s: begin", __FUNCTION__);
+
+	if (dev->suspend_count++)
+		return 0;
+
+	/* check for ongoing tx traffic */
+	if (dev->tx_goingon && dev->udev->auto_pm) {
+		dev->suspend_count--;
+		return -EBUSY;
 	}
+
+	stop_traffic(dev);
+
+	/* cancel work */
+	dev->flags = 0;
+	del_timer_sync(&dev->delay);
+	cancel_work_sync(&dev->kevent);
+
+	devdbg(dev, "%s: end", __FUNCTION__);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usbnet_suspend);
@@ -1294,9 +1380,32 @@ EXPORT_SYMBOL_GPL(usbnet_suspend);
 int usbnet_resume (struct usb_interface *intf)
 {
 	struct usbnet		*dev = usb_get_intfdata(intf);
+	int                     status;
+
+	devdbg(dev, "%s: begin", __FUNCTION__);
+
+	if (--dev->suspend_count)
+		return 0;
+
+	status = init_status (dev, dev->intf);
+	if (dev->interrupt) {
+		status = usb_submit_urb (dev->interrupt, GFP_KERNEL);
+		if (status < 0) {
+			devdbg(dev, "failed restarting interrupt urb");
+		}
+	}
+
+	tasklet_schedule(&dev->bh);
+
+	/* transmit package that triggered resume */
+	if (dev->tx_skb) {
+		status = __usbnet_start_xmit(dev->tx_skb, dev->net);
+		dev->tx_skb = NULL;
+	}
+
+	netif_wake_queue(dev->net);
 
-	if (!--dev->suspend_count)
-		tasklet_schedule (&dev->bh);
+	devdbg(dev, "%s: end", __FUNCTION__);
 
 	return 0;
 }
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index ba09fe8..93f3625 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -64,6 +64,12 @@ struct usbnet {
 #		define EVENT_RX_MEMORY	2
 #		define EVENT_STS_SPLIT	3
 #		define EVENT_LINK_RESET	4
+
+	/* autosuspend helpers */
+	struct work_struct	waker;
+	int                     used;
+	int                     tx_goingon;
+	struct sk_buff		*tx_skb; /* skb queued during suspend */
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Alan Stern]

On Thu, 23 Oct 2008, Per Hallsmark wrote:

> Enable more aggressive autosuspend in usbnet.
> Some commenting and cleanups done.
> Signed-off-by: Per Hallsmark <per@hallsmark.se>

I've been considering submitting the patch below.  It would help your 
work; you could remove all the "waker" stuff.

Do you think it would be worthwhile?

Alan Stern



Index: usb-2.6/Documentation/usb/power-management.txt
===================================================================
--- usb-2.6.orig/Documentation/usb/power-management.txt
+++ usb-2.6/Documentation/usb/power-management.txt
@@ -313,11 +313,13 @@ three of the methods listed above.  In a
 that it supports autosuspend by setting the .supports_autosuspend flag
 in its usb_driver structure.  It is then responsible for informing the
 USB core whenever one of its interfaces becomes busy or idle.  The
-driver does so by calling these three functions:
+driver does so by calling these five functions:
 
 	int  usb_autopm_get_interface(struct usb_interface *intf);
 	void usb_autopm_put_interface(struct usb_interface *intf);
 	int  usb_autopm_set_interface(struct usb_interface *intf);
+	int  usb_autopm_get_interface_async(struct usb_interface *intf);
+	void usb_autopm_put_interface_async(struct usb_interface *intf);
 
 The functions work by maintaining a counter in the usb_interface
 structure.  When intf->pm_usage_count is > 0 then the interface is
@@ -330,10 +332,12 @@ associated with the device itself rather
 This field is used only by the USB core.)
 
 The driver owns intf->pm_usage_count; it can modify the value however
-and whenever it likes.  A nice aspect of the usb_autopm_* routines is
-that the changes they make are protected by the usb_device structure's
-PM mutex (udev->pm_mutex); however drivers may change pm_usage_count
-without holding the mutex.
+and whenever it likes.  A nice aspect of the non-async usb_autopm_*
+routines is that the changes they make are protected by the usb_device
+structure's PM mutex (udev->pm_mutex); however drivers may change
+pm_usage_count without holding the mutex.  Drivers using the async
+routines are responsible for their own synchronization and mutual
+exclusion.
 
 	usb_autopm_get_interface() increments pm_usage_count and
 	attempts an autoresume if the new value is > 0 and the
@@ -348,6 +352,14 @@ without holding the mutex.
 	is suspended, and it attempts an autosuspend if the value is
 	<= 0 and the device isn't suspended.
 
+	usb_autopm_get_interface_async() and
+	usb_autopm_put_interface_async() do almost the same things as
+	their non-async counterparts.  The differences are: they do
+	not acquire the PM mutex, and they use a workqueue to do their
+	jobs.  As a result they can be called in an atomic context,
+	such as an URB's completion handler, but when they return the
+	device will not generally not yet be in the desired state.
+
 There also are a couple of utility routines drivers can use:
 
 	usb_autopm_enable() sets pm_usage_cnt to 0 and then calls
Index: usb-2.6/include/linux/usb.h
===================================================================
--- usb-2.6.orig/include/linux/usb.h
+++ usb-2.6/include/linux/usb.h
@@ -396,6 +396,7 @@ struct usb_tt;
  * @urbnum: number of URBs submitted for the whole device
  * @active_duration: total time device is not suspended
  * @autosuspend: for delayed autosuspends
+ * @autoresume: for autoresumes requested while in_interrupt
  * @pm_mutex: protects PM operations
  * @last_busy: time of last use
  * @autosuspend_delay: in jiffies
@@ -474,6 +475,7 @@ struct usb_device {
 
 #ifdef CONFIG_PM
 	struct delayed_work autosuspend;
+	struct work_struct autoresume;
 	struct mutex pm_mutex;
 
 	unsigned long last_busy;
@@ -511,6 +513,8 @@ extern struct usb_device *usb_find_devic
 extern int usb_autopm_set_interface(struct usb_interface *intf);
 extern int usb_autopm_get_interface(struct usb_interface *intf);
 extern void usb_autopm_put_interface(struct usb_interface *intf);
+extern int usb_autopm_get_interface_async(struct usb_interface *intf);
+extern void usb_autopm_put_interface_async(struct usb_interface *intf);
 
 static inline void usb_autopm_enable(struct usb_interface *intf)
 {
@@ -537,8 +541,13 @@ static inline int usb_autopm_set_interfa
 static inline int usb_autopm_get_interface(struct usb_interface *intf)
 { return 0; }
 
+static inline int usb_autopm_get_interface_async(struct usb_interface *intf)
+{ return 0; }
+
 static inline void usb_autopm_put_interface(struct usb_interface *intf)
 { }
+static inline void usb_autopm_put_interface_async(struct usb_interface *intf)
+{ }
 static inline void usb_autopm_enable(struct usb_interface *intf)
 { }
 static inline void usb_autopm_disable(struct usb_interface *intf)
Index: usb-2.6/drivers/usb/core/usb.h
===================================================================
--- usb-2.6.orig/drivers/usb/core/usb.h
+++ usb-2.6/drivers/usb/core/usb.h
@@ -45,6 +45,7 @@ extern int usb_suspend(struct device *de
 extern int usb_resume(struct device *dev);
 
 extern void usb_autosuspend_work(struct work_struct *work);
+extern void usb_autoresume_work(struct work_struct *work);
 extern int usb_port_suspend(struct usb_device *dev);
 extern int usb_port_resume(struct usb_device *dev);
 extern int usb_external_suspend_device(struct usb_device *udev,
Index: usb-2.6/drivers/usb/core/usb.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/usb.c
+++ usb-2.6/drivers/usb/core/usb.c
@@ -402,6 +402,7 @@ struct usb_device *usb_alloc_dev(struct 
 #ifdef	CONFIG_PM
 	mutex_init(&dev->pm_mutex);
 	INIT_DELAYED_WORK(&dev->autosuspend, usb_autosuspend_work);
+	INIT_WORK(&dev->autoresume, usb_autoresume_work);
 	dev->autosuspend_delay = usb_autosuspend_delay * HZ;
 	dev->connect_time = jiffies;
 	dev->active_duration = -jiffies;
Index: usb-2.6/drivers/usb/core/hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hub.c
+++ usb-2.6/drivers/usb/core/hub.c
@@ -1362,8 +1362,9 @@ static void usb_stop_pm(struct usb_devic
 		usb_autosuspend_device(udev->parent);
 	usb_pm_unlock(udev);
 
-	/* Stop any autosuspend requests already submitted */
-	cancel_rearming_delayed_work(&udev->autosuspend);
+	/* Stop any autosuspend or autoresume requests already submitted */
+	cancel_delayed_work_sync(&udev->autosuspend);
+	cancel_work_sync(&udev->autoresume);
 }
 
 #else
Index: usb-2.6/drivers/usb/core/driver.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/driver.c
+++ usb-2.6/drivers/usb/core/driver.c
@@ -1339,6 +1339,19 @@ void usb_autosuspend_work(struct work_st
 	usb_autopm_do_device(udev, 0);
 }
 
+/* usb_autoresume_work - callback routine to autoresume a USB device */
+void usb_autoresume_work(struct work_struct *work)
+{
+	struct usb_device *udev =
+		container_of(work, struct usb_device, autoresume);
+
+	/* Wake it up, let the drivers do their thing, and then put it
+	 * back to sleep.
+	 */
+	if (usb_autopm_do_device(udev, 1) == 0)
+		usb_autopm_do_device(udev, -1);
+}
+
 /**
  * usb_autosuspend_device - delayed autosuspend of a USB device and its interfaces
  * @udev: the usb_device to autosuspend
@@ -1490,6 +1503,45 @@ void usb_autopm_put_interface(struct usb
 EXPORT_SYMBOL_GPL(usb_autopm_put_interface);
 
 /**
+ * usb_autopm_put_interface_async - decrement a USB interface's PM-usage counter
+ * @intf: the usb_interface whose counter should be decremented
+ *
+ * This routine does essentially the same thing as
+ * usb_autopm_put_interface(): it decrements @intf's usage counter and
+ * queues a delayed autosuspend request if the counter is <= 0.  The
+ * difference is that it does not acquire the device's pm_mutex;
+ * callers must handle all synchronization issues themselves.
+ *
+ * Typically a driver would call this routine during an URB's completion
+ * handler, if no more URBs were pending.
+ *
+ * This routine can run in atomic context.
+ */
+void usb_autopm_put_interface_async(struct usb_interface *intf)
+{
+	struct usb_device	*udev = interface_to_usbdev(intf);
+	int			status = 0;
+
+	if (intf->condition == USB_INTERFACE_UNBOUND) {
+		status = -ENODEV;
+	} else {
+		udev->last_busy = jiffies;
+		--intf->pm_usage_cnt;
+		if (udev->autosuspend_disabled || udev->autosuspend_delay < 0)
+			status = -EPERM;
+		else if (intf->pm_usage_cnt <= 0 &&
+				!timer_pending(&udev->autosuspend.timer)) {
+			queue_delayed_work(ksuspend_usb_wq, &udev->autosuspend,
+					round_jiffies_relative(
+						udev->autosuspend_delay));
+		}
+	}
+	dev_vdbg(&intf->dev, "%s: status %d cnt %d\n",
+			__func__, status, intf->pm_usage_cnt);
+}
+EXPORT_SYMBOL_GPL(usb_autopm_put_interface_async);
+
+/**
  * usb_autopm_get_interface - increment a USB interface's PM-usage counter
  * @intf: the usb_interface whose counter should be incremented
  *
@@ -1535,6 +1587,37 @@ int usb_autopm_get_interface(struct usb_
 EXPORT_SYMBOL_GPL(usb_autopm_get_interface);
 
 /**
+ * usb_autopm_get_interface_async - increment a USB interface's PM-usage counter
+ * @intf: the usb_interface whose counter should be incremented
+ *
+ * This routine does much the same thing as
+ * usb_autopm_get_interface(): it increments @intf's usage counter and
+ * queues an autoresume request if the result is > 0.  The differences
+ * are that it does not acquire the device's pm_mutex (callers must
+ * handle all synchronization issues themselves), and it does not
+ * autoresume the device directly (it only queues a request).  After a
+ * successful call, the device will generally not yet be resumed.
+ *
+ * This routine can run in atomic context.
+ */
+int usb_autopm_get_interface_async(struct usb_interface *intf)
+{
+	struct usb_device	*udev = interface_to_usbdev(intf);
+	int			status = 0;
+
+	if (intf->condition == USB_INTERFACE_UNBOUND)
+		status = -ENODEV;
+	else if (udev->autoresume_disabled)
+		status = -EPERM;
+	else if (++intf->pm_usage_cnt > 0 && udev->state == USB_STATE_SUSPENDED)
+		queue_work(ksuspend_usb_wq, &udev->autoresume);
+	dev_vdbg(&intf->dev, "%s: status %d cnt %d\n",
+			__func__, status, intf->pm_usage_cnt);
+	return status;
+}
+EXPORT_SYMBOL_GPL(usb_autopm_get_interface_async);
+
+/**
  * usb_autopm_set_interface - set a USB interface's autosuspend state
  * @intf: the usb_interface whose state should be set
  *

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Per Hallsmark]

Alan Stern wrote:
> On Thu, 23 Oct 2008, Per Hallsmark wrote:
>
>   
>> Enable more aggressive autosuspend in usbnet.
>> Some commenting and cleanups done.
>> Signed-off-by: Per Hallsmark <per-46Ss7X5r2El6nIa31vAvqA@public.gmane.org>
>>     
>
> I've been considering submitting the patch below.  It would help your 
> work; you could remove all the "waker" stuff.
>   
So, if I understand the patch correctly, the waker's in cdc-acm, cdc-wdm 
and usbnet
could then be removed and a change to use *_async() should be done?
I work with a module that have usb interfaces driven by above three 
drivers so it
could be a good testing case I guess.
Generic solutions are the best so if this approach would work then of 
course a
change should be made I think. I have no time to test it in near time 
though,
perhaps later next week.
> Do you think it would be worthwhile?
>
> Alan Stern
>
>
>
> Index: usb-2.6/Documentation/usb/power-management.txt
> ===================================================================
> --- usb-2.6.orig/Documentation/usb/power-management.txt
> +++ usb-2.6/Documentation/usb/power-management.txt
> @@ -313,11 +313,13 @@ three of the methods listed above.  In a
>  that it supports autosuspend by setting the .supports_autosuspend flag
>  in its usb_driver structure.  It is then responsible for informing the
>  USB core whenever one of its interfaces becomes busy or idle.  The
> -driver does so by calling these three functions:
> +driver does so by calling these five functions:
>  
>  	int  usb_autopm_get_interface(struct usb_interface *intf);
>  	void usb_autopm_put_interface(struct usb_interface *intf);
>  	int  usb_autopm_set_interface(struct usb_interface *intf);
> +	int  usb_autopm_get_interface_async(struct usb_interface *intf);
> +	void usb_autopm_put_interface_async(struct usb_interface *intf);
>  
>  The functions work by maintaining a counter in the usb_interface
>  structure.  When intf->pm_usage_count is > 0 then the interface is
> @@ -330,10 +332,12 @@ associated with the device itself rather
>  This field is used only by the USB core.)
>  
>  The driver owns intf->pm_usage_count; it can modify the value however
> -and whenever it likes.  A nice aspect of the usb_autopm_* routines is
> -that the changes they make are protected by the usb_device structure's
> -PM mutex (udev->pm_mutex); however drivers may change pm_usage_count
> -without holding the mutex.
> +and whenever it likes.  A nice aspect of the non-async usb_autopm_*
> +routines is that the changes they make are protected by the usb_device
> +structure's PM mutex (udev->pm_mutex); however drivers may change
> +pm_usage_count without holding the mutex.  Drivers using the async
> +routines are responsible for their own synchronization and mutual
> +exclusion.
>  
>  	usb_autopm_get_interface() increments pm_usage_count and
>  	attempts an autoresume if the new value is > 0 and the
> @@ -348,6 +352,14 @@ without holding the mutex.
>  	is suspended, and it attempts an autosuspend if the value is
>  	<= 0 and the device isn't suspended.
>  
> +	usb_autopm_get_interface_async() and
> +	usb_autopm_put_interface_async() do almost the same things as
> +	their non-async counterparts.  The differences are: they do
> +	not acquire the PM mutex, and they use a workqueue to do their
> +	jobs.  As a result they can be called in an atomic context,
> +	such as an URB's completion handler, but when they return the
> +	device will not generally not yet be in the desired state.
> +
>  There also are a couple of utility routines drivers can use:
>  
>  	usb_autopm_enable() sets pm_usage_cnt to 0 and then calls
> Index: usb-2.6/include/linux/usb.h
> ===================================================================
> --- usb-2.6.orig/include/linux/usb.h
> +++ usb-2.6/include/linux/usb.h
> @@ -396,6 +396,7 @@ struct usb_tt;
>   * @urbnum: number of URBs submitted for the whole device
>   * @active_duration: total time device is not suspended
>   * @autosuspend: for delayed autosuspends
> + * @autoresume: for autoresumes requested while in_interrupt
>   * @pm_mutex: protects PM operations
>   * @last_busy: time of last use
>   * @autosuspend_delay: in jiffies
> @@ -474,6 +475,7 @@ struct usb_device {
>  
>  #ifdef CONFIG_PM
>  	struct delayed_work autosuspend;
> +	struct work_struct autoresume;
>  	struct mutex pm_mutex;
>  
>  	unsigned long last_busy;
> @@ -511,6 +513,8 @@ extern struct usb_device *usb_find_devic
>  extern int usb_autopm_set_interface(struct usb_interface *intf);
>  extern int usb_autopm_get_interface(struct usb_interface *intf);
>  extern void usb_autopm_put_interface(struct usb_interface *intf);
> +extern int usb_autopm_get_interface_async(struct usb_interface *intf);
> +extern void usb_autopm_put_interface_async(struct usb_interface *intf);
>  
>  static inline void usb_autopm_enable(struct usb_interface *intf)
>  {
> @@ -537,8 +541,13 @@ static inline int usb_autopm_set_interfa
>  static inline int usb_autopm_get_interface(struct usb_interface *intf)
>  { return 0; }
>  
> +static inline int usb_autopm_get_interface_async(struct usb_interface *intf)
> +{ return 0; }
> +
>  static inline void usb_autopm_put_interface(struct usb_interface *intf)
>  { }
> +static inline void usb_autopm_put_interface_async(struct usb_interface *intf)
> +{ }
>  static inline void usb_autopm_enable(struct usb_interface *intf)
>  { }
>  static inline void usb_autopm_disable(struct usb_interface *intf)
> Index: usb-2.6/drivers/usb/core/usb.h
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/usb.h
> +++ usb-2.6/drivers/usb/core/usb.h
> @@ -45,6 +45,7 @@ extern int usb_suspend(struct device *de
>  extern int usb_resume(struct device *dev);
>  
>  extern void usb_autosuspend_work(struct work_struct *work);
> +extern void usb_autoresume_work(struct work_struct *work);
>  extern int usb_port_suspend(struct usb_device *dev);
>  extern int usb_port_resume(struct usb_device *dev);
>  extern int usb_external_suspend_device(struct usb_device *udev,
> Index: usb-2.6/drivers/usb/core/usb.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/usb.c
> +++ usb-2.6/drivers/usb/core/usb.c
> @@ -402,6 +402,7 @@ struct usb_device *usb_alloc_dev(struct 
>  #ifdef	CONFIG_PM
>  	mutex_init(&dev->pm_mutex);
>  	INIT_DELAYED_WORK(&dev->autosuspend, usb_autosuspend_work);
> +	INIT_WORK(&dev->autoresume, usb_autoresume_work);
>  	dev->autosuspend_delay = usb_autosuspend_delay * HZ;
>  	dev->connect_time = jiffies;
>  	dev->active_duration = -jiffies;
> Index: usb-2.6/drivers/usb/core/hub.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hub.c
> +++ usb-2.6/drivers/usb/core/hub.c
> @@ -1362,8 +1362,9 @@ static void usb_stop_pm(struct usb_devic
>  		usb_autosuspend_device(udev->parent);
>  	usb_pm_unlock(udev);
>  
> -	/* Stop any autosuspend requests already submitted */
> -	cancel_rearming_delayed_work(&udev->autosuspend);
> +	/* Stop any autosuspend or autoresume requests already submitted */
> +	cancel_delayed_work_sync(&udev->autosuspend);
> +	cancel_work_sync(&udev->autoresume);
>  }
>  
>  #else
> Index: usb-2.6/drivers/usb/core/driver.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/driver.c
> +++ usb-2.6/drivers/usb/core/driver.c
> @@ -1339,6 +1339,19 @@ void usb_autosuspend_work(struct work_st
>  	usb_autopm_do_device(udev, 0);
>  }
>  
> +/* usb_autoresume_work - callback routine to autoresume a USB device */
> +void usb_autoresume_work(struct work_struct *work)
> +{
> +	struct usb_device *udev =
> +		container_of(work, struct usb_device, autoresume);
> +
> +	/* Wake it up, let the drivers do their thing, and then put it
> +	 * back to sleep.
> +	 */
> +	if (usb_autopm_do_device(udev, 1) == 0)
> +		usb_autopm_do_device(udev, -1);
> +}
> +
>  /**
>   * usb_autosuspend_device - delayed autosuspend of a USB device and its interfaces
>   * @udev: the usb_device to autosuspend
> @@ -1490,6 +1503,45 @@ void usb_autopm_put_interface(struct usb
>  EXPORT_SYMBOL_GPL(usb_autopm_put_interface);
>  
>  /**
> + * usb_autopm_put_interface_async - decrement a USB interface's PM-usage counter
> + * @intf: the usb_interface whose counter should be decremented
> + *
> + * This routine does essentially the same thing as
> + * usb_autopm_put_interface(): it decrements @intf's usage counter and
> + * queues a delayed autosuspend request if the counter is <= 0.  The
> + * difference is that it does not acquire the device's pm_mutex;
> + * callers must handle all synchronization issues themselves.
> + *
> + * Typically a driver would call this routine during an URB's completion
> + * handler, if no more URBs were pending.
> + *
> + * This routine can run in atomic context.
> + */
> +void usb_autopm_put_interface_async(struct usb_interface *intf)
> +{
> +	struct usb_device	*udev = interface_to_usbdev(intf);
> +	int			status = 0;
> +
> +	if (intf->condition == USB_INTERFACE_UNBOUND) {
> +		status = -ENODEV;
> +	} else {
> +		udev->last_busy = jiffies;
> +		--intf->pm_usage_cnt;
> +		if (udev->autosuspend_disabled || udev->autosuspend_delay < 0)
> +			status = -EPERM;
> +		else if (intf->pm_usage_cnt <= 0 &&
> +				!timer_pending(&udev->autosuspend.timer)) {
> +			queue_delayed_work(ksuspend_usb_wq, &udev->autosuspend,
> +					round_jiffies_relative(
> +						udev->autosuspend_delay));
> +		}
> +	}
> +	dev_vdbg(&intf->dev, "%s: status %d cnt %d\n",
> +			__func__, status, intf->pm_usage_cnt);
> +}
> +EXPORT_SYMBOL_GPL(usb_autopm_put_interface_async);
> +
> +/**
>   * usb_autopm_get_interface - increment a USB interface's PM-usage counter
>   * @intf: the usb_interface whose counter should be incremented
>   *
> @@ -1535,6 +1587,37 @@ int usb_autopm_get_interface(struct usb_
>  EXPORT_SYMBOL_GPL(usb_autopm_get_interface);
>  
>  /**
> + * usb_autopm_get_interface_async - increment a USB interface's PM-usage counter
> + * @intf: the usb_interface whose counter should be incremented
> + *
> + * This routine does much the same thing as
> + * usb_autopm_get_interface(): it increments @intf's usage counter and
> + * queues an autoresume request if the result is > 0.  The differences
> + * are that it does not acquire the device's pm_mutex (callers must
> + * handle all synchronization issues themselves), and it does not
> + * autoresume the device directly (it only queues a request).  After a
> + * successful call, the device will generally not yet be resumed.
> + *
> + * This routine can run in atomic context.
> + */
> +int usb_autopm_get_interface_async(struct usb_interface *intf)
> +{
> +	struct usb_device	*udev = interface_to_usbdev(intf);
> +	int			status = 0;
> +
> +	if (intf->condition == USB_INTERFACE_UNBOUND)
> +		status = -ENODEV;
> +	else if (udev->autoresume_disabled)
> +		status = -EPERM;
> +	else if (++intf->pm_usage_cnt > 0 && udev->state == USB_STATE_SUSPENDED)
> +		queue_work(ksuspend_usb_wq, &udev->autoresume);
> +	dev_vdbg(&intf->dev, "%s: status %d cnt %d\n",
> +			__func__, status, intf->pm_usage_cnt);
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(usb_autopm_get_interface_async);
> +
> +/**
>   * usb_autopm_set_interface - set a USB interface's autosuspend state
>   * @intf: the usb_interface whose state should be set
>   *
>
>   


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Alan Stern]

On Fri, 24 Oct 2008, Per Hallsmark wrote:

> Alan Stern wrote:
> > On Thu, 23 Oct 2008, Per Hallsmark wrote:
> >
> >   
> >> Enable more aggressive autosuspend in usbnet.
> >> Some commenting and cleanups done.
> >> Signed-off-by: Per Hallsmark <per@hallsmark.se>
> >>     
> >
> > I've been considering submitting the patch below.  It would help your 
> > work; you could remove all the "waker" stuff.
> >   

> So, if I understand the patch correctly, the waker's in cdc-acm, cdc-wdm 
> and usbnet
> could then be removed and a change to use *_async() should be done?

That's right.  There are a few races you have to worry about (maybe
they're already taken into account -- I'm not familiar with these
drivers):

	What happens if a new URB has to be submitted at about the
	same time as an autosuspend occurs?

	What happens if you call one of the *_async() routines at the
	same time as some other CPU updates intf->pm_usage_count?

> I work with a module that have usb interfaces driven by above three 
> drivers so it
> could be a good testing case I guess.
> Generic solutions are the best so if this approach would work then of 
> course a
> change should be made I think. I have no time to test it in near time 
> though,
> perhaps later next week.

Okay, let me know how it turns out.

Alan Stern

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Jeff Garzik]

Per Hallsmark wrote:
> Enable more aggressive autosuspend in usbnet.
> Some commenting and cleanups done.
> Signed-off-by: Per Hallsmark <per-46Ss7X5r2El6nIa31vAvqA@public.gmane.org>

Actually, this breaks the !CONFIG_PM build:

drivers/net/usb/usbnet.c: In function 'usbnet_suspend':
drivers/net/usb/usbnet.c:1357: error: 'struct usb_device' has no member 
named 'auto_pm'

I pondered taking the easy route of fixing this by surrounding the 
auto_pm reference with an ifdef, but it seems like usbnet could use a 
bit more thought -- it is questionable whether usbnet_suspend/resume 
should be built at all, if !CONFIG_PM, even though they are exported.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Oliver Neukum]

Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:
> I pondered taking the easy route of fixing this by surrounding the 
> auto_pm reference with an ifdef, but it seems like usbnet could use a 
> bit more thought -- it is questionable whether usbnet_suspend/resume 
> should be built at all, if !CONFIG_PM, even though they are exported.

As this is a generic problem, shouldn't we get the compiler to do
this for us?

	Regards
		Oliver

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Jeff Garzik]

Oliver Neukum wrote:
> Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:
>> I pondered taking the easy route of fixing this by surrounding the 
>> auto_pm reference with an ifdef, but it seems like usbnet could use a 
>> bit more thought -- it is questionable whether usbnet_suspend/resume 
>> should be built at all, if !CONFIG_PM, even though they are exported.
> 
> As this is a generic problem, shouldn't we get the compiler to do
> this for us?

Can you be more specific?

	Jeff

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Oliver Neukum]

Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:
> Oliver Neukum wrote:
> > Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:
> >> I pondered taking the easy route of fixing this by surrounding the 
> >> auto_pm reference with an ifdef, but it seems like usbnet could use a 
> >> bit more thought -- it is questionable whether usbnet_suspend/resume 
> >> should be built at all, if !CONFIG_PM, even though they are exported.
> > 
> > As this is a generic problem, shouldn't we get the compiler to do
> > this for us?
> 
> Can you be more specific?

As these methods are static the compiler is able to tell whether they
are referenced by anything but the tables. We should be able to set
an attribute in the header file that tells the compiler that these methods
won't be called and can be omitted in the build. Otherwise we have to ifdef
all those methods.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[David Miller]

From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Date: Fri, 7 Nov 2008 13:24:17 +0100

> Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:
> > Oliver Neukum wrote:
> > > Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:
> > >> I pondered taking the easy route of fixing this by surrounding the 
> > >> auto_pm reference with an ifdef, but it seems like usbnet could use a 
> > >> bit more thought -- it is questionable whether usbnet_suspend/resume 
> > >> should be built at all, if !CONFIG_PM, even though they are exported.
> > > 
> > > As this is a generic problem, shouldn't we get the compiler to do
> > > this for us?
> > 
> > Can you be more specific?
> 
> As these methods are static the compiler is able to tell whether they
> are referenced by anything but the tables. We should be able to set
> an attribute in the header file that tells the compiler that these methods
> won't be called and can be omitted in the build. Otherwise we have to ifdef
> all those methods.

The problem is that the content of these functions still needs to be
parsed, so references to ifdef'd out structure members are still going
to throw errors.

For the time being please add the necessary CONFIG_PM wrappers around
the suspend and resume methods, as this is what we do tree wide and
I don't think you want these usbnet changes blocked by some fancy
compiler facility that hasn't been implemented yet.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Per Hallsmark]

David Miller wrote:
> From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
> Date: Fri, 7 Nov 2008 13:24:17 +0100
>
>   
>> Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:
>>     
>>> Oliver Neukum wrote:
>>>       
>>>> Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:
>>>>         
>>>>> I pondered taking the easy route of fixing this by surrounding the 
>>>>> auto_pm reference with an ifdef, but it seems like usbnet could use a 
>>>>> bit more thought -- it is questionable whether usbnet_suspend/resume 
>>>>> should be built at all, if !CONFIG_PM, even though they are exported.
>>>>>           
>>>> As this is a generic problem, shouldn't we get the compiler to do
>>>> this for us?
>>>>         
>>> Can you be more specific?
>>>       
>> As these methods are static the compiler is able to tell whether they
>> are referenced by anything but the tables. We should be able to set
>> an attribute in the header file that tells the compiler that these methods
>> won't be called and can be omitted in the build. Otherwise we have to ifdef
>> all those methods.
>>     
>
> The problem is that the content of these functions still needs to be
> parsed, so references to ifdef'd out structure members are still going
> to throw errors.
>
> For the time being please add the necessary CONFIG_PM wrappers around
> the suspend and resume methods, as this is what we do tree wide and
> I don't think you want these usbnet changes blocked by some fancy
> compiler facility that hasn't been implemented yet.
>   
Ok, this news I've missed. Should I regenerate the patch or is it 
handled anyway?

Best regards,
Per

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbnet: enable more aggressive autosuspend

[Jeff Garzik]

Per Hallsmark wrote:
> David Miller wrote:
>> From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
>> Date: Fri, 7 Nov 2008 13:24:17 +0100
>>
>>  
>>> Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:
>>>    
>>>> Oliver Neukum wrote:
>>>>      
>>>>> Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:
>>>>>        
>>>>>> I pondered taking the easy route of fixing this by surrounding the 
>>>>>> auto_pm reference with an ifdef, but it seems like usbnet could 
>>>>>> use a bit more thought -- it is questionable whether 
>>>>>> usbnet_suspend/resume should be built at all, if !CONFIG_PM, even 
>>>>>> though they are exported.
>>>>>>           
>>>>> As this is a generic problem, shouldn't we get the compiler to do
>>>>> this for us?
>>>>>         
>>>> Can you be more specific?
>>>>       
>>> As these methods are static the compiler is able to tell whether they
>>> are referenced by anything but the tables. We should be able to set
>>> an attribute in the header file that tells the compiler that these 
>>> methods
>>> won't be called and can be omitted in the build. Otherwise we have to 
>>> ifdef
>>> all those methods.
>>>     
>>
>> The problem is that the content of these functions still needs to be
>> parsed, so references to ifdef'd out structure members are still going
>> to throw errors.
>>
>> For the time being please add the necessary CONFIG_PM wrappers around
>> the suspend and resume methods, as this is what we do tree wide and
>> I don't think you want these usbnet changes blocked by some fancy
>> compiler facility that hasn't been implemented yet.
>>   
> Ok, this news I've missed. Should I regenerate the patch or is it 
> handled anyway?

regenerate, please


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html