[PATCH v5 0/3] usb: notify hcd when USB device suspend or resume

[PATCH v5 0/3] usb: notify hcd when USB device suspend or resume

[Lu Baolu]

This patch series try to meet a design requirement in xHCI spec.

The xHCI spec is designed to allow an xHC implementation to cache the
endpoint state. Caching endpoint state allows an xHC to reduce latency
when handling ERDYs and other USB asynchronous events. However holding
this state in xHC consumes resources and power. The xHCI spec designs
some methods through which host controller driver can hint xHC about
how to optimize its operation, e.g. to determine when it holds state
internally or pushes it out to memory, when to power down logic, etc.

When a USB device is going to suspend, states of all endpoints cached
in the xHC should be pushed out to memory to save power and resources.
Vice versa, when a USB device resumes, those states should be brought
back to the cache.

It is harmless if a USB devices under USB 3.0 host controller suspends
or resumes without a notification to hcd driver. However there may be
less opportunities for power savings and there may be increased latency
for restarting an endpoint. The precise impact will be different for
each xHC implementation. It all depends on what an implementation does
with the hints.

Change log:
v4->v5:
 - add Alan's ACK for 1/3

v3->v4:
 - remove unused 'msg' parameter in the callbacks

v2->v3:
 - move two xhci specific comments from hub to xhci
 - define xhci_device_suspend(resume) as NULL when no PM_CONFIG

v1->v2:
 - make the callback name specific to the activity in question
 - no need to export hcd_notify
 - put the callback in the right place

Lu Baolu (3):
  usb: notify hcd when USB device suspend or resume
  usb: xhci: implement device_suspend/device_resume entries
  usb: xhci: remove stop device and ring doorbell in hub control and bus
    suspend

 drivers/usb/core/hcd.c      | 27 +++++++++++++++++++++++++
 drivers/usb/core/hub.c      |  5 +++++
 drivers/usb/host/xhci-hub.c | 49 +--------------------------------------------
 drivers/usb/host/xhci.c     | 38 +++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  9 +++++++++
 include/linux/usb/hcd.h     |  8 +++++++-
 6 files changed, 87 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH v5 1/3] usb: notify hcd when USB device suspend or resume

[Lu Baolu]

This patch adds two new entries in hc_driver. With these new entries,
USB core can notify host driver when a USB device is about to suspend
or just resumed.

The xHCI spec is designed to allow an xHC implementation to cache the
endpoint state. Caching endpoint state allows an xHC to reduce latency
when handling ERDYs and other USB asynchronous events. However holding
this state in xHC consumes resources and power. The xHCI spec designs
some methods through which host controller driver can hint xHC about
how to optimize its operation, e.g. to determine when it holds state
internally or pushes it out to memory, when to power down logic, etc.

When a USB device is going to suspend, states of all endpoints cached
in the xHC should be pushed out to memory to save power and resources.
Vice versa, when a USB device resumes, those states should be brought
back to the cache. USB core suspends or resumes a USB device by sending
set/clear port feature requests to the parent hub where the USB device
is connected. Unfortunately, these operations are transparent to xHCI
driver unless the USB device is plugged in a root port. This patch
utilizes the callback entries to notify xHCI driver whenever a USB
device suspends or resumes.

It is harmless if a USB devices under USB 3.0 host controller suspends
or resumes without a notification to hcd driver. However there may be
less opportunities for power savings and there may be increased latency
for restarting an endpoint. The precise impact will be different for
each xHC implementation. It all depends on what an implementation does
with the hints.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/core/hcd.c  | 27 +++++++++++++++++++++++++++
 drivers/usb/core/hub.c  |  5 +++++
 include/linux/usb/hcd.h |  8 +++++++-
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..007450d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2289,6 +2289,33 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
 
+/**
+ * hcd_suspend_notify - notify hcd driver when a device is going to suspend
+ * @udev: USB device to be suspended
+ *
+ * Call back to hcd driver to notify that a USB device is going to suspend.
+ */
+void hcd_suspend_notify(struct usb_device *udev)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->device_suspend)
+		hcd->driver->device_suspend(hcd, udev);
+}
+
+/**
+ * hcd_resume_notify - notify hcd driver when a device is just resumed
+ * @udev: USB device just resumed
+ *
+ * Call back to hcd driver to notify that a USB device is just resumed.
+ */
+void hcd_resume_notify(struct usb_device *udev)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->device_resume)
+		hcd->driver->device_resume(hcd, udev);
+}
 #endif	/* CONFIG_PM */
 
 /*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 52178bc..28d76b7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3149,6 +3149,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 			goto err_lpm3;
 	}
 
+	hcd_suspend_notify(udev);
+
 	/* see 7.1.7.6 */
 	if (hub_is_superspeed(hub->hdev))
 		status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
@@ -3174,6 +3176,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 	if (status) {
 		dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
 
+		hcd_resume_notify(udev);
+
 		/* Try to enable USB3 LPM and LTM again */
 		usb_unlocked_enable_lpm(udev);
  err_lpm3:
@@ -3427,6 +3431,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	}
 
  SuspendCleared:
+	hcd_resume_notify(udev);
 	if (status == 0) {
 		udev->port_is_suspended = 0;
 		if (hub_is_superspeed(hub->hdev)) {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 68b1e83..bc2eb1c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -383,7 +383,11 @@ struct hc_driver {
 	int	(*find_raw_port_number)(struct usb_hcd *, int);
 	/* Call for power on/off the port if necessary */
 	int	(*port_power)(struct usb_hcd *hcd, int portnum, bool enable);
-
+	/* Call back to hcd when a USB device is going to suspend or just
+	 * resumed.
+	 */
+	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev);
+	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev);
 };
 
 static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
@@ -632,6 +636,8 @@ extern void usb_root_hub_lost_power(struct usb_device *rhdev);
 extern int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg);
 extern int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg);
 extern void usb_hcd_resume_root_hub(struct usb_hcd *hcd);
+extern void hcd_suspend_notify(struct usb_device *udev);
+extern void hcd_resume_notify(struct usb_device *udev);
 #else
 static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
 {
-- 
2.1.0

[PATCH v5 2/3] usb: xhci: implement device_suspend/device_resume entries

[Lu Baolu]

This patch implements device_suspend/device_resume entries for xHC driver.
device_suspend will be called when a USB device is about to suspend. It
will issue a stop endpoint command for each endpoint in this device. The
Suspend(SP) bit in the command TRB will set which will give xHC a hint
about the suspend. device_resume will be called when a USB device is just
resumed. It will ring doorbells of all endpoint unconditionally. XHC may
use these suspend/resume hints to optimize its operation.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  9 +++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0827d7c..a83e82e 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -266,7 +266,7 @@ int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
  * to complete.
  * suspend will set to 1, if suspend bit need to set in command.
  */
-static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
+int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 {
 	struct xhci_virt_device *virt_dev;
 	struct xhci_command *cmd;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ec8ac16..330961d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4680,6 +4680,38 @@ int xhci_disable_usb3_lpm_timeout(struct usb_hcd *hcd,
 		return ret;
 	return 0;
 }
+
+/*
+ * xHCI compatible host controller driver expects to be notified prior to
+ * selectively suspending a device. xHCI hcd could optimize the endpoint
+ * cache for power saving purpose. Refer to 4.15.1.1 of xHCI 1.1.
+ */
+void xhci_device_suspend(struct usb_hcd *hcd, struct usb_device *udev)
+{
+	struct xhci_hcd	*xhci;
+
+	xhci = hcd_to_xhci(hcd);
+	if (!xhci || !xhci->devs[udev->slot_id])
+		return;
+
+	xhci_stop_device(xhci, udev->slot_id, 1);
+}
+
+/*
+ * xHCI compatible host controller driver expects to be notified after a
+ * USB device is resumed. xHCI hcd could optimize the endpoint cache
+ * to reduce the latency. Refer to 4.15.1.1 of xHCI 1.1.
+ */
+void xhci_device_resume(struct usb_hcd *hcd, struct usb_device *udev)
+{
+	struct xhci_hcd	*xhci;
+
+	xhci = hcd_to_xhci(hcd);
+	if (!xhci || !xhci->devs[udev->slot_id])
+		return;
+
+	xhci_ring_device(xhci, udev->slot_id);
+}
 #else /* CONFIG_PM */
 
 int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
@@ -4976,6 +5008,12 @@ static const struct hc_driver xhci_hc_driver = {
 	.enable_usb3_lpm_timeout =	xhci_enable_usb3_lpm_timeout,
 	.disable_usb3_lpm_timeout =	xhci_disable_usb3_lpm_timeout,
 	.find_raw_port_number =	xhci_find_raw_port_number,
+
+	/*
+	 * call back when devices suspend or resume
+	 */
+	.device_suspend =	xhci_device_suspend,
+	.device_resume =	xhci_device_resume,
 };
 
 void xhci_init_driver(struct hc_driver *drv, int (*setup_fn)(struct usb_hcd *))
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8e421b8..67c13c5 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1867,10 +1867,19 @@ u32 xhci_port_state_to_neutral(u32 state);
 int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
 		u16 port);
 void xhci_ring_device(struct xhci_hcd *xhci, int slot_id);
+int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend);
 
 /* xHCI contexts */
 struct xhci_input_control_ctx *xhci_get_input_control_ctx(struct xhci_container_ctx *ctx);
 struct xhci_slot_ctx *xhci_get_slot_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx);
 struct xhci_ep_ctx *xhci_get_ep_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx, unsigned int ep_index);
 
+#ifdef	CONFIG_PM
+void xhci_device_suspend(struct usb_hcd *hcd, struct usb_device *udev);
+void xhci_device_resume(struct usb_hcd *hcd, struct usb_device *udev);
+#else
+#define xhci_device_suspend	NULL
+#define xhci_device_resume	NULL
+#endif /* CONFIG_PM */
+
 #endif /* __LINUX_XHCI_HCD_H */
-- 
2.1.0

[PATCH v5 3/3] usb: xhci: remove stop device and ring doorbell in hub control and bus suspend

[Lu Baolu]

There is no need to call xhci_stop_device() and xhci_ring_device() in
hub control and bus suspend functions since all device suspend and
resume have been notified through device_suspend/device_resume interfaces.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c | 47 ---------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index a83e82e..f12e1b7 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -704,7 +704,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	u32 temp, status;
 	int retval = 0;
 	__le32 __iomem **port_array;
-	int slot_id;
 	struct xhci_bus_state *bus_state;
 	u16 link_state = 0;
 	u16 wake_mask = 0;
@@ -818,17 +817,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				goto error;
 			}
 
-			slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-					wIndex + 1);
-			if (!slot_id) {
-				xhci_warn(xhci, "slot_id is zero\n");
-				goto error;
-			}
-			/* unlock to execute stop endpoint commands */
-			spin_unlock_irqrestore(&xhci->lock, flags);
-			xhci_stop_device(xhci, slot_id, 1);
-			spin_lock_irqsave(&xhci->lock, flags);
-
 			xhci_set_link_state(xhci, port_array, wIndex, XDEV_U3);
 
 			spin_unlock_irqrestore(&xhci->lock, flags);
@@ -876,19 +864,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				goto error;
 			}
 
-			if (link_state == USB_SS_PORT_LS_U3) {
-				slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-						wIndex + 1);
-				if (slot_id) {
-					/* unlock to execute stop endpoint
-					 * commands */
-					spin_unlock_irqrestore(&xhci->lock,
-								flags);
-					xhci_stop_device(xhci, slot_id, 1);
-					spin_lock_irqsave(&xhci->lock, flags);
-				}
-			}
-
 			xhci_set_link_state(xhci, port_array, wIndex,
 						link_state);
 
@@ -994,14 +969,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 							XDEV_U0);
 			}
 			bus_state->port_c_suspend |= 1 << wIndex;
-
-			slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-					wIndex + 1);
-			if (!slot_id) {
-				xhci_dbg(xhci, "slot_id is zero\n");
-				goto error;
-			}
-			xhci_ring_device(xhci, slot_id);
 			break;
 		case USB_PORT_FEAT_C_SUSPEND:
 			bus_state->port_c_suspend &= ~(1 << wIndex);
@@ -1133,20 +1100,12 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 	while (port_index--) {
 		/* suspend the port if the port is not suspended */
 		u32 t1, t2;
-		int slot_id;
 
 		t1 = readl(port_array[port_index]);
 		t2 = xhci_port_state_to_neutral(t1);
 
 		if ((t1 & PORT_PE) && !(t1 & PORT_PLS_MASK)) {
 			xhci_dbg(xhci, "port %d not suspended\n", port_index);
-			slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-					port_index + 1);
-			if (slot_id) {
-				spin_unlock_irqrestore(&xhci->lock, flags);
-				xhci_stop_device(xhci, slot_id, 1);
-				spin_lock_irqsave(&xhci->lock, flags);
-			}
 			t2 &= ~PORT_PLS_MASK;
 			t2 |= PORT_LINK_STROBE | XDEV_U3;
 			set_bit(port_index, &bus_state->bus_suspended);
@@ -1207,7 +1166,6 @@ int xhci_bus_resume(struct usb_hcd *hcd)
 		/* Check whether need resume ports. If needed
 		   resume port and disable remote wakeup */
 		u32 temp;
-		int slot_id;
 
 		temp = readl(port_array[port_index]);
 		if (DEV_SUPERSPEED(temp))
@@ -1240,11 +1198,6 @@ int xhci_bus_resume(struct usb_hcd *hcd)
 			/* Clear PLC */
 			xhci_test_and_clear_bit(xhci, port_array, port_index,
 						PORT_PLC);
-
-			slot_id = xhci_find_slot_id_by_port(hcd,
-					xhci, port_index + 1);
-			if (slot_id)
-				xhci_ring_device(xhci, slot_id);
 		} else
 			writel(temp, port_array[port_index]);
 	}
-- 
2.1.0

Re: [PATCH v5 2/3] usb: xhci: implement device_suspend/device_resume entries

[Greg Kroah-Hartman]

On Sat, May 09, 2015 at 09:15:29AM +0800, Lu Baolu wrote:
> This patch implements device_suspend/device_resume entries for xHC driver.
> device_suspend will be called when a USB device is about to suspend. It
> will issue a stop endpoint command for each endpoint in this device. The
> Suspend(SP) bit in the command TRB will set which will give xHC a hint
> about the suspend. device_resume will be called when a USB device is just
> resumed. It will ring doorbells of all endpoint unconditionally. XHC may
> use these suspend/resume hints to optimize its operation.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/usb/host/xhci-hub.c |  2 +-
>  drivers/usb/host/xhci.c     | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci.h     |  9 +++++++++
>  3 files changed, 48 insertions(+), 1 deletion(-)

I want to see these come through Mathias, after he has verified that
they work properly.  I'll take them that way.

thanks,

greg k-h

Re: [PATCH v5 3/3] usb: xhci: remove stop device and ring doorbell in hub control and bus suspend

[Mathias Nyman]

On 09.05.2015 04:15, Lu Baolu wrote:
> There is no need to call xhci_stop_device() and xhci_ring_device() in
> hub control and bus suspend functions since all device suspend and
> resume have been notified through device_suspend/device_resume interfaces.

I was looking through this code again before sending it forward, and it occurred to
me that this might be breaking the PORT_SUSPEND and PORT_SET_LINK_STATE port features
for xhci root hub.

In normal use these requests are called by usb core in usb_port_suspend(), which 
also now notifies xhci, which makes sure xhci_stop_device() is called. 

But I don't think there is anything preventing an URB to be sent to the xhci roothub
with a PORT_SUSPEND or PORT_SET_LINK_STATE port feature request. In this case the usb_port_suspend()
is not called, and no notify will stop the device. 

For example hub validation tests might do this

-Mathias

Re: [PATCH v5 3/3] usb: xhci: remove stop device and ring doorbell in hub control and bus suspend

[Lu, Baolu]

On 06/25/2015 10:53 PM, Mathias Nyman wrote:
> On 09.05.2015 04:15, Lu Baolu wrote:
>> There is no need to call xhci_stop_device() and xhci_ring_device() in
>> hub control and bus suspend functions since all device suspend and
>> resume have been notified through device_suspend/device_resume interfaces.
> I was looking through this code again before sending it forward, and it occurred to
> me that this might be breaking the PORT_SUSPEND and PORT_SET_LINK_STATE port features
> for xhci root hub.
>
> In normal use these requests are called by usb core in usb_port_suspend(), which
> also now notifies xhci, which makes sure xhci_stop_device() is called.
>
> But I don't think there is anything preventing an URB to be sent to the xhci roothub
> with a PORT_SUSPEND or PORT_SET_LINK_STATE port feature request. In this case the usb_port_suspend()
> is not called, and no notify will stop the device.
>
> For example hub validation tests might do this.

If that, we can drop this patch. It doesn't impact the other two patches 
in this patch series.

Thanks,
Baolu
>
> -Mathias
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>