[RFC][PATCH 0/3] usb: add a hcd notify entry in hc_driver

[RFC][PATCH 0/3] usb: add a hcd notify entry in hc_driver

[Lu Baolu]

This patch adds a new entry pointer in hc_driver. With this new entry,
USB core can notify host driver when something happens and host driver
is willing to be notified. One use case of this interface comes from
xHCI compatible host controller driver.

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 notify interface to notify xHCI driver whenever a USB
device's power state is changed.

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.


Lu Baolu (3):
  usb: add a hcd notify entry in hc_driver
  usb: xhci: implement hc_driver notify entry
  usb: xhci: cleanup unnecessary stop device and ring doorbell
    operations

 drivers/usb/core/generic.c  | 10 +++++++--
 drivers/usb/core/hcd.c      | 23 +++++++++++++++++++++
 drivers/usb/host/xhci-hub.c | 49 +--------------------------------------------
 drivers/usb/host/xhci.c     | 28 ++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  3 +++
 include/linux/usb/hcd.h     | 11 +++++++++-
 6 files changed, 73 insertions(+), 51 deletions(-)

-- 
2.1.0

[RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Lu Baolu]

This patch adds a new entry pointer in hc_driver. With this new entry,
USB core can notify host driver when something happens and host driver
is willing to be notified. One use case of this interface comes from
xHCI compatible host controller driver.

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 notify interface to notify xHCI driver whenever a USB
device's power state is changed.

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>
---
 drivers/usb/core/generic.c | 10 ++++++++--
 drivers/usb/core/hcd.c     | 23 +++++++++++++++++++++++
 include/linux/usb/hcd.h    | 11 ++++++++++-
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 358ca8d..92bee33 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
 	/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
 	else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
 		rc = 0;
-	else
+	else {
+		hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
 		rc = usb_port_suspend(udev, msg);
+		if (rc)
+			hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
+	}
 
 	return rc;
 }
@@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg)
 	 */
 	if (!udev->parent)
 		rc = hcd_bus_resume(udev, msg);
-	else
+	else {
 		rc = usb_port_resume(udev, msg);
+		hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
+	}
 	return rc;
 }
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..725d611 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2289,6 +2289,29 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
 
+/**
+ * hcd_notify - notify hcd driver with a message
+ * @udev: USB device
+ * @type: message type of this notification
+ * @data: message type specific data
+ *
+ * Call back to hcd driver to notify an event.
+ */
+void hcd_notify(struct usb_device *udev,
+		enum hcd_notification_type type, void *data)
+{
+	struct usb_hcd *hcd;
+
+	if (!udev)
+		return;
+
+	hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->notify)
+		hcd->driver->notify(hcd, udev, type, data);
+}
+EXPORT_SYMBOL_GPL(hcd_notify);
+
 #endif	/* CONFIG_PM */
 
 /*-------------------------------------------------------------------------*/
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 68b1e83..bdb422c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -76,6 +76,11 @@ struct giveback_urb_bh {
 	struct usb_host_endpoint *completing_ep;
 };
 
+enum hcd_notification_type {
+	HCD_MSG_DEV_SUSPEND = 0,		/* USB device suspend */
+	HCD_MSG_DEV_RESUME,			/* USB device resume */
+};
+
 struct usb_hcd {
 
 	/*
@@ -383,7 +388,9 @@ 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 to notify hcd when it is necessary. */
+	void	(*notify)(struct usb_hcd *, struct usb_device *,
+			enum hcd_notification_type, void *);
 };
 
 static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
@@ -632,6 +639,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_notify(struct usb_device *udev,
+		enum hcd_notification_type type, void *data);
 #else
 static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
 {
-- 
2.1.0

[RFC][PATCH 2/3] usb: xhci: implement hc_driver notify entry

[Lu Baolu]

This patch implements the notify entry defined in hc_driver for xHC
driver. For HCD_MSG_DEV_SUSPEND message, it will issue stop endpoint
command for each endpoint in the device. The Suspend(SP) bit in the
command TRB will be set, which gives xHC a hint about the suspend.
For HCD_MSG_DEV_RESUME, it will ring doorbells for all endpoints
unconditionally. XHC may use these hints to optimize its operation
on endpoint state cashes.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 28 ++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  3 +++
 3 files changed, 32 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..1e4aa78 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4917,6 +4917,29 @@ error:
 }
 EXPORT_SYMBOL_GPL(xhci_gen_setup);
 
+void xhci_notify(struct usb_hcd *hcd, struct usb_device *udev,
+		enum hcd_notification_type type, void *data)
+{
+	struct xhci_hcd	*xhci;
+
+	xhci = hcd_to_xhci(hcd);
+	if (!xhci || !xhci->devs[udev->slot_id])
+		return;
+
+	switch (type) {
+	case HCD_MSG_DEV_SUSPEND:
+		xhci_stop_device(xhci, udev->slot_id, 1);
+		break;
+	case HCD_MSG_DEV_RESUME:
+		xhci_ring_device(xhci, udev->slot_id);
+		break;
+	default:
+		xhci_dbg(xhci, "unknown notification message %d.\n", type);
+		break;
+	}
+
+}
+
 static const struct hc_driver xhci_hc_driver = {
 	.description =		"xhci-hcd",
 	.product_desc =		"xHCI Host Controller",
@@ -4976,6 +4999,11 @@ 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,
+
+	/*
+	 * notification call back
+	 */
+	.notify =		xhci_notify,
 };
 
 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..f71c643 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1867,10 +1867,13 @@ 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);
 
+void xhci_notify(struct usb_hcd *hcd, struct usb_device *udev,
+		enum hcd_notification_type type, void *data);
 #endif /* __LINUX_XHCI_HCD_H */
-- 
2.1.0

[RFC][PATCH 3/3] usb: xhci: cleanup unnecessary stop device and ring doorbell operations

[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 the new notify entry in hc_driver.

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: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Greg Kroah-Hartman]

On Mon, May 04, 2015 at 11:15:30AM +0800, Lu Baolu wrote:
> This patch adds a new entry pointer in hc_driver. With this new entry,
> USB core can notify host driver when something happens and host driver
> is willing to be notified. One use case of this interface comes from
> xHCI compatible host controller driver.
> 
> 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 notify interface to notify xHCI driver whenever a USB
> device's power state is changed.
> 
> 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>
> ---
>  drivers/usb/core/generic.c | 10 ++++++++--
>  drivers/usb/core/hcd.c     | 23 +++++++++++++++++++++++
>  include/linux/usb/hcd.h    | 11 ++++++++++-
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index 358ca8d..92bee33 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
>  	/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
>  	else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
>  		rc = 0;
> -	else
> +	else {
> +		hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
>  		rc = usb_port_suspend(udev, msg);
> +		if (rc)
> +			hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
> +	}
>  
>  	return rc;
>  }
> @@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg)
>  	 */
>  	if (!udev->parent)
>  		rc = hcd_bus_resume(udev, msg);
> -	else
> +	else {
>  		rc = usb_port_resume(udev, msg);
> +		hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
> +	}
>  	return rc;
>  }
>  
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 45a915c..725d611 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2289,6 +2289,29 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
>  
> +/**
> + * hcd_notify - notify hcd driver with a message
> + * @udev: USB device
> + * @type: message type of this notification
> + * @data: message type specific data
> + *
> + * Call back to hcd driver to notify an event.
> + */
> +void hcd_notify(struct usb_device *udev,
> +		enum hcd_notification_type type, void *data)
> +{
> +	struct usb_hcd *hcd;
> +
> +	if (!udev)
> +		return;
> +
> +	hcd = bus_to_hcd(udev->bus);
> +
> +	if (hcd->driver->notify)
> +		hcd->driver->notify(hcd, udev, type, data);
> +}
> +EXPORT_SYMBOL_GPL(hcd_notify);

Why does this have to be exported?

Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Alan Stern]

On Mon, 4 May 2015, Lu Baolu wrote:

> This patch adds a new entry pointer in hc_driver. With this new entry,
> USB core can notify host driver when something happens and host driver
> is willing to be notified. One use case of this interface comes from
> xHCI compatible host controller driver.

"Notify" is too generic.  It's better to make the callback routine
specific to the activity in question.  So this patch series should add
"device_suspend" and "device_resume" callbacks, not a general "notify"
callback.

> 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 notify interface to notify xHCI driver whenever a USB
> device's power state is changed.
> 
> 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>
> ---
>  drivers/usb/core/generic.c | 10 ++++++++--
>  drivers/usb/core/hcd.c     | 23 +++++++++++++++++++++++
>  include/linux/usb/hcd.h    | 11 ++++++++++-
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index 358ca8d..92bee33 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
>  	/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
>  	else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
>  		rc = 0;
> -	else
> +	else {
> +		hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
>  		rc = usb_port_suspend(udev, msg);
> +		if (rc)
> +			hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
> +	}
>  
>  	return rc;
>  }
> @@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg)
>  	 */
>  	if (!udev->parent)
>  		rc = hcd_bus_resume(udev, msg);
> -	else
> +	else {
>  		rc = usb_port_resume(udev, msg);
> +		hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
> +	}

Don't you want to tell the HCD about the resume _before_ it happens?  
After all, the devices endpoint will be used as soon as the resume 
takes place.

And besides, this should be symmetrical with the code above, where you
tell the HCD about a suspend _after_ it happens.

Alan Stern

Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Lu, Baolu]

Hi Alan,

Thanks for your review comments. Below has my response inline.

On 05/04/2015 10:28 PM, Alan Stern wrote:
> On Mon, 4 May 2015, Lu Baolu wrote:
>
>> This patch adds a new entry pointer in hc_driver. With this new entry,
>> USB core can notify host driver when something happens and host driver
>> is willing to be notified. One use case of this interface comes from
>> xHCI compatible host controller driver.
> "Notify" is too generic.  It's better to make the callback routine
> specific to the activity in question.  So this patch series should add
> "device_suspend" and "device_resume" callbacks, not a general "notify"
> callback.
Fair enough. I will make this change in v2 if there is no objection.

>
>> 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 notify interface to notify xHCI driver whenever a USB
>> device's power state is changed.
>>
>> 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>
>> ---
>>   drivers/usb/core/generic.c | 10 ++++++++--
>>   drivers/usb/core/hcd.c     | 23 +++++++++++++++++++++++
>>   include/linux/usb/hcd.h    | 11 ++++++++++-
>>   3 files changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
>> index 358ca8d..92bee33 100644
>> --- a/drivers/usb/core/generic.c
>> +++ b/drivers/usb/core/generic.c
>> @@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
>>   	/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
>>   	else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
>>   		rc = 0;
>> -	else
>> +	else {
>> +		hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
>>   		rc = usb_port_suspend(udev, msg);
>> +		if (rc)
>> +			hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
>> +	}
>>   
>>   	return rc;
>>   }
>> @@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg)
>>   	 */
>>   	if (!udev->parent)
>>   		rc = hcd_bus_resume(udev, msg);
>> -	else
>> +	else {
>>   		rc = usb_port_resume(udev, msg);
>> +		hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
>> +	}
> Don't you want to tell the HCD about the resume _before_ it happens?
> After all, the devices endpoint will be used as soon as the resume
> takes place.
The order that software should do during device suspend/resume is 
defined in 4.15.1.1 of xHCI spec 1.1.

Spec 4.15.1.1:

Software shall stop all endpoints of a device using the Stop Endpoint 
Command and setting the Suspend
(SP) flag to ‘1’ prior to selectively suspending a device. After the 
device is resumed software shall ring an
endpoint’s doorbell to restart it.

--end--

So the order looks like:

tell hcd device suspend
usb_port_suspend()

usb_port_resume()
tell hcd device resume

>
> And besides, this should be symmetrical with the code above, where you
> tell the HCD about a suspend _after_ it happens.
This was done in above change in generic_suspend():

@@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
  	/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
  	else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
  		rc = 0;
-	else
+	else {
+		hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
  		rc = usb_port_suspend(udev, msg);
+		if (rc)
+			hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
+	}
  
  	return rc;
  }

If usb_port_suspend() returns error, we should roll back HCD to the previous state.



>
> Alan Stern

Thanks again,
Baolu

Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1371 bytes --]

On Tue, 5 May 2015, Lu, Baolu wrote:

> The order that software should do during device suspend/resume is 
> defined in 4.15.1.1 of xHCI spec 1.1.
> 
> Spec 4.15.1.1:
> 
> Software shall stop all endpoints of a device using the Stop Endpoint 
> Command and setting the Suspend
> (SP) flag to ‘1’ prior to selectively suspending a device.

But _after_ all the URBs sent to the device have completed, right?

>  After the 
> device is resumed software shall ring an
> endpoint’s doorbell to restart it.

The driver would ring the endpoint's doorbell anyway when a new URB is 
submitted, wouldn't it?  Which means the resume callback doesn't 
actually have to do anything.

> --end--
> 
> So the order looks like:
> 
> tell hcd device suspend
> usb_port_suspend()

You have forgotten that usb_port_suspend() can send URBs to the device 
(to enable remote wakeup, for example).  Therefore you shouldn't notify 
the HCD until usb_port_suspend() is partly or totally finished.

> usb_port_resume()
> tell hcd device resume

You have also forgotten that usb_port_resume() calls various functions
that send URBs to ep0 on the device.  Therefore if the HCD's
device_resume callback needs to do something (like ringing ep0's
doorbell), you had better invoke the callback _before_ calling
usb_port_resume().  Or maybe you had better do this _within_
usb_port_resume().

Alan Stern

Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Lu, Baolu]

On 05/04/2015 04:14 PM, Greg Kroah-Hartman wrote:
> On Mon, May 04, 2015 at 11:15:30AM +0800, Lu Baolu wrote:
>> This patch adds a new entry pointer in hc_driver. With this new entry,
>> USB core can notify host driver when something happens and host driver
>> is willing to be notified. One use case of this interface comes from
>> xHCI compatible host controller driver.
>>
>> 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 notify interface to notify xHCI driver whenever a USB
>> device's power state is changed.
>>
>> 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>
>> ---
>>   drivers/usb/core/generic.c | 10 ++++++++--
>>   drivers/usb/core/hcd.c     | 23 +++++++++++++++++++++++
>>   include/linux/usb/hcd.h    | 11 ++++++++++-
>>   3 files changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
>> index 358ca8d..92bee33 100644
>> --- a/drivers/usb/core/generic.c
>> +++ b/drivers/usb/core/generic.c
>> @@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
>>   	/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
>>   	else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
>>   		rc = 0;
>> -	else
>> +	else {
>> +		hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
>>   		rc = usb_port_suspend(udev, msg);
>> +		if (rc)
>> +			hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
>> +	}
>>   
>>   	return rc;
>>   }
>> @@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg)
>>   	 */
>>   	if (!udev->parent)
>>   		rc = hcd_bus_resume(udev, msg);
>> -	else
>> +	else {
>>   		rc = usb_port_resume(udev, msg);
>> +		hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
>> +	}
>>   	return rc;
>>   }
>>   
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 45a915c..725d611 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2289,6 +2289,29 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
>>   
>> +/**
>> + * hcd_notify - notify hcd driver with a message
>> + * @udev: USB device
>> + * @type: message type of this notification
>> + * @data: message type specific data
>> + *
>> + * Call back to hcd driver to notify an event.
>> + */
>> +void hcd_notify(struct usb_device *udev,
>> +		enum hcd_notification_type type, void *data)
>> +{
>> +	struct usb_hcd *hcd;
>> +
>> +	if (!udev)
>> +		return;
>> +
>> +	hcd = bus_to_hcd(udev->bus);
>> +
>> +	if (hcd->driver->notify)
>> +		hcd->driver->notify(hcd, udev, type, data);
>> +}
>> +EXPORT_SYMBOL_GPL(hcd_notify);
> Why does this have to be exported?
It doesn't have to be exported. I will remove it in v2.

>
>

Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

[Lu, Baolu]

On 05/05/2015 10:50 PM, Alan Stern wrote:
> On Tue, 5 May 2015, Lu, Baolu wrote:
>
>> The order that software should do during device suspend/resume is
>> defined in 4.15.1.1 of xHCI spec 1.1.
>>
>> Spec 4.15.1.1:
>>
>> Software shall stop all endpoints of a device using the Stop Endpoint
>> Command and setting the Suspend
>> (SP) flag to 1 prior to selectively suspending a device.
> But _after_ all the URBs sent to the device have completed, right?
Yes, that's right.

>
>>   After the
>> device is resumed software shall ring an
>> endpoint's doorbell to restart it.
> The driver would ring the endpoint's doorbell anyway when a new URB is
> submitted, wouldn't it?  Which means the resume callback doesn't
> actually have to do anything.
The value of ringing door bell after resume is that hcd can fetch
endpoint state from memory to cache and get ready for transfer
as early as possible.

>
>> --end--
>>
>> So the order looks like:
>>
>> tell hcd device suspend
>> usb_port_suspend()
> You have forgotten that usb_port_suspend() can send URBs to the device
> (to enable remote wakeup, for example).  Therefore you shouldn't notify
> the HCD until usb_port_suspend() is partly or totally finished.
Yes, I agree with you. I will move the notification into usb_port_suspend(),
just before sending suspend request to parent hub.

>
>> usb_port_resume()
>> tell hcd device resume
> You have also forgotten that usb_port_resume() calls various functions
> that send URBs to ep0 on the device.  Therefore if the HCD's
> device_resume callback needs to do something (like ringing ep0's
> doorbell), you had better invoke the callback _before_ calling
> usb_port_resume().  Or maybe you had better do this _within_
> usb_port_resume().
Agree. I will do this within usb_port_resume() just after completing
sending clear port feature.

>
> Alan Stern
Thank you for the comments. I will send the v2 patch series with all
your comments addressed.

Thanks,
Baolu

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