[RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Ming Lei]

Hi,

Currently only very few usbnet devices support the traffic based
runtime PM, eg. wake up devices if there are packets to be transmitted.

For the below situation, it should make sense to runtime suspend usbnet
device to save power:
	
	- after network link becomes down

This patch implements the runtime PM triggered by network link change
event, and it works basically on asix usbnet device after a simple
runtime PM test.

 drivers/net/usb/asix_devices.c |    6 +-
 drivers/net/usb/cdc_ether.c    |    5 +-
 drivers/net/usb/cdc_ncm.c      |    9 +-
 drivers/net/usb/dm9601.c       |    7 +-
 drivers/net/usb/mcs7830.c      |    6 +-
 drivers/net/usb/sierra_net.c   |    6 +-
 drivers/net/usb/usbnet.c       |  224 +++++++++++++++++++++++++++++++++++++++-
 include/linux/usb/usbnet.h     |   21 +++-
 8 files changed, 250 insertions(+), 34 deletions(-)


Thanks
--
Ming Lei

[RFC PATCH 1/3] usbnet: introduce usbnet_link_change API

[Ming Lei]

This patch introduces the API of usbnet_link_change, so that
usbnet can trace the link change, which may help to implement
the later runtime PM triggered by usb ethernet link change.

Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
 drivers/net/usb/usbnet.c   |   11 +++++++++++
 include/linux/usb/usbnet.h |    2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e944109..95a96b1 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1591,6 +1591,17 @@ int usbnet_resume (struct usb_interface *intf)
 }
 EXPORT_SYMBOL_GPL(usbnet_resume);
 
+void usbnet_link_change(struct usbnet *dev, int link, int need_reset)
+{
+	if (link)
+		netif_carrier_on(dev->net);
+	else
+		netif_carrier_off(dev->net);
+
+	if (need_reset && link)
+		usbnet_defer_kevent(dev, EVENT_LINK_RESET);
+}
+EXPORT_SYMBOL_GPL(usbnet_link_change);
 
 /*-------------------------------------------------------------------------*/
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index f87cf62..1937b74 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -160,7 +160,7 @@ extern int usbnet_probe(struct usb_interface *, const struct usb_device_id *);
 extern int usbnet_suspend(struct usb_interface *, pm_message_t);
 extern int usbnet_resume(struct usb_interface *);
 extern void usbnet_disconnect(struct usb_interface *);

[RFC PATCH 2/3] usbnet: apply usbnet_link_change

[Ming Lei]

This patch applies the introduce usbnet_link_change API.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/asix_devices.c |    6 +-----
 drivers/net/usb/cdc_ether.c    |    5 +----
 drivers/net/usb/cdc_ncm.c      |    9 +++------
 drivers/net/usb/dm9601.c       |    7 +------
 drivers/net/usb/mcs7830.c      |    6 +-----
 drivers/net/usb/sierra_net.c   |    3 +--
 drivers/net/usb/usbnet.c       |    2 +-
 7 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 4fd48df..c354bb1 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -55,11 +55,7 @@ static void asix_status(struct usbnet *dev, struct urb *urb)
 	event = urb->transfer_buffer;
 	link = event->link & 0x01;
 	if (netif_carrier_ok(dev->net) != link) {
-		if (link) {
-			netif_carrier_on(dev->net);
-			usbnet_defer_kevent (dev, EVENT_LINK_RESET );
-		} else
-			netif_carrier_off(dev->net);
+		usbnet_link_change(dev, link, 1);
 		netdev_dbg(dev->net, "Link Status is: %d\n", link);
 	}
 }
diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index a03de71..c6e4be5 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -406,10 +406,7 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
 	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
 		netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
 			  event->wValue ? "on" : "off");
-		if (event->wValue)
-			netif_carrier_on(dev->net);
-		else
-			netif_carrier_off(dev->net);
+		usbnet_link_change(dev, event->wValue, 0);
 		break;
 	case USB_CDC_NOTIFY_SPEED_CHANGE:	/* tx/rx rates */
 		netif_dbg(dev, timer, dev->net, "CDC: speed change (len %d)\n",
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4cd582a..f425c2c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -593,7 +593,7 @@ advance:
 	 * (carrier is OFF) during attach, so the IP network stack does not
 	 * start IPv6 negotiation and more.
 	 */
-	netif_carrier_off(dev->net);
+	usbnet_link_change(dev, 0, 0);
 	ctx->tx_speed = ctx->rx_speed = 0;
 	return 0;
 
@@ -1131,12 +1131,9 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
 			" %sconnected\n",
 			ctx->netdev->name, ctx->connected ? "" : "dis");
 
-		if (ctx->connected)
-			netif_carrier_on(dev->net);
-		else {
-			netif_carrier_off(dev->net);
+		usbnet_link_change(dev, ctx->connected, 0);
+		if (!ctx->connected)
 			ctx->tx_speed = ctx->rx_speed = 0;
-		}
 		break;
 
 	case USB_CDC_NOTIFY_SPEED_CHANGE:
diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index e0433ce..7422d5a 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -587,12 +587,7 @@ static void dm9601_status(struct usbnet *dev, struct urb *urb)
 
 	link = !!(buf[0] & 0x40);
 	if (netif_carrier_ok(dev->net) != link) {
-		if (link) {
-			netif_carrier_on(dev->net);
-			usbnet_defer_kevent (dev, EVENT_LINK_RESET);
-		}
-		else
-			netif_carrier_off(dev->net);
+		usbnet_link_change(dev, link, 1);
 		netdev_dbg(dev->net, "Link Status is: %d\n", link);
 	}
 }
diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index 03c2d8d..49a98b7 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -639,11 +639,7 @@ static void mcs7830_status(struct usbnet *dev, struct urb *urb)
 
 	link = !(buf[1] & 0x20);
 	if (netif_carrier_ok(dev->net) != link) {
-		if (link) {
-			netif_carrier_on(dev->net);
-			usbnet_defer_kevent(dev, EVENT_LINK_RESET);
-		} else
-			netif_carrier_off(dev->net);
+		usbnet_link_change(dev, link, 1);
 		netdev_dbg(dev->net, "Link Status is: %d\n", link);
 	}
 }
diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 7ae70e9..08ed9e5 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -414,11 +414,10 @@ static void sierra_net_handle_lsi(struct usbnet *dev, char *data,
 	if (link_up) {
 		sierra_net_set_ctx_index(priv, hh->msgspecific.byte);
 		priv->link_up = 1;
-		netif_carrier_on(dev->net);
 	} else {
 		priv->link_up = 0;
-		netif_carrier_off(dev->net);
 	}
+	usbnet_link_change(dev, link_up, 0);
 }
 
 static void sierra_net_dosync(struct usbnet *dev)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 95a96b1..054ffd8 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1487,7 +1487,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	netif_device_attach (net);
 
 	if (dev->driver_info->flags & FLAG_LINK_INTR)
-		netif_carrier_off(net);
+		usbnet_link_change(dev, 0, 0);
 
 	return 0;
 
-- 
1.7.9.5

[RFC PATCH 3/3] usbnet: support runtime PM triggered by link change

[Ming Lei]

This patch implements runtime PM triggered by link change event
for devices which haven't defined manage_power() callback, based
on the below consideration:

- this kind of runtime PM has been supported by some PCI network
interfaces already, and it does make sense to suspend the usb
device to save power if no link is detected

- link down triggered runtime needn't to be implemented for devices
which have already supported traffic based runtime PM by .manage_power,
because runtime suspend can be triggered when no tx frames are to be
transmitted after link becoms down.

Unfortunately, some usbnet devices don't support remote wakeup,
or some devices may support it but the remote wakup can't be enabled
for link change event for some reason(no documents are public, not
supported ...).

This patch takes a periodic timer to wake up devices for detecting
the link change event if remote wakeup by link change can't be
supported. If the link is found to be down, put the device into
suspend immediately.

For the devices which support remote wakeup by link change and
don't support remote wakeup by incoming packets(not implement
manage_power callback), the patch can still make link change
triggered runtime PM working on these devices.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/sierra_net.c |    3 +-
 drivers/net/usb/usbnet.c     |  211 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/usb/usbnet.h   |   19 ++++
 3 files changed, 229 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 08ed9e5..0993f2d 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -418,6 +418,7 @@ static void sierra_net_handle_lsi(struct usbnet *dev, char *data,
 		priv->link_up = 0;
 	}
 	usbnet_link_change(dev, link_up, 0);
+	usbnet_link_updated(dev);
 }
 
 static void sierra_net_dosync(struct usbnet *dev)
@@ -915,7 +916,7 @@ static struct sk_buff *sierra_net_tx_fixup(struct usbnet *dev,
 
 static const struct driver_info sierra_net_info_direct_ip = {
 	.description = "Sierra Wireless USB-to-WWAN Modem",
-	.flags = FLAG_WWAN | FLAG_SEND_ZLP,
+	.flags = FLAG_WWAN | FLAG_SEND_ZLP | FLAG_LINK_UPDATE_BY_DRIVER,
 	.bind = sierra_net_bind,
 	.unbind = sierra_net_unbind,
 	.status = sierra_net_status,
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 054ffd8..8db1618 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -677,6 +677,186 @@ static void usbnet_terminate_urbs(struct usbnet *dev)
 	remove_wait_queue(&unlink_wakeup, &wait);
 }
 
+void usbnet_link_updated(struct usbnet *dev)
+{
+	complete(&dev->link_update_completion);
+}
+EXPORT_SYMBOL_GPL(usbnet_link_updated);
+
+#define usbnet_link_suspend(dev) do { \
+	dev_dbg(&dev->intf->dev, "%s:link suspend", __func__); \
+	usb_autopm_put_interface_async(dev->intf); \
+} while(0)
+
+#define usbnet_link_resume(dev) do { \
+	dev_dbg(&dev->intf->dev, "%s:link resume", __func__); \
+	usb_autopm_get_interface_async(dev->intf); \
+} while(0)
+
+static int need_link_runtime_pm(struct usbnet *dev)
+{
+	if (dev->driver_info->manage_power)
+		return 0;
+
+	if (!dev->driver_info->status)
+		return 0;
+
+	return 1;
+}
+
+/* called by usbnet_suspend */
+static void start_link_detect(struct usbnet *dev)
+{
+	if (!dev->link_rpm_enabled)
+		return;
+
+	if (dev->link_remote_wakeup)
+		return;
+
+	if (dev->link_check_started)
+		return;
+
+	dev->link_check_started = 1;
+	schedule_delayed_work(&dev->link_detect_work,
+			      msecs_to_jiffies(3000));
+}
+
+/* called by usbnet_resume */
+static void end_link_detect(struct usbnet *dev, int force_cancel)
+{
+	if (!dev->link_rpm_enabled)
+		return;
+
+	if (!dev->link_check_started)
+		return;
+
+	/*
+	 * cancel the link detect work if usbnet is resumed
+	 * not by link detect work
+	 */
+	if (!dev->link_checking || force_cancel)
+		cancel_delayed_work_sync(&dev->link_detect_work);
+
+	dev->link_check_started = 0;
+}
+
+/* called by usbnet_open */
+static void enable_link_runtime_pm(struct usbnet *dev)
+{
+	dev->link_rpm_enabled = 1;
+
+	if (!dev->link_remote_wakeup) {
+		dev->old_autosuspend_delay =
+			dev->udev->dev.power.autosuspend_delay;
+		pm_runtime_set_autosuspend_delay(&dev->udev->dev, 1);
+	}
+
+	if (!netif_carrier_ok(dev->net)) {
+		dev->link_open_suspend = 1;
+		usbnet_link_suspend(dev);
+	}
+}
+
+/* called by usbnet_stop */
+static void disable_link_runtime_pm(struct usbnet *dev)
+{
+	if (!dev->link_rpm_enabled)
+		return;
+	dev->link_rpm_enabled = 0;
+	end_link_detect(dev, 1);
+	if (dev->link_open_suspend) {
+		usbnet_link_resume(dev);
+		dev->link_open_suspend = 0;
+	}
+	if (!dev->link_remote_wakeup)
+		pm_runtime_set_autosuspend_delay(&dev->udev->dev,
+						 dev->old_autosuspend_delay);
+}
+
+static void update_link_state(struct usbnet *dev)
+{
+	char		*buf = NULL;
+	unsigned	pipe = 0;
+	unsigned	maxp;
+	int		ret, act_len, timeout;
+	struct urb	urb;
+
+	pipe = usb_rcvintpipe(dev->udev,
+			      dev->status->desc.bEndpointAddress
+				& USB_ENDPOINT_NUMBER_MASK);
+	maxp = usb_maxpacket(dev->udev, pipe, 0);
+
+	/*
+	 * Take default timeout as 2 times of period.
+	 * It is observed that asix device can update its link
+	 * state duing one period(128ms). Low level driver can set
+	 * its default update link time in bind() callback.
+	 */
+	if (!dev->link_update_timeout) {
+		timeout = max((int) dev->status->desc.bInterval,
+			(dev->udev->speed == USB_SPEED_HIGH) ? 7 : 3);
+		timeout = 1 << timeout;
+		if (dev->udev->speed == USB_SPEED_HIGH)
+			timeout /= 8;
+		if (timeout < 128)
+			timeout = 128;
+	} else
+		timeout = dev->link_update_timeout;
+
+	buf = kmalloc(maxp, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	dev_dbg(&dev->intf->dev, "%s: timeout %dms\n", __func__, timeout);
+	ret = usb_interrupt_msg(dev->udev, pipe, buf, maxp,
+			&act_len, timeout);
+	if (!ret) {
+		urb.status = 0;
+		urb.actual_length = act_len;
+		urb.transfer_buffer = buf;
+		urb.transfer_buffer_length = maxp;
+		dev->driver_info->status(dev, &urb);
+		if (dev->driver_info->flags &
+		    FLAG_LINK_UPDATE_BY_DRIVER)
+			wait_for_completion(&dev->link_update_completion);
+		dev_dbg(&dev->intf->dev, "%s: link updated\n", __func__);
+	} else
+		dev_dbg(&dev->intf->dev, "%s: link update failed %d\n",
+				__func__, ret);
+	kfree(buf);
+}
+
+static void link_detect_work(struct work_struct *work)
+{
+	struct usbnet	*dev = container_of(work, struct usbnet,
+					    link_detect_work.work);
+
+	dev_dbg(&dev->intf->dev, "%s: link resume\n", __func__);
+
+	dev->link_checking = 1;
+	usb_autopm_get_interface(dev->intf);
+	update_link_state(dev);
+	dev->link_checking = 0;
+
+	dev_dbg(&dev->intf->dev, "%s: link state %d\n",
+		__func__, netif_carrier_ok(dev->net));
+
+	if (!netif_carrier_ok(dev->net))
+		usb_autopm_put_interface(dev->intf);
+	else
+		usb_submit_urb(dev->interrupt, GFP_NOIO);
+}
+
+static void init_link_rpm(struct usbnet *dev)
+{
+	INIT_DELAYED_WORK(&dev->link_detect_work, link_detect_work);
+	init_completion(&dev->link_update_completion);
+
+	dev->link_remote_wakeup = !!(dev->driver_info->flags &
+				  FLAG_LINK_SUPPORT_REMOTE_WAKEUP);
+	dev->link_state = 1;
+}
+
 int usbnet_stop (struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
@@ -719,8 +899,10 @@ int usbnet_stop (struct net_device *net)
 	tasklet_kill (&dev->bh);
 	if (info->manage_power)
 		info->manage_power(dev, 0);
-	else
+	else {
+		disable_link_runtime_pm(dev);
 		usb_autopm_put_interface(dev->intf);
+	}
 
 	return 0;
 }
@@ -795,6 +977,9 @@ int usbnet_open (struct net_device *net)
 		if (retval < 0)
 			goto done_manage_power_error;
 		usb_autopm_put_interface(dev->intf);
+	} else {
+		if (need_link_runtime_pm(dev))
+			enable_link_runtime_pm(dev);
 	}
 	return retval;
 
@@ -1489,6 +1674,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	if (dev->driver_info->flags & FLAG_LINK_INTR)
 		usbnet_link_change(dev, 0, 0);
 
+	init_link_rpm(dev);
+
 	return 0;
 
 out4:
@@ -1538,6 +1725,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
 		 * wake the device
 		 */
 		netif_device_attach (dev->net);
+
+		if (PMSG_IS_AUTO(message))
+			start_link_detect(dev);
 	}
 	return 0;
 }
@@ -1552,8 +1742,10 @@ int usbnet_resume (struct usb_interface *intf)
 
 	if (!--dev->suspend_count) {
 		/* resume interrupt URBs */
-		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
-			usb_submit_urb(dev->interrupt, GFP_NOIO);
+		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags)) {
+			if (!dev->link_checking)
+				usb_submit_urb(dev->interrupt, GFP_NOIO);
+		}
 
 		spin_lock_irq(&dev->txq.lock);
 		while ((res = usb_get_from_anchor(&dev->deferred))) {
@@ -1586,6 +1778,8 @@ int usbnet_resume (struct usb_interface *intf)
 				netif_tx_wake_all_queues(dev->net);
 			tasklet_schedule (&dev->bh);
 		}
+
+		end_link_detect(dev, 0);
 	}
 	return 0;
 }
@@ -1593,6 +1787,9 @@ EXPORT_SYMBOL_GPL(usbnet_resume);
 
 void usbnet_link_change(struct usbnet *dev, int link, int need_reset)
 {
+	dev_dbg(&dev->intf->dev, "%s: old_link=%d link=%d\n", __func__,
+		dev->link_state, link);
+
 	if (link)
 		netif_carrier_on(dev->net);
 	else
@@ -1600,6 +1797,14 @@ void usbnet_link_change(struct usbnet *dev, int link, int need_reset)
 
 	if (need_reset && link)
 		usbnet_defer_kevent(dev, EVENT_LINK_RESET);
+
+	if (dev->link_rpm_enabled) {
+		if (!link && dev->link_state)
+			usbnet_link_suspend(dev);
+		else if (link && !dev->link_state && dev->link_remote_wakeup)
+			usbnet_link_resume(dev);
+	}
+	dev->link_state = link;
 }
 EXPORT_SYMBOL_GPL(usbnet_link_change);
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 1937b74..c96a623 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -68,6 +68,18 @@ struct usbnet {
 #		define EVENT_RX_PAUSED	5
 #		define EVENT_DEV_ASLEEP 6
 #		define EVENT_DEV_OPEN	7
+
+	/* link down triggered runtime PM */
+	struct delayed_work	link_detect_work;
+	struct completion	link_update_completion;
+	int			link_update_timeout;
+	int			old_autosuspend_delay;
+	unsigned int		link_rpm_enabled:1;
+	unsigned int		link_check_started:1;
+	unsigned int		link_checking:1;
+	unsigned int		link_open_suspend:1;
+	unsigned int		link_state:1;
+	unsigned int		link_remote_wakeup:1;
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
@@ -106,6 +118,12 @@ struct driver_info {
 #define FLAG_MULTI_PACKET	0x2000
 #define FLAG_RX_ASSEMBLE	0x4000	/* rx packets may span >1 frames */
 
+/* some drivers may not update link state in .status */
+#define FLAG_LINK_UPDATE_BY_DRIVER	0x8000
+
+/* device support remote wakeup by link change */
+#define FLAG_LINK_SUPPORT_REMOTE_WAKEUP	0x10000
+
 	/* init device ... can sleep, or cause probe() failure */
 	int	(*bind)(struct usbnet *, struct usb_interface *);
 
@@ -161,6 +179,7 @@ extern int usbnet_suspend(struct usb_interface *, pm_message_t);
 extern int usbnet_resume(struct usb_interface *);
 extern void usbnet_disconnect(struct usb_interface *);
 extern void usbnet_link_change(struct usbnet *dev, int link, int need_reset);
+extern void usbnet_link_updated(struct usbnet *dev);
 
 /* Drivers that reuse some of the standard USB CDC infrastructure
  * (notably, using multiple interfaces according to the CDC
-- 
1.7.9.5

Re: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Oliver Neukum]

On Sunday 16 September 2012 01:48:16 Ming Lei wrote:

Hi,

> Currently only very few usbnet devices support the traffic based
> runtime PM, eg. wake up devices if there are packets to be transmitted.
> 
> For the below situation, it should make sense to runtime suspend usbnet
> device to save power:
>         
>         - after network link becomes down

Basically cool design, but it raises two fundamental questions
and some detail questions.

> This patch implements the runtime PM triggered by network link change
> event, and it works basically on asix usbnet device after a simple
> runtime PM test.

1) Does it actually save power? You are constantly waking up a CPU.
>From that perspective it is possible that leaving on the ethernet is actually
better in terms of power. Only measurements can answer that question.

2) Do we have many devices that would be serviced with this approach?

	Regards
		Oliver

Re: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Ming Lei]

On Mon, Sep 17, 2012 at 4:04 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> On Sunday 16 September 2012 01:48:16 Ming Lei wrote:
>
> Hi,
>
>> Currently only very few usbnet devices support the traffic based
>> runtime PM, eg. wake up devices if there are packets to be transmitted.
>>
>> For the below situation, it should make sense to runtime suspend usbnet
>> device to save power:
>>
>>         - after network link becomes down
>
> Basically cool design, but it raises two fundamental questions
> and some detail questions.
>
>> This patch implements the runtime PM triggered by network link change
>> event, and it works basically on asix usbnet device after a simple
>> runtime PM test.
>
> 1) Does it actually save power? You are constantly waking up a CPU.

Of course, it does. I don't know it will save how much power just on
usbnet device, but it may save power from usb hub and usb host
controller in the bus at least.

Anyway we don't need to waste power if the link of usbnet is down.

It just wake up CPU one time each 3sec. In modern linux distribution,
the CPU will be wakup tens times per second, so it shouldn't be a
big problem.

> From that perspective it is possible that leaving on the ethernet is actually
> better in terms of power. Only measurements can answer that question.

You know it is a bit difficult to test power save for this situation. And
most of runtime PM patches didn't provide power save data. In fact,
I'd like to do it, but I have not some equipment to measure it, :-(.

>
> 2) Do we have many devices that would be serviced with this approach?

At least I found asix can be serviced by this approach. Considered asix
is quite popular, it is worthy of the effort. Also the below devices can be
serviced by the patch in theory:

                   dm9601.c / mcs7830.c / sierra_net.c

In fact, it might be used by other non-usbnet devices too.

Thanks,
--
Ming Lei
--
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: [RFC PATCH 3/3] usbnet: support runtime PM triggered by link change

[Oliver Neukum]

On Sunday 16 September 2012 01:48:19 Ming Lei wrote:

 
> +void usbnet_link_updated(struct usbnet *dev)
> +{
> +	complete(&dev->link_update_completion);
> +}
> +EXPORT_SYMBOL_GPL(usbnet_link_updated);

Isn't that a bit too trivial to get the _GPL version?

> +#define usbnet_link_suspend(dev) do { \
> +	dev_dbg(&dev->intf->dev, "%s:link suspend", __func__); \
> +	usb_autopm_put_interface_async(dev->intf); \
> +} while(0)
> +
> +#define usbnet_link_resume(dev) do { \
> +	dev_dbg(&dev->intf->dev, "%s:link resume", __func__); \
> +	usb_autopm_get_interface_async(dev->intf); \
> +} while(0)

Why macros?


[..]
> +/* called by usbnet_open */
> +static void enable_link_runtime_pm(struct usbnet *dev)
> +{
> +	dev->link_rpm_enabled = 1;
> +
> +	if (!dev->link_remote_wakeup) {
> +		dev->old_autosuspend_delay =
> +			dev->udev->dev.power.autosuspend_delay;
> +		pm_runtime_set_autosuspend_delay(&dev->udev->dev, 1);

This is a problem. Suppose the user changes the autosuspend timeout.
You cannot assume that the old value remains valid.

> +	}
> +
> +	if (!netif_carrier_ok(dev->net)) {
> +		dev->link_open_suspend = 1;
> +		usbnet_link_suspend(dev);
> +	}
> +}

> +static void update_link_state(struct usbnet *dev)
> +{
> +	char		*buf = NULL;
> +	unsigned	pipe = 0;
> +	unsigned	maxp;
> +	int		ret, act_len, timeout;
> +	struct urb	urb;
> +
> +	pipe = usb_rcvintpipe(dev->udev,
> +			      dev->status->desc.bEndpointAddress
> +				& USB_ENDPOINT_NUMBER_MASK);
> +	maxp = usb_maxpacket(dev->udev, pipe, 0);
> +
> +	/*
> +	 * Take default timeout as 2 times of period.
> +	 * It is observed that asix device can update its link
> +	 * state duing one period(128ms). Low level driver can set
> +	 * its default update link time in bind() callback.
> +	 */
> +	if (!dev->link_update_timeout) {
> +		timeout = max((int) dev->status->desc.bInterval,
> +			(dev->udev->speed == USB_SPEED_HIGH) ? 7 : 3);
> +		timeout = 1 << timeout;
> +		if (dev->udev->speed == USB_SPEED_HIGH)
> +			timeout /= 8;
> +		if (timeout < 128)
> +			timeout = 128;
> +	} else
> +		timeout = dev->link_update_timeout;
> +
> +	buf = kmalloc(maxp, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	dev_dbg(&dev->intf->dev, "%s: timeout %dms\n", __func__, timeout);
> +	ret = usb_interrupt_msg(dev->udev, pipe, buf, maxp,
> +			&act_len, timeout);
> +	if (!ret) {
> +		urb.status = 0;
> +		urb.actual_length = act_len;
> +		urb.transfer_buffer = buf;
> +		urb.transfer_buffer_length = maxp;
> +		dev->driver_info->status(dev, &urb);
> +		if (dev->driver_info->flags &
> +		    FLAG_LINK_UPDATE_BY_DRIVER)
> +			wait_for_completion(&dev->link_update_completion);

If a driver calls usbnet_link_updated() from the same workqueue this
will deadlock.

> +		dev_dbg(&dev->intf->dev, "%s: link updated\n", __func__);
> +	} else
> +		dev_dbg(&dev->intf->dev, "%s: link update failed %d\n",
> +				__func__, ret);
> +	kfree(buf);
> +}

[..]
> @@ -795,6 +977,9 @@ int usbnet_open (struct net_device *net)
>  		if (retval < 0)
>  			goto done_manage_power_error;
>  		usb_autopm_put_interface(dev->intf);
> +	} else {
> +		if (need_link_runtime_pm(dev))
> +			enable_link_runtime_pm(dev);
>  	}
>  	return retval;
>  
> @@ -1489,6 +1674,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>  	if (dev->driver_info->flags & FLAG_LINK_INTR)
>  		usbnet_link_change(dev, 0, 0);
>  
> +	init_link_rpm(dev);
> +
>  	return 0;
>  
>  out4:
> @@ -1538,6 +1725,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>  		 * wake the device
>  		 */
>  		netif_device_attach (dev->net);
> +
> +		if (PMSG_IS_AUTO(message))
> +			start_link_detect(dev);

What happens if the device is autosuspended, then the system is suspended
and the work is executed while the suspension is underway?

>  	}
>  	return 0;
>  }
> @@ -1552,8 +1742,10 @@ int usbnet_resume (struct usb_interface *intf)
>  
>  	if (!--dev->suspend_count) {
>  		/* resume interrupt URBs */
> -		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
> -			usb_submit_urb(dev->interrupt, GFP_NOIO);
> +		if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags)) {
> +			if (!dev->link_checking)

That is impossible. If the device is resumed the interrupt URB must
be restarted in every case (if it exists).
You cannot assume that its only function is checking the link state.
And it introduces a race with the workqueue.

> +				usb_submit_urb(dev->interrupt, GFP_NOIO);
> +		}
>  
>  		spin_lock_irq(&dev->txq.lock);
>  		while ((res = usb_get_from_anchor(&dev->deferred))) {
> @@ -1586,6 +1778,8 @@ int usbnet_resume (struct usb_interface *intf)
>  				netif_tx_wake_all_queues(dev->net);
>  			tasklet_schedule (&dev->bh);
>  		}
> +
> +		end_link_detect(dev, 0);
>  	}
>  	return 0;
>  }
> @@ -1593,6 +1787,9 @@ EXPORT_SYMBOL_GPL(usbnet_resume);

	Regards
		Oliver

RE: [RFC PATCH 3/3] usbnet: support runtime PM triggered by link change

[David Laight]

> > +void usbnet_link_updated(struct usbnet *dev)
> > +{
> > +	complete(&dev->link_update_completion);
> > +}
> > +EXPORT_SYMBOL_GPL(usbnet_link_updated);
> 
> Isn't that a bit too trivial to get the _GPL version?

Particularly if the usb infrastructure (that I presume this
is part of) might be reasonably usable by non-gpl drivers.

A few years back the function to release a reference on
the kernel 'pid' structure was added as GPL - making
it impossible for non-GPL to hold the reference.

	David
 


--
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: [RFC PATCH 3/3] usbnet: support runtime PM triggered by link change

[Greg Kroah-Hartman]

On Mon, Sep 17, 2012 at 10:02:27AM +0100, David Laight wrote:
> > > +void usbnet_link_updated(struct usbnet *dev)
> > > +{
> > > +	complete(&dev->link_update_completion);
> > > +}
> > > +EXPORT_SYMBOL_GPL(usbnet_link_updated);
> > 
> > Isn't that a bit too trivial to get the _GPL version?
> 
> Particularly if the usb infrastructure (that I presume this
> is part of) might be reasonably usable by non-gpl drivers.

It can not be, and has not been able to, for quite a number of years now
(since the 2.6.15 kernel release or so I think.)  So this should not be
an issue.

greg k-h
--
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: [RFC PATCH 3/3] usbnet: support runtime PM triggered by link change

[Ming Lei]

On Mon, Sep 17, 2012 at 4:50 PM, Oliver Neukum <oneukum@suse.de> wrote:
> On Sunday 16 September 2012 01:48:19 Ming Lei wrote:
>
>
>> +void usbnet_link_updated(struct usbnet *dev)
>> +{
>> +     complete(&dev->link_update_completion);
>> +}
>> +EXPORT_SYMBOL_GPL(usbnet_link_updated);
>
> Isn't that a bit too trivial to get the _GPL version?

OK, will use the non GPL version.

>
>> +#define usbnet_link_suspend(dev) do { \
>> +     dev_dbg(&dev->intf->dev, "%s:link suspend", __func__); \
>> +     usb_autopm_put_interface_async(dev->intf); \
>> +} while(0)
>> +
>> +#define usbnet_link_resume(dev) do { \
>> +     dev_dbg(&dev->intf->dev, "%s:link resume", __func__); \
>> +     usb_autopm_get_interface_async(dev->intf); \
>> +} while(0)
>
> Why macros?

Just for easy debug by dumping the caller function name.

>
>
> [..]
>> +/* called by usbnet_open */
>> +static void enable_link_runtime_pm(struct usbnet *dev)
>> +{
>> +     dev->link_rpm_enabled = 1;
>> +
>> +     if (!dev->link_remote_wakeup) {
>> +             dev->old_autosuspend_delay =
>> +                     dev->udev->dev.power.autosuspend_delay;
>> +             pm_runtime_set_autosuspend_delay(&dev->udev->dev, 1);
>
> This is a problem. Suppose the user changes the autosuspend timeout.
> You cannot assume that the old value remains valid.

Good catch, I will fix it in -v1 by reading again the timeout value in
disable_link_runtime_pm().

>> +     }
>> +
>> +     if (!netif_carrier_ok(dev->net)) {
>> +             dev->link_open_suspend = 1;
>> +             usbnet_link_suspend(dev);
>> +     }
>> +}
>
>> +static void update_link_state(struct usbnet *dev)
>> +{
>> +     char            *buf = NULL;
>> +     unsigned        pipe = 0;
>> +     unsigned        maxp;
>> +     int             ret, act_len, timeout;
>> +     struct urb      urb;
>> +
>> +     pipe = usb_rcvintpipe(dev->udev,
>> +                           dev->status->desc.bEndpointAddress
>> +                             & USB_ENDPOINT_NUMBER_MASK);
>> +     maxp = usb_maxpacket(dev->udev, pipe, 0);
>> +
>> +     /*
>> +      * Take default timeout as 2 times of period.
>> +      * It is observed that asix device can update its link
>> +      * state duing one period(128ms). Low level driver can set
>> +      * its default update link time in bind() callback.
>> +      */
>> +     if (!dev->link_update_timeout) {
>> +             timeout = max((int) dev->status->desc.bInterval,
>> +                     (dev->udev->speed == USB_SPEED_HIGH) ? 7 : 3);
>> +             timeout = 1 << timeout;
>> +             if (dev->udev->speed == USB_SPEED_HIGH)
>> +                     timeout /= 8;
>> +             if (timeout < 128)
>> +                     timeout = 128;
>> +     } else
>> +             timeout = dev->link_update_timeout;
>> +
>> +     buf = kmalloc(maxp, GFP_KERNEL);
>> +     if (!buf)
>> +             return;
>> +
>> +     dev_dbg(&dev->intf->dev, "%s: timeout %dms\n", __func__, timeout);
>> +     ret = usb_interrupt_msg(dev->udev, pipe, buf, maxp,
>> +                     &act_len, timeout);
>> +     if (!ret) {
>> +             urb.status = 0;
>> +             urb.actual_length = act_len;
>> +             urb.transfer_buffer = buf;
>> +             urb.transfer_buffer_length = maxp;
>> +             dev->driver_info->status(dev, &urb);
>> +             if (dev->driver_info->flags &
>> +                 FLAG_LINK_UPDATE_BY_DRIVER)
>> +                     wait_for_completion(&dev->link_update_completion);
>
> If a driver calls usbnet_link_updated() from the same workqueue this
> will deadlock.

Good catch, I will fix it in -v1 by using system_freezable_wq.

>
>> +             dev_dbg(&dev->intf->dev, "%s: link updated\n", __func__);
>> +     } else
>> +             dev_dbg(&dev->intf->dev, "%s: link update failed %d\n",
>> +                             __func__, ret);
>> +     kfree(buf);
>> +}
>
> [..]
>> @@ -795,6 +977,9 @@ int usbnet_open (struct net_device *net)
>>               if (retval < 0)
>>                       goto done_manage_power_error;
>>               usb_autopm_put_interface(dev->intf);
>> +     } else {
>> +             if (need_link_runtime_pm(dev))
>> +                     enable_link_runtime_pm(dev);
>>       }
>>       return retval;
>>
>> @@ -1489,6 +1674,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>>       if (dev->driver_info->flags & FLAG_LINK_INTR)
>>               usbnet_link_change(dev, 0, 0);
>>
>> +     init_link_rpm(dev);
>> +
>>       return 0;
>>
>>  out4:
>> @@ -1538,6 +1725,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>>                * wake the device
>>                */
>>               netif_device_attach (dev->net);
>> +
>> +             if (PMSG_IS_AUTO(message))
>> +                     start_link_detect(dev);
>
> What happens if the device is autosuspended, then the system is suspended
> and the work is executed while the suspension is underway?

IMO we can avoid the problem by scheduling 'link_detect_work' on the
workqueue of 'system_freezable_wq', which will be introduced in -v1.

>
>>       }
>>       return 0;
>>  }
>> @@ -1552,8 +1742,10 @@ int usbnet_resume (struct usb_interface *intf)
>>
>>       if (!--dev->suspend_count) {
>>               /* resume interrupt URBs */
>> -             if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
>> -                     usb_submit_urb(dev->interrupt, GFP_NOIO);
>> +             if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags)) {
>> +                     if (!dev->link_checking)
>
> That is impossible. If the device is resumed the interrupt URB must
> be restarted in every case (if it exists).

The interrupt URB will be submitted later in link_detect_work() under
the situation of being resumed by link detect work.

> You cannot assume that its only function is checking the link state.
> And it introduces a race with the workqueue.

Looks no race because usbnet_resume() will be run in workqueue
task context under the situation of being resumed by link detect work.


Thanks,
--
Ming Lei

Re: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Bjørn Mork]

Ming Lei <ming.lei@canonical.com> writes:
> On Mon, Sep 17, 2012 at 4:04 PM, Oliver Neukum <oneukum@suse.de> wrote:
>
>> 1) Does it actually save power? You are constantly waking up a CPU.
>
> Of course, it does. I don't know it will save how much power just on
> usbnet device, but it may save power from usb hub and usb host
> controller in the bus at least.
>
> Anyway we don't need to waste power if the link of usbnet is down.
>
> It just wake up CPU one time each 3sec. In modern linux distribution,
> the CPU will be wakup tens times per second, so it shouldn't be a
> big problem.

I do not buy that constantly polling a device necessarily saves any
power compared to keeping the USB link to the device alive.  You need to
measure the savings if you want us to believe that.

You are not only waking the host CPU.  You are also waking the device
CPU. 

Any usbnet device will consist of more than one building block, having
at least a network block, a USB block and a CPU.  For all you know, the
device CPU might be in deep sleep until it has to service either the
network or USB block, and those might also be sleeping until they see an
interrupt.  Constantly polling the device to receive network link status
might use considerably more power than keeping the USB link up waiting
for a link notification.

If you were to implement this feature anyway, then I would prefer a
userspace based approach instead.  I see no reason to keep the timer in
the kernel.  You could decide to suspend whenever the netdev is down,
and only poll the link when userspace tries to bring up the netdev.
That would let a userspace daemon implement the same feature by trying
to bring up the netdev every 3 seconds (or whatever frequency the user
selected).  And it would allow me to avoid the polling until I know I
have plugged in a cable.

>> From that perspective it is possible that leaving on the ethernet is actually
>> better in terms of power. Only measurements can answer that question.
>
> You know it is a bit difficult to test power save for this situation. And
> most of runtime PM patches didn't provide power save data. In fact,
> I'd like to do it, but I have not some equipment to measure it, :-(.

We don't know, you don't know.  But you claim that your change saves
power, so please provide some documentation showing that it does.

>> 2) Do we have many devices that would be serviced with this approach?
>
> At least I found asix can be serviced by this approach. Considered asix
> is quite popular, it is worthy of the effort. Also the below devices can be
> serviced by the patch in theory:
>
>                    dm9601.c / mcs7830.c / sierra_net.c

The sierra_net.c driver is only used for wireless devices AFAIK. I
really don't see the use case for network link based resume of that.
There is no cable to plug.  Userspace will have to initiate a
connection.

And the DirectIP device I've got (MC7710) supports remote wakeup.  I
assume that will be the case for all such devices, given that this is
mostly a firmware feature. So the correct fix for sierra_net.c is to add
support for remote wakeup.  Then you will be able to suspend the device
regardless of whether the link is up or down, without the constant
polling.


Bjørn

Re: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[David Miller]

There seems to be some discussion about the legitimacy of doing things
this way, and in any event the patches were an RFC.

Please resubmit as a non-RFC once all the issues have been worked
out, if appropriate.

Thanks.

Re: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Oliver Neukum]

On Thursday 20 September 2012 17:02:27 David Miller wrote:
> 
> There seems to be some discussion about the legitimacy of doing things
> this way, and in any event the patches were an RFC.
> 
> Please resubmit as a non-RFC once all the issues have been worked
> out, if appropriate.

Just to make this clear, I'd like to state that the discussion involved
only the third, last patch in the series. The first two are fine and make
sense by themselves.

	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: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[David Miller]

From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Date: Thu, 20 Sep 2012 23:04:38 +0200

> On Thursday 20 September 2012 17:02:27 David Miller wrote:
>> 
>> There seems to be some discussion about the legitimacy of doing things
>> this way, and in any event the patches were an RFC.
>> 
>> Please resubmit as a non-RFC once all the issues have been worked
>> out, if appropriate.
> 
> Just to make this clear, I'd like to state that the discussion involved
> only the third, last patch in the series. The first two are fine and make
> sense by themselves.

I want changes in those, see my replies.
--
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: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Ming Lei]

On Fri, Sep 21, 2012 at 5:16 AM, David Miller <davem@davemloft.net> wrote:
> From: Oliver Neukum <oliver@neukum.org>
> Date: Thu, 20 Sep 2012 23:04:38 +0200
>
>> On Thursday 20 September 2012 17:02:27 David Miller wrote:
>>>
>>> There seems to be some discussion about the legitimacy of doing things
>>> this way, and in any event the patches were an RFC.
>>>
>>> Please resubmit as a non-RFC once all the issues have been worked
>>> out, if appropriate.
>>
>> Just to make this clear, I'd like to state that the discussion involved
>> only the third, last patch in the series. The first two are fine and make
>> sense by themselves.
>
> I want changes in those, see my replies.

No problem, I will send out -v2 of the first two patches later.

thanks,
--
Ming Lei

Re: [RFC PATCH 0/3] usbnet: support runtime PM triggered by link change

[Ming Lei]

On Thu, Sep 20, 2012 at 4:30 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>> On Mon, Sep 17, 2012 at 4:04 PM, Oliver Neukum <oneukum@suse.de> wrote:
>>
>>> 1) Does it actually save power? You are constantly waking up a CPU.
>>
>> Of course, it does. I don't know it will save how much power just on
>> usbnet device, but it may save power from usb hub and usb host
>> controller in the bus at least.
>>
>> Anyway we don't need to waste power if the link of usbnet is down.
>>
>> It just wake up CPU one time each 3sec. In modern linux distribution,
>> the CPU will be wakup tens times per second, so it shouldn't be a
>> big problem.
>
> I do not buy that constantly polling a device necessarily saves any
> power compared to keeping the USB link to the device alive.  You need to
> measure the savings if you want us to believe that.

Yes, good suggestion.

>
> You are not only waking the host CPU.  You are also waking the device
> CPU.

Some usbnet devices doesn't have CPU, and I don't think there are
one in smsc95xx or asix, :-)

>
> Any usbnet device will consist of more than one building block, having
> at least a network block, a USB block and a CPU.  For all you know, the
> device CPU might be in deep sleep until it has to service either the
> network or USB block, and those might also be sleeping until they see an
> interrupt.  Constantly polling the device to receive network link status
> might use considerably more power than keeping the USB link up waiting
> for a link notification.
>
> If you were to implement this feature anyway, then I would prefer a
> userspace based approach instead.  I see no reason to keep the timer in
> the kernel.  You could decide to suspend whenever the netdev is down,
> and only poll the link when userspace tries to bring up the netdev.

I am wondering if userspace knows when it should bring up the interface,
maybe it depends on the link becomes up, maybe udev always bring it up.

> That would let a userspace daemon implement the same feature by trying
> to bring up the netdev every 3 seconds (or whatever frequency the user
> selected).  And it would allow me to avoid the polling until I know I
> have plugged in a cable.

I don't see any advantage by using a userspace timer, and it will convert to
a kernel timer finally.

Also putting the interface down may introduce some side effect on user
space: the IP address may be lost, etc.

>
>>> From that perspective it is possible that leaving on the ethernet is actually
>>> better in terms of power. Only measurements can answer that question.
>>
>> You know it is a bit difficult to test power save for this situation. And
>> most of runtime PM patches didn't provide power save data. In fact,
>> I'd like to do it, but I have not some equipment to measure it, :-(.
>
> We don't know, you don't know.  But you claim that your change saves
> power, so please provide some documentation showing that it does.

The motivation of the patch is that suspending the usbnet device may put
all usb devices in the bus into suspend.

I am trying to get some data by powertop, but looks it is not reliable enough.

I even found that the discharge rate of battery when the asix is disconnected
and let the whole bus suspend is even more than when the asix is connected
with ethernet cable link down and all devices in the whole usb bus are active.
There is about 1w difference, so odd.

I will try to get some correct power data.

>
>>> 2) Do we have many devices that would be serviced with this approach?
>>
>> At least I found asix can be serviced by this approach. Considered asix
>> is quite popular, it is worthy of the effort. Also the below devices can be
>> serviced by the patch in theory:
>>
>>                    dm9601.c / mcs7830.c / sierra_net.c
>
> The sierra_net.c driver is only used for wireless devices AFAIK. I
> really don't see the use case for network link based resume of that.
> There is no cable to plug.  Userspace will have to initiate a
> connection.

Wireless link is still one kind of link, :-)

Otherwise, why sierra_net.c bother to use netif_carrier_on?

>
> And the DirectIP device I've got (MC7710) supports remote wakeup.  I
> assume that will be the case for all such devices, given that this is
> mostly a firmware feature. So the correct fix for sierra_net.c is to add
> support for remote wakeup.  Then you will be able to suspend the device
> regardless of whether the link is up or down, without the constant
> polling.

In fact, the patch supports link change via remote wakeup, and it may be
useful if remote wakeup is not supported by incoming packet.

Not mention the power save data, the patch is not mature enough, for example,
ethtool_ops interface may access a suspended device.

I will update it if some progresses are got.

Thanks,
--
Ming Lei