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

[PATCH v3 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:
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      | 29 +++++++++++++++++++++++++++
 drivers/usb/core/hub.c      |  5 +++++
 drivers/usb/host/xhci-hub.c | 49 +--------------------------------------------
 drivers/usb/host/xhci.c     | 40 ++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     | 11 ++++++++++
 include/linux/usb/hcd.h     | 10 ++++++++-
 6 files changed, 95 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH v3 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>
---
 drivers/usb/core/hcd.c  | 29 +++++++++++++++++++++++++++++
 drivers/usb/core/hub.c  |  5 +++++
 include/linux/usb/hcd.h | 10 +++++++++-
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..a4cfc2d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2289,6 +2289,35 @@ 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
+ * @msg: Power Management message describing this state transition
+ *
+ * Call back to hcd driver to notify that a USB device is going to suspend.
+ */
+void hcd_suspend_notify(struct usb_device *udev, pm_message_t msg)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->device_suspend)
+		hcd->driver->device_suspend(hcd, udev, msg);
+}
+
+/**
+ * hcd_resume_notify - notify hcd driver when a device is just resumed
+ * @udev: USB device just resumed
+ * @msg: Power Management message describing this state transition
+ *
+ * Call back to hcd driver to notify that a USB device is just resumed.
+ */
+void hcd_resume_notify(struct usb_device *udev, pm_message_t msg)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->device_resume)
+		hcd->driver->device_resume(hcd, udev, msg);
+}
 #endif	/* CONFIG_PM */
 
 /*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3b71516..b3f0dc8 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3144,6 +3144,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 			goto err_lpm3;
 	}
 
+	hcd_suspend_notify(udev, msg);
+
 	/* see 7.1.7.6 */
 	if (hub_is_superspeed(hub->hdev))
 		status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
@@ -3169,6 +3171,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, msg);
+
 		/* Try to enable USB3 LPM and LTM again */
 		usb_unlocked_enable_lpm(udev);
  err_lpm3:
@@ -3422,6 +3426,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	}
 
  SuspendCleared:
+	hcd_resume_notify(udev, msg);
 	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..621d9b7 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -383,7 +383,13 @@ 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,
+			pm_message_t msg);
+	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
+			pm_message_t msg);
 };
 
 static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
@@ -632,6 +638,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, pm_message_t msg);
+extern void hcd_resume_notify(struct usb_device *udev, pm_message_t msg);
 #else
 static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
 {
-- 
2.1.0

[PATCH v3 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     | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     | 11 +++++++++++
 3 files changed, 52 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..6b61833 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4680,6 +4680,40 @@ 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, pm_message_t msg)
+{
+	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, pm_message_t msg)
+{
+	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 +5010,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..d826439 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1867,10 +1867,21 @@ 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, pm_message_t msg);
+void xhci_device_resume(struct usb_hcd *hcd,
+		struct usb_device *udev, pm_message_t msg);
+#else
+#define xhci_device_suspend	NULL
+#define xhci_device_resume	NULL
+#endif /* CONFIG_PM */
+
 #endif /* __LINUX_XHCI_HCD_H */
-- 
2.1.0

[PATCH v3 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 v3 2/3] usb: xhci: implement device_suspend/device_resume entries

[Greg Kroah-Hartman]

On Thu, May 07, 2015 at 10:18:15AM +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     | 40 ++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci.h     | 11 +++++++++++
>  3 files changed, 52 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..6b61833 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4680,6 +4680,40 @@ 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, pm_message_t msg)

You never use the 'msg' field, so please remove it from the callback.

> +{
> +	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, pm_message_t msg)

Same here.

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

[Lu, Baolu]

On 05/08/2015 04:47 AM, Greg Kroah-Hartman wrote:
> On Thu, May 07, 2015 at 10:18:15AM +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     | 40 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/usb/host/xhci.h     | 11 +++++++++++
>>   3 files changed, 52 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..6b61833 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4680,6 +4680,40 @@ 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, pm_message_t msg)
> You never use the 'msg' field, so please remove it from the callback.

'msg' could be used in another patch to determine the pm event types:
auto-suspend or system suspend. I didn't bring up that patch together
with this patch series because 1) that's part of xhci 1.1 features, and
2) it's still not tested yet.

>
>> +{
>> +	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, pm_message_t msg)
> Same here.

I will remove 'msg' field in this call back. I don't see any cases where
msg could be used.

Thank you,
Baolu

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

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

[Greg Kroah-Hartman]

On Fri, May 08, 2015 at 09:23:40AM +0800, Lu, Baolu wrote:
> 
> 
> On 05/08/2015 04:47 AM, Greg Kroah-Hartman wrote:
> >On Thu, May 07, 2015 at 10:18:15AM +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     | 40 ++++++++++++++++++++++++++++++++++++++++
> >>  drivers/usb/host/xhci.h     | 11 +++++++++++
> >>  3 files changed, 52 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..6b61833 100644
> >>--- a/drivers/usb/host/xhci.c
> >>+++ b/drivers/usb/host/xhci.c
> >>@@ -4680,6 +4680,40 @@ 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, pm_message_t msg)
> >You never use the 'msg' field, so please remove it from the callback.
> 
> 'msg' could be used in another patch to determine the pm event types:
> auto-suspend or system suspend. I didn't bring up that patch together
> with this patch series because 1) that's part of xhci 1.1 features, and
> 2) it's still not tested yet.

We don't add "features" to kernel code that is not used when it is
added.  We can always change function calls later as we do not have a
stable api at all.

thanks,

greg k-h

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

[Lu, Baolu]

On 05/08/2015 03:28 PM, Greg Kroah-Hartman wrote:
> On Fri, May 08, 2015 at 09:23:40AM +0800, Lu, Baolu wrote:
>>
>> On 05/08/2015 04:47 AM, Greg Kroah-Hartman wrote:
>>> On Thu, May 07, 2015 at 10:18:15AM +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     | 40 ++++++++++++++++++++++++++++++++++++++++
>>>>   drivers/usb/host/xhci.h     | 11 +++++++++++
>>>>   3 files changed, 52 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..6b61833 100644
>>>> --- a/drivers/usb/host/xhci.c
>>>> +++ b/drivers/usb/host/xhci.c
>>>> @@ -4680,6 +4680,40 @@ 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, pm_message_t msg)
>>> You never use the 'msg' field, so please remove it from the callback.
>> 'msg' could be used in another patch to determine the pm event types:
>> auto-suspend or system suspend. I didn't bring up that patch together
>> with this patch series because 1) that's part of xhci 1.1 features, and
>> 2) it's still not tested yet.
> We don't add "features" to kernel code that is not used when it is
> added.  We can always change function calls later as we do not have a
> stable api at all.

Understand it now. I will remove 'msg' fields in v4 patch series.

>
> thanks,
>
> greg k-h

Thank you.
Baolu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>