usbnet: enable more aggressive autosuspend

Enable more aggressive autosuspend in usbnet.
Some commenting and cleanups done.

Signed-off-by Per Hallsmark <[email protected]>

Quoted 4 lines · Per Hallsmark

Enable more aggressive autosuspend in usbnet.
Some commenting and cleanups done.

Signed-off-by Per Hallsmark <[email protected]>

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
===================================================================

Documentation/usb/power-management.txt +17 -5

@@ -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
===================================================================

include/linux/usb.h +9 -0

@@ -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
===================================================================

drivers/usb/core/usb.h +1 -0

@@ -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
===================================================================

drivers/usb/core/usb.c +1 -0

@@ -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
===================================================================

drivers/usb/core/hub.c +3 -2

@@ -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
===================================================================

drivers/usb/core/driver.c +76 -0

@@ -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);

Quoted 8 lines · Alan Stern

Quoted 4 lines · Per Hallsmark

Enable more aggressive autosuspend in usbnet.
Some commenting and cleanups done.

Signed-off-by Per Hallsmark <[email protected]>

Quoted 3 lines

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.

Enable more aggressive autosuspend in usbnet.
Some commenting and cleanups done.

Signed-off-by Per Hallsmark <[email protected]>

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.

Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:

Quoted 1 line

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?

Quoted 8 lines · Oliver Neukum

Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:

Quoted 4 lines

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?

Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:

Quoted 10 lines

Quoted 8 lines · Oliver Neukum

Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:

Quoted 4 lines

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?

Quoted 1 line

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.

From: Oliver Neukum <[email protected]>
Date: Fri, 7 Nov 2008 13:24:17 +0100

Quoted 11 lines

Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:

Quoted 9 lines

Quoted 7 lines · Oliver Neukum

Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:

Quoted 2 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 28 lines · David Miller

From: Oliver Neukum <[email protected]>
Date: Fri, 7 Nov 2008 13:24:17 +0100

Quoted 11 lines

Am Freitag, 7. November 2008 13:00:05 schrieb Jeff Garzik:

Quoted 9 lines

Quoted 7 lines · Oliver Neukum

Am Freitag, 7. November 2008 09:26:33 schrieb Jeff Garzik:

Quoted 2 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines

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?